DMPRoadmap / roadmap

DCC/UC3 collaboration for a data management planning tool
MIT License
102 stars 109 forks source link

Missing method User.update_without_password #397

Closed briri closed 7 years ago

briri commented 7 years ago

Scenario:

Error: screen shot 2017-06-05 at 12 58 34 pm

It looks like we can just call the normal update method. The user will have supplied a password in the registration form. If not it will return an error as expected

vyruss commented 7 years ago

I was sure I fixed this exact issue scratches head - I'll take another look at the logic

vyruss commented 7 years ago

@briri I cannot replicate this on dmponline-test - I believed this was fixed as #322

sjDCC commented 7 years ago

This works fine on DMPonline-test. Have just redone the process to check.

I could register the new account but don't have access to the plan shared previously as that stem account and token are now deleted.

Will leave open for now for @stephaniesimms to check since the issue was on staging

stephaniesimms commented 7 years ago

Testing on Roadmap stage produces the following outcomes, neither of which is desirable:

Scenario 1:

Scenario 2:

screen shot 2017-06-07 at 9 10 51 am

screen shot 2017-06-07 at 9 14 37 am

vyruss commented 7 years ago

Scenario 1 cannot be avoided with current implementation of devise_invitable for invitations - the plan sharing and account creation depends entirely on the token in the invitation email. Because we were getting many requests from users who hadn't got the email but were blocked from registering we decided it would be better to delete the stem account and let them register (even if it means the plan needs to be re-shared)

Scenario 2 cannot be replicated on dmponline-test

sjDCC commented 7 years ago

This is odd. I've followed the invite link to register a few times over past few days while testing and it's always created account and logged me straight in. Is there something different in your token setup @briri? I can't understand why you're getting an invalid token error.

Happy to rethink the workflow on this, but don't want to delay our testing for it now. If we rework the existing scenario 1, can we do it as part of the overall MVP please?

vyruss commented 7 years ago

All, make sure you click on the invitation link from a Chrome Incognito window (Command/Ctrl+Shift+N) or a Firefox Private window (Command/Ctrl+Shift+P) so that you have no cookies from any other Roadmap session.

stephaniesimms commented 7 years ago

@briri and I were playing w/this more earlier. I'll retest and confirm later today. we're going to create a new issue to patch devise to solve scenario 1 (for larger MVP). scenario 2 probably just needs a revised error message since it looks like it relates to stem account/data problems and is most likely an edge case. stay tuned.

stephaniesimms commented 7 years ago

I retested with new accounts and all works as expected now. I created a new issue #407 to address Scenario 1 for CDL MVP. For Scenario 2 outlined above, whatever the cause for the error messages I think we should just change how we communicate the error to users:

I suggest we revise "The invitation token provided is not valid!" to "The system encountered an error. Please visit [app URL] to create an account." what do you think @sjDCC ?

sjDCC commented 7 years ago

Sure, that sounds good. Thanks @stephaniesimms

vyruss commented 7 years ago

I can't see a use case where normal users would come across this message - except if they copy&paste/type the invitation link by hand and get the last part wrong (in which case the new message would be less informative), or if they click on the invitation link and are already logged into the system via another account, perhaps it's not worth the effort coding around this as it is a default devise_invitable message

sjDCC commented 7 years ago

@stephaniesimms can you explain how you got the error messages listed in scenario 2. I've not been able to replicate that so am trying to understand what's going wrong to consider what messaging would be best.

The only way I can imagine these being invoked is if a person without an account has had a plan shared with them, creates an account without following the invitation link, and then subsequently follows the invitation link. That's the only way the token would be invalid as it would have been deleted when they created an account as normal in the interim. Or maybe it's a copy/paste error in the token as @vyruss suggests?

If this is the case, I think the original message (the invitation token provided is not valid) works best. We wouldn't want to redirect them to create an account as they must have already done this. It's more of a user error. Surely it would be quite rare that someone misses the invite and then comes back to it later, having created an account in the interim.

stephaniesimms commented 7 years ago

@sjDCC and @vyruss I'm fine w/leaving as is since I can't replicate the error and am not 100% sure how I produced it in the first place. a user would not understand the message about an invalid token, which is why i suggested changing it, but if none of us thinks it likely that anyone encounters it in the first place it's safe to ignore for now. will close this issue.