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

Implement easy import of schools from the ME #248

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Igifigi
Copy link
Contributor

@Igifigi Igifigi commented Aug 21, 2023

This PR implements an algorithm that will bring order to school selection in SIO. Implemented script is used to import schools from the RSPO database, which is provided by the Ministry of Education and contains all educational institutions in Poland. Due to the poor quality of this data, it is likely that SIO administrators will modify school data. The task of this script is to detect which corrections from newer and more recent versions of the RSPO database should be accepted and applied, and which should not.

@Igifigi Igifigi requested a review from twalen as a code owner August 21, 2023 17:21
@Igifigi Igifigi marked this pull request as draft August 30, 2023 09:19
@twalen
Copy link
Contributor

twalen commented Aug 30, 2023

some more verbose commit messages might be useful (instead of "temp commit")

@Igifigi
Copy link
Contributor Author

Igifigi commented Aug 30, 2023

some more verbose commit messages might be useful (instead of "temp commit")

As you can see, this PR is set as a draft, so commit names will be changed soon (probably tomorrow).

@Igifigi Igifigi force-pushed the feature-easy_import_of_schools branch from 2db4b84 to fe33c00 Compare August 31, 2023 08:46
@Igifigi Igifigi marked this pull request as ready for review August 31, 2023 08:47
@Igifigi
Copy link
Contributor Author

Igifigi commented Aug 31, 2023

@twalen now it's ready for review :))

@DietPawel
Copy link
Contributor

DietPawel commented Aug 31, 2023

I am missing some migration related trickery to allow migrating a database with existing schools lacking rspo fields.
As well as I am not sure it is right to get rid of legacy non-rspo csv import.

@Igifigi Igifigi force-pushed the feature-easy_import_of_schools branch from fe33c00 to 168e6b8 Compare August 31, 2023 14:20
@Igifigi Igifigi force-pushed the feature-easy_import_of_schools branch 3 times, most recently from eecc457 to 121a358 Compare September 15, 2023 16:51
@Igifigi Igifigi force-pushed the feature-easy_import_of_schools branch from 121a358 to 57dcebd Compare September 15, 2023 17:27
Comment on lines +450 to +451
update_school(curr_rspo_school, db_school)
rows_affected += 1
Copy link
Member

Choose a reason for hiding this comment

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

Changes from MoE may introduce new duplicates.

Suggested change
update_school(curr_rspo_school, db_school)
rows_affected += 1
try:
with transaction.atomic():
update_school(curr_rspo_school, db_school)
rows_affected += 1
except utils.IntegrityError:
self.log(
f'Warning: Django found a duplicate school. Please manually check the school {db_school.rspo} data in SIO database, '
'current RSPO and the last backup. Something suspicious may be going on.'
)

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.

4 participants