-
Notifications
You must be signed in to change notification settings - Fork 689
feat: add option to email users about duplicate accounts. Fixes #8174. #9850
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9850 +/- ##
========================================
Coverage 88.54% 88.54%
========================================
Files 316 317 +1
Lines 42265 42449 +184
========================================
+ Hits 37423 37586 +163
- Misses 4842 4863 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jennifer-richards
left a comment
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.
I have a few comments inline, but the merge persons view needs a refactoring. It's currently dangerous enough that IMO it needs to be part of this PR.
The workflow currently looks like:
- GET the /person/merge/ page
- fill in the from/to fields with person IDs
- Submit with a GET, returning a page with the preview of the persons.
- Review and submit with a POST
However, in step 3, the from/to fields are still present and editable. Editing them does not update the preview, and submitting will merge persons based on the edited values without much warning. Obviously "don't do that," but it's an opportunity for a serious error that would be difficult to roll back.
This should instead be two views, one that is only selecting the persons to merge and another that is only previewing and submitting. It can have the current "swap" button and the new "send email" button in addition to the "Merge" button, but shouldn't allow editing the IDs to be affected. Probably it should add a "go back" type button to return to the original view.
LMK if that doesn't make sense
ietf/person/forms.py
Outdated
|
|
||
| def clean_to(self): | ||
| addresses = self.cleaned_data['to'] | ||
| return addresses.split(',') |
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.
Not ideal to be "cleaning" a CharField into a list in the clean_* method. That sort of transformation needs to be in a to_python() method of a custom field class. We have ietf.utils.fields.MultiEmailField that might be close to what you need, though it is probably in need of some clean up.
If that's not doing the right things, it'd be worth making your own field class here.
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.
It looks like this would work, except for the fact that validate_email doesn't handle emails in the form "IETF Secretariat ietf-secretariat-reply@ietf.org" which is the settings default from address. I could just use "ietf-secretariat-reply@ietf.org". What clean up are you referring to?
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.
Didn't look hard at cleanup, but it's written in old style / dialect that caught my eye (specifically, not using super().validate(...)). Not asking you to refactor it, just a comment.
I agree it doesn't do what you need. The main reason to point at it was just the structure it has. I.e., rather than doing data conversion in a clean() method, it'd be preferable to do something like
class MutliNameAddrField(forms.Field):
def to_python(self, value):
"""Split on commas into a list of maybe-valid name-addr strings, or raise ValidationError if it can't do that"""
# ...
def validate(self, value):
"""Do more detailed validation on the value that came out of to_python"""
# ...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.
https://docs.djangoproject.com/en/5.2/ref/forms/validation/#form-and-field-validation is a useful reference that it always takes me too long to find
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.
Ok. I mixed this up. For the send_mail_text() "to" and "reply-to", which expect lists, the existing MultiEmailField works fine. For the From address, which gets a name-addr style email address from settings.DEFAULT_FROM_EMAIL, I created a custom field NameAddrEmailField.
See commit 4212526
No description provided.