Skip to content
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

Fix/login signup #359

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix/login signup #359

wants to merge 1 commit into from

Conversation

lennerd
Copy link
Collaborator

@lennerd lennerd commented Aug 15, 2016

This PR tries to fix #175, #179 and #181.

What did change?

  • The user email address is mandatory now. If a user is logged in for the first time or did not set an email address yet, we always show them after_signup_update page to add a email address.
    This part is really critical I think. We need to be sure to force user to have a email address. There is a quite big opportunity users get angry or frustrated because they are forced to give data to us.
    We also need to check if this has any implications for the apps on the mobile devices.
  • The user password was only needed so users were able to login via their mobile devices. As we refactored this process completely via an integrated WebView, I removed the password field completely.
  • The after_signup_update view cannot be skipped anymore.
  • I remove some of the duplicate flash messages and JavaScript alerts, which were shown when the user clicked on the submit button or reached the front page.

What needs to be done?

  • Fix tests.

Note:

  • I thought a while about making after_signup_update "beautiful again" and I decided to don't touch it. The problem is that we do not have any kind of Style Guide which can be applied here or was applied to every page on the platform. Adding once again another form style will lead to more and more CSS lines and a quite bloated Stylesheet.

@lennerd
Copy link
Collaborator Author

lennerd commented Aug 15, 2016

I realized, that it was a total stupid idea to remove password completely as we have admin users who need a password to login. I will revert that part of the PR.

@holgerd
Copy link
Member

holgerd commented Aug 16, 2016

@lennerd I tried to test this on staging but get errors:

  • Using Firefox Mac I click "login" and "login with OSM" and login to OSM using holger@mailinator.com. I get back to the add email form on wheelmap and type in holger@mailinator.com. I get a notification that I'll get a confirmation email and click "OK"
  • I expect the then come to wheelmap homepage and be logged in
  • Instead, I see a "502 bad gateway" error from cloudflare (see screenshot). When I reload the page https://staging.wheelmap.org/profile/after_signup_update (which is in the browser bar) I get "The page you were looking for doesn't exist."

staging_wheelmap_org___502__bad_gateway

@lennerd
Copy link
Collaborator Author

lennerd commented Aug 16, 2016

@holgerd This not on deployed on Staging so maybe you discovered another error.

@holgerd
Copy link
Member

holgerd commented Aug 16, 2016

ok, let me know when I can test something on staging.

Lennart Hildebrandt notifications@github.com schrieb am Di., 16. Aug.
2016 um 11:42 Uhr:

This not on deployed on Staging so maybe you discovered another error.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#359 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AANVilIZrkmd5lcGLhaHea8du6aOCILgks5qgYYfgaJpZM4JkW5v
.

@holgerd
Copy link
Member

holgerd commented Aug 17, 2016

I described the bug in separate issue #361

@lennerd
Copy link
Collaborator Author

lennerd commented Aug 29, 2016

👍 Thanks!

@lennerd
Copy link
Collaborator Author

lennerd commented Aug 29, 2016

FYI: Holger and I discussed the flow again and agreed on just fixing the current "Almost there" view without making the users email address mandatory.

@lennerd
Copy link
Collaborator Author

lennerd commented Aug 29, 2016

  • Update new translations.

@lennerd
Copy link
Collaborator Author

lennerd commented Aug 29, 2016

@1000miles What do you think about deploying this on Staging? Does it make sense to deploy this now? Or should we wait for the new server setup.

@Xylakant
Copy link
Collaborator

go ahead and deploy on staging.

@Xylakant
Copy link
Collaborator

there's no reason not to continue development as normal, we're building fully parallel

@lennerd
Copy link
Collaborator Author

lennerd commented Aug 29, 2016

Didn't want to break something. Thanks for the information.

@lennerd
Copy link
Collaborator Author

lennerd commented Aug 29, 2016

@holgerd @Svenyo This is deployed on staging. Looking forward to your feedback.

@Svenyo I removed a source translation string and would like to push this to Transifex to update all other translation files. Okay?

@lennerd
Copy link
Collaborator Author

lennerd commented Sep 5, 2016

@holgerd @Svenyo Please review.

@holgerd
Copy link
Member

holgerd commented Sep 5, 2016

This is still blocked by #361

@holgerd holgerd self-assigned this Oct 25, 2016
@1000miles
Copy link
Collaborator

This may need to be deployed on staging again for @holgerd to review.

@holgerd
Copy link
Member

holgerd commented Jan 24, 2017

@1000miles can you look into this and decide whether this can be resolved?

@ghost ghost added this to the Start February 7 milestone Feb 7, 2017
@ghost ghost modified the milestones: Start February 14, Start Feb 21 Feb 22, 2017
@holgerd
Copy link
Member

holgerd commented May 15, 2017

@1000miles what is the status of this?

@holgerd holgerd modified the milestone: Start Feb 21 May 15, 2017
@1000miles
Copy link
Collaborator

@holgerd The sign-up/sign-in part was put on hold until the front-end part got a make-over by @opyh ? This is why this PR (#359) and the related issues (#175, #179) were assigned to you, or?

@holgerd
Copy link
Member

holgerd commented May 15, 2017

@1000miles thanks. so this is on hold as well. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On after signup edit page: form validation not working properly
4 participants