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

Feature: individual import allows location and checks user's row-level security #132

Closed
wants to merge 21 commits into from

Conversation

weilu
Copy link
Contributor

@weilu weilu commented Oct 22, 2024

  • validate_import_individuals (used by DataUploadWorkflow and DataUpdateWorkflow) --- services.py
  • import_individuals ---- views.py
  • download_template_file
  • download_invalid_items
  • download_individual_upload
  • base_individual_upload --- workflows
  • individual_upload_valid
  • base_individual_update
  • individual_update_valid

In the process of working on this feature I added tests to cover the existing functionalities of the template download, import, and import validation. I also came across some previous refactoring – I believe to address the performance issue of individual uniqueness validation – that is incomplete, as evidenced by the leftover unused code. There was also a bug uncovered by the added tests, so I'm putting up this draft PR for quick review by @sniedzielski first of the refactored change and bug fix.

Also, @sniedzielski, not sure if you are aware, the with code before this PR the uniqueness check doesn't use the code in calcrule_validations/strategies/deduplication_individual_validation_strategy.py anymore, to which you are the original author and instead the uniqueness validation result is simply national_id_uniqueness: false. This PR puts back the dictionary structure but doesn't put back the attribute duplications as it would involve lots of database calls (which I believe was where the performance optimization your teammate did), so now with the PR, the validation result becomes

"national_id_uniqueness":{
               "success": false,
               "field_name": "national_id",
               "note": "'national_id' Field value '1345320000AN' is duplicated",
}

Can you have a look at the relevant changes in this PR and let me know if it's ok? If so, I'll also go ahead and update the social_protection module's README to remove the duplications sample output to keep things consistent: https://github.com/openimis/openimis-be-social_protection_py/blob/b7c5359cf78898c061c7482bddff3c273adbf96f/README.md?plain=1#L202-L274

Should deduplication_individual_validation_strategy.py be removed from calcrule_validations if it's not used anymore?

@weilu weilu requested a review from sniedzielski October 22, 2024 20:57
@sniedzielski
Copy link
Contributor

@weilu Thanks for fixing the bugs and adding tests. To be honest, the uniqueness check in the validation module seems like overkill - of course because of our initial plan. It doesn't pass our expectation therefore it's good opportunity to rid out of it now. Your suggestion to simplify it is spot on for improving performance and clarity. I agree with removing the relevant README sections and handling uniqueness in the validation as you suggested in your initial comment on the PR. Please go ahead and continue with that.
Just let me know if you have final version.

sniedzielski
sniedzielski previously approved these changes Oct 24, 2024
@weilu weilu marked this pull request as ready for review October 31, 2024 11:24
@weilu weilu linked an issue Nov 1, 2024 that may be closed by this pull request
@weilu weilu force-pushed the feature/location-import branch from cec1d6e to 5381964 Compare November 1, 2024 20:32
@weilu weilu force-pushed the feature/location-import branch from 5381964 to 806096a Compare November 1, 2024 20:39
weilu added 3 commits November 3, 2024 10:21
Group takes on head’s location when it doesn’t have one, and
group member assignment results in alignment with group location
@weilu weilu force-pushed the feature/location-import branch from f1afa57 to 38c8c4f Compare November 25, 2024 22:22
@weilu weilu force-pushed the feature/location-import branch from e23c88a to 56d1413 Compare November 26, 2024 14:47
@weilu
Copy link
Contributor Author

weilu commented Nov 27, 2024

Closing this as the same change set is included and merged in #137

@weilu weilu closed this Nov 27, 2024
@weilu weilu deleted the feature/location-import branch November 27, 2024 11:41
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.

Allow edit of individual & group location at record level Add row-level security to individuals import
2 participants