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

Add enrollment preprocessing CLI tool #2011

Merged
merged 71 commits into from
Apr 8, 2024

Conversation

Kakadus
Copy link
Collaborator

@Kakadus Kakadus commented Aug 30, 2023

fix #1490

Upload my progress on the cli tool from monday.

  • There needs to be a link to the user export route somewhere.
  • There is probably a test missing for the cli tool.
  • The django-pylint plugin has a problem with linting the preprocessor (I manually linted it though)
  • codecov does not see the tools

tools/enrolment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrolment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrolment_preprocessor.py Outdated Show resolved Hide resolved
evap/staff/views.py Outdated Show resolved Hide resolved
evap/staff/tests/test_views.py Outdated Show resolved Hide resolved
@Kakadus Kakadus force-pushed the enrolment-preprocessor branch 2 times, most recently from 15021a3 to 09d6bf6 Compare October 9, 2023 19:13
@Kakadus Kakadus marked this pull request as ready for review October 23, 2023 15:53
@Kakadus Kakadus changed the title WIP: Add enrolment preprocessing CLI tool Add enrolment preprocessing CLI tool Oct 23, 2023
Copy link
Member

@richardebeling richardebeling left a comment

Choose a reason for hiding this comment

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

Looking good! :)

tools/enrollment_preprocessor.py Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
Copy link
Member

@richardebeling richardebeling left a comment

Choose a reason for hiding this comment

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

More in-depth this time, so a few more comments, but it's all very detail-y.

evap/staff/fixtures/excel_files_test_data.py Show resolved Hide resolved
evap/staff/tests/test_tools.py Outdated Show resolved Hide resolved
evap/staff/tests/test_tools.py Outdated Show resolved Hide resolved
evap/staff/tests/test_tools.py Outdated Show resolved Hide resolved
evap/staff/views.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
@Kakadus Kakadus changed the title Add enrolment preprocessing CLI tool Add enrollment preprocessing CLI tool Oct 30, 2023
@Kakadus Kakadus requested a review from janno42 February 12, 2024 21:21
evap/staff/tests/test_tools.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
@Kakadus
Copy link
Collaborator Author

Kakadus commented Feb 19, 2024

I were ably to combine initial excel parsing and conflict grouping, because of direct conversion from Cells to Users, but the update logic needed more complexity (Codacy is not very happy)

@richardebeling richardebeling self-requested a review February 23, 2024 16:41
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
tools/enrollment_preprocessor.py Outdated Show resolved Hide resolved
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

As discussed: The tool asks for the same user multiple times.

evap/staff/tests/test_tools.py Outdated Show resolved Hide resolved
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

Seems to work now :shipit:

@Kakadus
Copy link
Collaborator Author

Kakadus commented Mar 11, 2024

Just to be sure: Currently the whole userdata from the conflict is shown, so this question is possible:

First_name
---------
existing: 'Bastius' (Bastius Prorsus, [email protected])
imported: 'Lucilia' (Lucilia Manilium, [email protected])

Selecting imported only applies the conflicting field, here the first name (resulting data: Lucilia Prorsus, [email protected])
Of cause, you are then asked about Prorsus vs Manilium later.
Instead, we could show the resulting data always, so

existing: 'Bastius' (Bastius Prorsus, [email protected])
imported: 'Lucilia' (Lucilia Prorsus, [email protected])

is asked instead.

@janno42
Copy link
Member

janno42 commented Mar 11, 2024

We can leave that as it is now. I'd say the second way makes interpretation harder.

@Kakadus Kakadus merged commit f879b1c into e-valuation:main Apr 8, 2024
12 checks passed
@Kakadus Kakadus deleted the enrolment-preprocessor branch April 8, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Name change control on import
4 participants