-
Notifications
You must be signed in to change notification settings - Fork 8
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
Conversation
@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. |
cec1d6e
to
5381964
Compare
Previous implementation would cause an error in save_validation_error_in_data_source_bulk’s value.get('success', False) when uniqueness check is present
so it gets picked up automatically
Also modify where config is patched for individual upload test
5381964
to
806096a
Compare
Group takes on head’s location when it doesn’t have one, and group member assignment results in alignment with group location
806096a
to
2f90462
Compare
To minimize ambiguous location issue
f1afa57
to
38c8c4f
Compare
e23c88a
to
56d1413
Compare
Closing this as the same change set is included and merged in #137 |
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 attributeduplications
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 becomesCan 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?