DMPRoadmap / roadmap

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

is being logged in really an error #751

Closed marisastrong closed 6 years ago

marisastrong commented 6 years ago

I invited someone to a plan (actually myself, but just my gmail account) and clicked on the link to accept the invitation.

I was directed to the application but it said Error you are already logged in.
Is this really an error?

Here's the email (separate issue but could use some formatting)

Hello mxs4ever@gmail.com,

A colleague has invited you to contribute to their Data Management Plan at DMPRoadmap - STAGE.

Click here to accept the invitation (or copy https://roadmap-stg.cdlib.org/users/invitation/accept?invitation_token=6LfYmtPwZ-WUy3Sh2MQF into your browser).

If you don't want to accept the invitation, please ignore this email. Your account won't be created until you access the link above and set your password.

All the best, The DMPRoadmap - STAGE team.

screen shot 2017-10-19 at 12 06 58 pm

marisastrong commented 6 years ago

Do we have other categories of messages other than error - Alert, Warning, etc

briri commented 6 years ago

We're using the standard Rails flash messages (alert and notice). I agree that isn't really an error (or at least not a negative thing anyway). We will have to dig into the code though to see if this message is being thrown by one of our controllers or within the devise gem that we're using to handle authentication.

sjDCC commented 6 years ago

I suspect this is because your browser was already logged in as marisa.strong@ucop when you followed the invite token issued to mxs4...

I'd noticed an error the other day which displayed in the standard yellow alert. Are we not colour coding them or is that to come?

briri commented 6 years ago

noted the desire to set the notice/alert colors to mimic those in the current DMPonline site in #746

jollopre commented 6 years ago

@sjDCC was right, this message comes up due to the user being signed in when a link for a new user creation according to the token is passed.

I have override the flash alert with a more sensible message "Cannot validate invitation token unless you sign out". We can certainly change it to a flash notice instead.

Please, let me know if you come up with a better message to be displayed.

sjDCC commented 6 years ago

That message sounds fine to me @jollopre. I think it's going to be a rare case anyhow, when the likes of us are testing sharing with multiple email addresses.

marisastrong commented 6 years ago

Actually, that sounds more confusing to me as a user - I have no idea about an invitation token validation. Just to clarify, the issue was raised because it's not clear to me, as a tester, if the issue is actually an error or a notification/alert. And as a user, I'm not quite sure what I'm do to.

So perhaps, the message could be, "Notice: You are already logged in as another user. Please log out to activate your invitation"

But this also might be an edge case since i'm logged in and logging in as a different instance of myself. That said, perhaps just changing "Error" to "Notice" would suffice here.

sjDCC commented 6 years ago

Yeah that reworked text sounds good @mxsstrong It give the user a clear action to follow and explains why the error / notice is being thrown. I tried the workflow the other day and if you logout then re-click the invitation token it works fine second time round.

jollopre commented 6 years ago

Thanks for the comments. Re-reading the question raised by @mxsstrong on the first comment: "Is this really an error? Yes, according to devise, i.e. there is a filter that checks if an user is signed in before rendering the view that allows you to register given an invitation....

For devise, an error message is described as a flash alert (yellow colour for us) and I think aligns well with the problem encountered. Regarding the message proposed by @mxsstrong, it seems good to me.

Changing the colour to blue is debatable, I personally think is best as it is but let's decide that as a team.

stephaniesimms commented 6 years ago

@jollopre - when I tested the same steps marisa reported above, the link in the email to my GMAIL account directed me to my current logged-in session for my UCOP account. it looks like the invitation token is missing from the link @jollopre (see screenshot below).

the desired behavior is to provide an error/notice message.

screen shot 2017-11-13 at 12 07 18 pm

jollopre commented 6 years ago

@stephaniesimms, the error message works as expected, see screenshot below: untitled

However, the email sent is not getting the adequate URL for the invitation. It seems to me a deployment problem... The only thing I can think right now is not having the accept_user_invitation_url helper configured. @briri, do you have https://github.com/DMPRoadmap/roadmap/blob/CDL-MVP/config/routes.rb#L38 on your config/routes.rb?

briri commented 6 years ago

@jollopre yes, the line on roadmap-stg matches what is in CDL-MVP

stephaniesimms commented 6 years ago

I tested again @jollopre and i'm still not getting the correct URL for the invitation (screenshot below). the link in the email to my GMAIL account directed me to my current logged-in session for my UCOP account. no error message.

screen shot 2017-11-30 at 6 46 41 am

briri commented 6 years ago

I think there is some confusion here. We have 2 possible scenarios:

  1. The current user shares their plan with a user who does not have an account in Roadmap. This sends an invitation to create an account (see @mxsstrong original email above)
  2. The current user shares the plan with a user who does have an account. This sends a share notification with a direct link to the plan (see the other emails above).

Scenario 2 simply brings you to the plan page if you're logged in or the login page if you're not. Scenario 1 will try to bring you to the invitation page but if you're already logged in it will correctly realize that you have an account and bypass the process.

We could perhaps hack the Devise code to force a logout when reaching the invitation page but I don't think its necessary. The testing we've been doing here isn't something users are likely to encounter normally (inviting themselves and then immediately clicking the invite before logging out).

I vote that we close this one

stephaniesimms commented 6 years ago

i agree that the original bug scenario is an edge case. the only thing that seems to still be an issue in my mind is that the URL in the email invitation does not include an auth token. should it @briri ?

briri commented 6 years ago

No, it shouldn't contain a token in that scenario.

jollopre commented 6 years ago

The issue addresses the type of message that a user receives when: "tries to register, given an invitation token URL but there is another user signed in the tool". @stephaniesimms, if the email you are receiving (a new invitation to join the tool) does not contain that invitation token, it must be something regarding your deploy since on our development environment that works.