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

Improved email phone in form #3191

Merged
merged 6 commits into from Sep 30, 2020
Merged

Improved email phone in form #3191

merged 6 commits into from Sep 30, 2020

Conversation

dracos
Copy link
Member

@dracos dracos commented Sep 23, 2020

This does three things:

  1. only shows username errors on the form where they were triggered (ie. sign in form or 'by email' form) which should fix https://github.com/mysociety/fixmystreet-commercial/issues/1993
  2. If SMS_AUTHENTICATION is set, changes the new report form so that it shows email/phone, asks you to pick between them and uses your choice as the 'username' for account purposes, which should fix https://github.com/mysociety/fixmystreet-commercial/issues/1996.

image

And then one I am not sure about:
3. It splits the dual-use username field into username (signing in form) and username_register (confirm by code). I think this might help with obscure errors where e.g. if new report form submission to server has a problem, it would display the username in both fields client-side, which would then cause an issue on next submission if you only altered the one you could see, not realising there was another. I did not rename username to username_sign_in (I did get halfway through, sigh) because it starts to interact with the auth code e.g. with 2FA and change password etc and I didn't want to affect them (I mean, hopefully I haven't!).

@dracos dracos requested a review from struan September 23, 2020 15:05
@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

Merging #3191 into master will increase coverage by 0.03%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3191      +/-   ##
==========================================
+ Coverage   83.66%   83.69%   +0.03%     
==========================================
  Files         251      251              
  Lines       15744    15778      +34     
  Branches     2942     2952      +10     
==========================================
+ Hits        13172    13206      +34     
  Misses       1639     1639              
  Partials      933      933              
Impacted Files Coverage Δ
perllib/FixMyStreet/App/Controller/Report/New.pm 90.93% <92.59%> (+0.24%) ⬆️
...erllib/FixMyStreet/App/Controller/Report/Update.pm 84.27% <100.00%> (+0.88%) ⬆️
perllib/FixMyStreet/DB/Result/User.pm 96.34% <0.00%> (-0.61%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49a2bf5...2abe443. Read the comment docs.

@dracos dracos requested review from struan and removed request for struan September 24, 2020 13:58
Copy link
Member

@struan struan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not fully reviewed this but there's at least one bug so I might as well point that out first.

Copy link
Member

@struan struan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few design things, some JS issues, and non JS validation issues

If a staff user is reporting as another user, and gives an invalid
email address, for example.
Rename the not-logging-in username field to username_register.
Keep the sign-in field as username because that e.g. overlaps
with auth code in two-factor authentication.
Copy link
Member

@struan struan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, one final thing, there's not a template set for sms in redirect_or_confirm_creation so you get an error page as it tries to display the report/new template and doesn't have all the information.

Copy link
Member

@struan struan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error problem with SMS failing is fixed in fe39d46 so the rest of this looks ok.

@dracos dracos merged commit 3a511f5 into master Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants