-
-
Notifications
You must be signed in to change notification settings - Fork 24
feature: create a feature to allow easy anonymization of donors #619
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
base: main
Are you sure you want to change the base?
Conversation
tudoramariei
commented
Jan 13, 2026
- create a feature to allow the anonymization of all the "old" donations (>1/2 years old)
- allow anonymization of a donation in different contexts in the admin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request introduces a donor anonymization feature that allows administrators to anonymize donation data for privacy compliance, particularly for donations older than 1-2 years. The feature includes both manual and bulk anonymization capabilities through the Django admin interface.
Changes:
- Added donor anonymization functionality with async/sync execution support
- Created crypto helper utilities to centralize encryption/decryption operations
- Introduced admin actions for anonymizing individual and bulk donations
- Added time constants (MONTH, YEAR) for date calculations in anonymization logic
Reviewed changes
Copilot reviewed 11 out of 14 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/utils/constants/time.py | Added MONTH and YEAR constants for time-based calculations |
| backend/utils/common/crypto_helper.py | Created centralized encryption/decryption helper functions |
| backend/redirectioneaza/settings/environment.py | Added USER_ANONIMIZATION_METHOD configuration parameter |
| backend/redirectioneaza/settings/app_configs.py | Configured anonymization method with fallback to default run method |
| backend/donations/models/donors.py | Implemented anonymize() method and refactored encryption to use crypto_helper |
| backend/donations/admin/donors.py | Added anonymization admin actions and confirmation workflow |
| backend/donations/management/commands/generate_donations.py | Enhanced donation generation with PDF handling options |
| backend/locale/*/LC_MESSAGES/django.po | Added translations for anonymization messages |
| .idea/runConfigurations/*.xml | Updated IDE run configurations |
Files not reviewed (3)
- .idea/runConfigurations/compilemessages.xml: Language not supported
- .idea/runConfigurations/generate_donations_PDF_only.xml: Language not supported
- .idea/runConfigurations/makemessages.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if form.is_valid(): | ||
| result: dict = self._anonymize_donation(donor) | ||
|
|
||
| self.message_user(request, result["message"], level=DEFAULT_LEVELS.get(result["status"], messages.INFO)) |
Copilot
AI
Jan 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code uses DEFAULT_LEVELS.get() to map task result statuses to message levels, but DEFAULT_LEVELS is a dictionary that maps level names to integers (e.g., 'SUCCESS' -> 25, 'ERROR' -> 40), not the reverse. The task result flags like TASK_SUCCESS_FLAG are strings ("SUCCESS", "ERROR", etc.), which won't match the expected keys in DEFAULT_LEVELS. This will always fall back to messages.INFO, which may not be the intended behavior. Consider creating an explicit mapping like: {"SUCCESS": messages.SUCCESS, "ERROR": messages.ERROR, "SCHEDULED": messages.INFO}.
- create a feature to allow the anonymization of all the "old" donations (>1/2 years old) - allow anonymization of a donation in different contexts in the admin # Conflicts: # backend/locale/en/LC_MESSAGES/django.po # backend/locale/hy/LC_MESSAGES/django.po # backend/locale/ro/LC_MESSAGES/django.po
b39452a to
f3e5545
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 22 comments.
Files not reviewed (3)
- .idea/runConfigurations/03_02_generate_donations_PDF_only.xml: Language not supported
- .idea/runConfigurations/compilemessages.xml: Language not supported
- .idea/runConfigurations/makemessages.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def run_anonymize_old_donations(self, request): | ||
| async_wrapper(anonymize_old_donations, async_flag=settings.USER_ANONIMIZATION_METHOD) | ||
|
|
||
| self.message_user(request, _("Succesfully scheduled")) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error in success message. "Succesfully" should be "Successfully" (missing 's').
| class HasPersonalDataFilter(admin.SimpleListFilter): | ||
| title = _("Has personal data") | ||
| parameter_name = "has_personal_data" | ||
| horizontal = True |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "horizontal" attribute is not a valid attribute for Django's SimpleListFilter. This appears to be an attempt to configure UI layout, but it's not supported by Django admin filters and will be ignored or cause issues.
| horizontal = True |
| ENABLE_MULTIPLE_FORMS = env.bool("ENABLE_MULTIPLE_FORMS", True) | ||
| ENABLE_BYOF = env.bool("ENABLE_BYOF", False) | ||
| ENABLE_CSV_DOWNLOAD = env.bool("ENABLE_CSV_DOWNLOAD", False) | ||
| ENABLE_BULK_ANONIMIZATION = env.bool("ENABLE_BULK_ANONIMIZATION") |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature flag ENABLE_BULK_ANONIMIZATION lacks a default value. Unlike other feature flags in the same file (lines 6-8), this one doesn't provide a default value to env.bool(). This will cause an error if the environment variable is not set, rather than defaulting to a safe value like False.
| ENABLE_BULK_ANONIMIZATION = env.bool("ENABLE_BULK_ANONIMIZATION") | |
| ENABLE_BULK_ANONIMIZATION = env.bool("ENABLE_BULK_ANONIMIZATION", False) |
| return queryset.exclude(personal_data_removed__isnull=False) | ||
| if self.value() == "no": | ||
| return queryset.filter(personal_data_removed__isnull=False) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter logic appears to be incorrect. The field name should be "personal_data_removed_at" (not "personal_data_removed") to match the model field defined in the Donor model. This will cause a FieldError at runtime.
| return queryset.exclude(personal_data_removed__isnull=False) | |
| if self.value() == "no": | |
| return queryset.filter(personal_data_removed__isnull=False) | |
| return queryset.exclude(personal_data_removed_at__isnull=False) | |
| if self.value() == "no": | |
| return queryset.filter(personal_data_removed_at__isnull=False) |
| WEEK = 7 * DAY | ||
|
|
||
| DAY_IN_DAYS = 1 | ||
| WEEK_IN_DAYS = 1 * DAY_IN_DAYS |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming pattern is inconsistent and confusing. "WEEK_IN_DAYS = 1 * DAY_IN_DAYS" suggests it's 1 day, but it should be 7 days for a week. This should be "WEEK_IN_DAYS = 7 * DAY_IN_DAYS" to accurately represent a week.
| WEEK_IN_DAYS = 1 * DAY_IN_DAYS | |
| WEEK_IN_DAYS = 7 * DAY_IN_DAYS |
| ENABLE_FULL_VALIDATION_CNP=(bool, True), | ||
| # Feature flags | ||
| ENABLE_MULTIPLE_FORMS=(bool, False), | ||
| ENABLE_BULK_ANONIMIZATION=(bool, False), |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configuration key contains a spelling error. "ENABLE_BULK_ANONIMIZATION" should be "ENABLE_BULK_ANONYMIZATION" (missing 'y'). This should match the corrected spelling in the feature flag.
| ENABLE_BULK_ANONIMIZATION=(bool, False), | |
| ENABLE_BULK_ANONYMIZATION=(bool, False), |
| class HasNgoFilter(admin.SimpleListFilter): | ||
| title = _("Has NGO") | ||
| parameter_name = "has_ngo" | ||
| horizontal = True |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "horizontal" attribute is not a valid attribute for Django's SimpleListFilter. This appears to be an attempt to configure UI layout, but it's not supported by Django admin filters and will be ignored or cause issues.
| horizontal = True |
| result: dict = run_anonymize_donation_task(donor) | ||
|
|
||
| self.message_user(request, result["message"], level=DEFAULT_LEVELS.get(result["status"], messages.INFO)) | ||
|
|
||
| return redirect(reverse_lazy("admin:donations_donor_change", args=[donor.pk])) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The form validation doesn't actually verify that the user checked the confirmation checkbox. The form.is_valid() call will pass even if the checkbox isn't checked because MultipleChoiceField with required=True only requires the field to be present, not that a specific value is selected. The validation should check that 'yes' is in the cleaned_data for the confirmation field.
| result: dict = run_anonymize_donation_task(donor) | |
| self.message_user(request, result["message"], level=DEFAULT_LEVELS.get(result["status"], messages.INFO)) | |
| return redirect(reverse_lazy("admin:donations_donor_change", args=[donor.pk])) | |
| confirmation = form.cleaned_data.get("confirmation") or [] | |
| if "yes" in confirmation: | |
| result: dict = run_anonymize_donation_task(donor) | |
| self.message_user( | |
| request, | |
| result["message"], | |
| level=DEFAULT_LEVELS.get(result["status"], messages.INFO), | |
| ) | |
| return redirect(reverse_lazy("admin:donations_donor_change", args=[donor.pk])) | |
| else: | |
| messages.error(request, _("Please confirm the anonymization by checking the box.")) |
| except CommandError: | ||
| self.message_user(request, _("Error calling the command")) | ||
|
|
||
| self.message_user(request, _("Succesfully scheduled")) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error in success message. "Succesfully" should be "Successfully" (missing 's').
| return async_task(func, **kwargs) | ||
|
|
||
| return func(*args, **kwargs) | ||
| return func(**kwargs) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result of anonymize_old_donations is used even though it is always None.