Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: county field #2229

Merged
merged 16 commits into from
Nov 13, 2023
Merged

fix: county field #2229

merged 16 commits into from
Nov 13, 2023

Conversation

SKairinos
Copy link
Collaborator

@SKairinos SKairinos commented Nov 7, 2023

This change is Reviewable

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SKairinos)


cfl_common/common/tests/test_models.py line 5 at r1 (raw file):

from django.utils import timezone

from ..helpers.organisation import sanitise_uk_postcode

We shouldn't still need to import this, in fact you should be able to get rid of this helper altogether.


cfl_common/common/tests/test_models.py line 72 at r1 (raw file):

        assert teacher3 in school.admins()

    def test_sanitise_uk_postcode(self):

This can be removed too


portal/static/portal/js/school.js line 5 at r1 (raw file):

    $('#form-row-county').hide();
  }
  

Extra whitespace

Copy link
Collaborator Author

@SKairinos SKairinos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faucomte97)


cfl_common/common/tests/test_models.py line 5 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

We shouldn't still need to import this, in fact you should be able to get rid of this helper altogether.

Unfortunately, it cannot due to the way the code is currently structured. Migration 41 in common imports this helper. As a rule, migrations must never import our custom code for this very reason (it's okay for migrations to depend on 3rd party packages, like Django) - we have removed the postcode logic from the app but this postcode helper is needed by an old migration. When we restructure, we'll ensure all migrations are self contained.


cfl_common/common/tests/test_models.py line 72 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

This can be removed too

can't. see above comment about an old migration depending on this.


portal/static/portal/js/school.js line 5 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Extra whitespace

Done.

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SKairinos)

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Merging #2229 (19eccfd) into master (f992daf) will increase coverage by 0.00%.
The diff coverage is 86.04%.

❗ Current head 19eccfd differs from pull request most recent head 4f96204. Consider uploading reports for the commit 4f96204 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2229   +/-   ##
=======================================
  Coverage   94.12%   94.12%           
=======================================
  Files         169      170    +1     
  Lines        4595     4582   -13     
=======================================
- Hits         4325     4313   -12     
+ Misses        270      269    -1     
Files Coverage Δ
cfl_common/common/models.py 89.45% <100.00%> (-0.47%) ⬇️
cfl_common/common/tests/test_models.py 100.00% <100.00%> (ø)
cfl_common/common/tests/utils/organisation.py 100.00% <100.00%> (ø)
portal/admin.py 87.64% <100.00%> (ø)
portal/forms/organisation.py 100.00% <100.00%> (ø)
portal/views/organisation.py 98.21% <100.00%> (-0.04%) ⬇️
portal/views/teacher/dashboard.py 97.00% <100.00%> (+1.76%) ⬆️
...l_common/common/migrations/0047_school_location.py 60.00% <60.00%> (ø)

... and 2 files with indirect coverage changes

Copy link
Contributor

@faucomte97 faucomte97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SKairinos)

@SKairinos SKairinos merged commit 669ce8a into master Nov 13, 2023
4 checks passed
@SKairinos SKairinos deleted the county_field branch November 13, 2023 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants