-
Notifications
You must be signed in to change notification settings - Fork 81
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
Add a Contact Us page (T201159) #211
Conversation
Nice! It's probably worth adding a test to ensure that the contact form fires an email correctly. |
Added a test case with my limited understanding of testing to trigger the email function linked to the contact us form. |
Looking good! Just a minor thing - "comtact" rather than "contact" :D https://github.com/WikipediaLibrary/TWLight/pull/211/files#diff-e591183c0ca13c657d1ff08a1b13a093R359 |
Now up at https://twlight-staging.wmflabs.org/contact/ if you want to poke around. Seems to be working fine, except Aaron is frustrated because the boxes don't line up :D |
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.
This is mostly good. I do see a few issues though:
- We haven't thought the UX for the email field all the way through. We present the form before sign-in with empty, required input fields. If the user isn't signed in, we then throw away their input for the email field after sign in. Maybe we could really rethink this part. Some ideas for discussion:
- we remove this as an input field and simply use the email from the user profile directly. We basically set the email in the profile as a requirement, and give people a warning or redirect to fill that out if it's empty or invalid
- we discuss what integrating with wiki talk might look like
- we integrate with on wiki contact forms, eg https://meta.wikimedia.org/wiki/Special:EmailUser/username
- something else?
This doesn't seem to be the case for me - if I click Submit while logged out, I get logged in and returned to the page with my form data intact. |
I think Jason is referring to the use case where the user actually has their email in the platform which is written over their email field input after logging in? (assuming the two are different) |
Oh, right - I see that, yep. Honestly I like the first idea - we can display a 'you need to go set your email' message if they haven't set it. |
I think ideally we'd still display the email value (after sign in) as a locked/grey-out field (my recollection is that crispy makes this simple) with some help text stating that this comes from the profile. |
The email field no longer is directly editable by users, both logged in and not logged in. |
@uyscuti-wiki Did you mean to make your last commit in this PR? Feels like its unrelated to the Contact page, unless I'm missing something. |
Not my wisest decision, but in a weird way I meant to. I got sidetracked following this line because users won't be able to update or delete their email after landing on their user profiles. What was supposed to be a single I'll reset the branch to commit |
4c6c125
to
7c4fcf5
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.
This is looking good! Some feedback:
we should change the email field into a piece display markup within the form instead of processed form input. Right now it's disabled, but all someone has to due is remove those flags with the dev tools in their browser (or do acurl -xPOST
) and then they can submit whatever data they wish for the email field- we (maybe me?) should add the code for this to parse the email address from the user profile and do validation (e.g. kick the user back to the form with an error/warning as if they left a real form field empty/invalid)
- we (probably me?) should add the code to cc the user or set their email address as the reply-to.
About the first point, I thought so too, until I came across this - Django won't let users tamper with the data. What am I missing here? |
@uyscuti-wiki I stand corrected on that point! You're not missing anything, this is Django doing the right thing! Since we're not actually using that email address anywhere yet, I couldn't actually see the parsed value. I just modified it and saw that it succeeded. |
@uyscuti-wiki Do you want to work on any more steps for this or would you like to hand off? |
Not yet. I'll give it a whirl, see if I get anywhere. On a related note, can you point me to documentation/guides (if any) on djmail? Thanks for asking. |
django email docs: djmail docs: |
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.
See inline comments.
71a8312
to
24078ae
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.
This is great. Literally the only thing left is a slight tweak to the subject line. I added an inline suggestion. Once that's changed, we'll merge!
email subject tweak Co-Authored-By: uyscuti-wiki <uyscuti-wiki@users.noreply.github.com>
Thanks! |
A very basic working model of the contact us page. Probably needs some more work before it can be merged, but this is all I've got on my own.