User Registration Overhaul: break down

[emcoding]

Continue the discussion that started in #989.
My proposal would be to first clean up the most obvious things, start building feature tests that cover all the cases/user states and user roles. Then identify if there are underlying problems left.

1. We need to give unconfirmed users access to update their email address, while denying access to restricted areas. That should be solved with point 2) and 3).

2. Bug: Unconfirmed users having access to restricted pages.

• TODO Quick fix /temporary solution to deny access for unconfirmed users. I 'll pushed a PR today, so we can build Allow infinite confirmation period #989 onto that. See PR Rearrange, not change the abilities in Ability #997

3. Users should be able to update their email address, even those who did not receive a confirmation email.

• Solved in Devise overhaul #1007
• TODO make resending the confirmation email accessible for unconfirmed users
• Make sure that the 'resend confirmation' messages + logic are in place.

4. Identify and solve omissions in the abilities.

• TODO UsersController : see PR Spring cleaning users #1006

• TODO TeamsController : same as ^

• TODO MailingsController

• TODO CommentsController
  It looks like there is something missing in the authorising part of the polymorphic relations. Maybe best to see if the fixing the UsersController works before we tackle the comments.

• (Please add other issues re: authorisation here)

5. Write feature tests

• TODO: add feature tests for all the user paths (including the correct messages and sending the correct confirmation mails)

6. Separate the authentication from the authorisation.

• TODO Use authenticate_user! (by Devise) instead of ability
• TODO Update abilities, removing the authentication parts.

7. Apart from all the Ability stuff, there is an underlying thingy with Devise and GH sign in and users created in the Teams form. Maybe things will be easier to maintain if we go Devise all the way. OTOH, it could be that a little cleaning up, polishing and dusting off is all that is needed. Hard to tell at this point.

8. @klappradla 's suggestion: Ensure that the appropriate mails are being sent.

• We are sending confirmation mails on different occasions.
  For example: When a user is created, and when a user updates their own email.
• TODO Double check if User#update resend_confirmation only when a user's has changed their email address, not for other updates.
• TODO Change the email confirmation mailing content to reflect that the email address has changed, and that the change needs to be confirmed. new issue Add urgency to the email confirmation message #1004
• TODO Update the profile edit form and show view. The old email address is displayed until the new email address is confirmed. The user isn't noticed about this, and that is confusing. It looks like the change has failed. (And if they try again, a confirmation mail is sent again. And again. )
• It seems not all mailers' sending happens in the background. Is the method send_devise_notification ever triggered at all? Needs investigation.

9. @pgaspar suggestion: Change the flow for unconfirmed users without email address,

maybe we should redirect to a very simple form explaining you need to confirm your email, which would also allow them to edit the email. This would be a simpler single-field form, not the standard edit form.

Looks like a great plan. I'd prefer not to start with the latter, do the clean up first.

Is this a plan? Are steps missing?

[emcoding]

@pgaspar @klappradla @ramonh @carpodaster Your opinions please 🙇

[hola-soy-milk]

I think this is a good plan. Thank you @F3PiX.

I'll begin by having a look at #992 👍

[klappradla]

Looks and sounds great 👍

Is the logic for resending the confirmation email in place and working? If yes, triggering it should be accessible for unconfirmed users as well of course - how else should they be able to confirm their account in case the lost or did not receive the initial email?

Devise normally offers all the logic out of the box and the routes are already there:

               new_user_confirmation GET      /users/confirmation/new(.:format)                               devise/confirmations#new
                   user_confirmation GET      /users/confirmation(.:format)                                   devise/confirmations#show
                                     POST     /users/confirmation(.:format)                                   devise/confirmations#create
resend_confirmation_instruction_user POST     /users/:id/resend_confirmation_instruction(.:format)            users#resend_confirmation_instruction
                        confirm_role PUT      /roles/:id/confirm(.:format)                                    roles#confirm

I'll add double checking this to the list of Todos above.

[emcoding]

Thanks! Updated description.

[pgaspar]

I also like this plan 🙌 Agree with cleaning up first, improving UX afterwards.

[emcoding]

Follow up issues and questions. Noted here so we don't forget to address them at due time

• In the console, I assigned my team_less account a student_role. I added a conf. Is that even possible IRL? When hitting Conferences#create, it redirects to -path. => UrlGenerationError. Conference not created. Create issue or irrelevant?
• A student can destroy their own team, which make sense. But what about an accepted team, mid-season?
• Note: Some 'ability-ish' feature are managed by methods in application_helper.rb (:can_see_private_info, :can_only_review_private-info.).
• Investigate the relation between a role mailer and an account confirmation mailer. Especially for coaches and students.
• Move messages to devise yml file
• Investigate Devise messages, to see if we can show different messages for confirmation and reconfirmation; maybe other messages as well if needed
• One important Devise issue left: we are sending confirmation mails over and over again to unconfirmed users (in users#update) when they change any profile field. There are several options that we can discuss after this PR is agreed upon.
• After landing on the Edit Profile page, a user can still navigate away without submitting the form.
• The Welcome message and the hint under the email field is party duplicated. Needs finetuning later. And the hint needs design love. Maybe highlighting?
• The email hint should only show when the user changed the value in the email field (JS :on_change ?)
• A confirmed user who changed their email address should have the yellow confirm-your-email notice after submitting the form. Currently, they don't see that. (Their status is still confirmed)
• The country + city fields are required, but not validated as a combination. So, Amsterdam on Antartica is valid input. (TBH I think the UX on first sign in would be better if only the email address would be required.)

[hola-soy-milk]

Thanks for the further breakdown!

I think it should not be possible to delete your selected team mid-season. Sounds like we need to add a validation, right?

[hmesander]

Hi there! I’m a first-time contributor and was hoping to help out with this issue. Specifically, I could tackle one of these components to start:

• After landing on the Edit Profile page, a user can still navigate away without submitting the form.

-A confirmed user who changed their email address should have the yellow confirm-your-email notice after submitting the form. Currently, they don't see that. (Their status is still confirmed)

Would it be okay if I work on this?

[emcoding]

Hi @hmesander! Welcome to the Teams App!!
This issue is an overview for a bigger project. Actionable issues are or will be added separately. For a first contribution, I'd recommend to find an issue with the beginner friendly label.
Looking forward to your contribution!

[klappradla]

Not sure how up to date this is, but FYI: I just started looking into feature testing (and therefore probably also refactoring the registration). Just in case someone else is already on the verge of submitting a PR for this 😉