-
Notifications
You must be signed in to change notification settings - Fork 83
Upgrades to contact messages #4845
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: master
Are you sure you want to change the base?
Conversation
a7107c2 to
138ecfc
Compare
c457b14 to
53750df
Compare
70f5a95 to
e7c4f6c
Compare
ajrbyers
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've added a few comments inline for you to go over, however, I have a fundamental question about this implementation. We have a contact form to obscure email addresses from being scraped. If we require a user to create a URL with the users email address in it we may be defeating this purpose. I think the account UUID field is the least painful option here. The Contacts page could present contact link that a user could simply copy and paste so they don't have to form it themselves?
Open to thoughts on this.
All good suggestions, thanks! Updated with some changes:
|
|
Note: Going to rebase this as there are conflicts keeping the CI job from running. |
4fe988c to
311b196
Compare
|
One other thing to be aware of in review: the migration in this PR wipes the client IP data on contact messages, for GDPR purposes. This is what we want, right? |
27d1754 to
a59fde6
Compare
mauromsl
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.
a few comments inline around the complexity of email comparisons and related data migrations.
src/core/forms/forms.py
Outdated
| label=_("Who would you like to contact?"), | ||
| ) | ||
| sender = forms.EmailField( | ||
| max_length=200, |
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.
we probably want a constant to keep Logentry.actor_email and this in sync.
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 can't work out what you mean. In sync in what way?
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.
Sorry, I meant the max_length, by using a constant, so that if one changes, the other does too. (since we copy from one to the other)
src/core/migrations/0110_rename_contacts_contactperson_and_more.py
Outdated
Show resolved
Hide resolved
src/core/migrations/0110_rename_contacts_contactperson_and_more.py
Outdated
Show resolved
Hide resolved
| matching_accounts = Account.objects.filter( | ||
| models.Q(username__iexact=email) | models.Q(email__iexact=email), | ||
| ) | ||
| if matching_accounts.exists(): | ||
| account = matching_accounts.first() | ||
| else: | ||
| normalized_email = normalize_email(email) | ||
| account = Account.objects.create( | ||
| email=normalized_email, | ||
| username=normalized_email, | ||
| ) |
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.
as above, consider querying against email.lower() and handle potential IntegrityError on Account.objects.create()
|
@mauromsl I've made some changes and responded above with some questions and comments. Remember to hit the "Load more" under hidden conversations to see the main one. |
src/core/forms/forms.py
Outdated
| label=_("Who would you like to contact?"), | ||
| ) | ||
| sender = forms.EmailField( | ||
| max_length=200, |
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.
Sorry, I meant the max_length, by using a constant, so that if one changes, the other does too. (since we copy from one to the other)
mauromsl
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.
replied to comments, we are nearly there
|
OK @mauromsl, I've addressed your comments. I've also tested it with a larger database and optimized the db access in the migration so it's quicker. There was also a bug with log entry dates because |
Closes #4138.
Closes #1720.
Please also review and merge docs PR: https://github.com/openlibhums/memory-alpha/pull/155.Notes
Screenshots for contact system
Screenshots for admin docs feature
I enabled this in the process of doing this work. It is enormously helpful to view a comprehensive list of things in Janeway sometimes.