Arvados

Tom, with the updates you put in for the profile implementation, a couple redundant items to be cleaned up.

- Since we are no longer using the div.rounded I added to application.css.scss, would you please remove this.

- Can you please update the workbench application.default.yml to no longer say "If this [message] is not provided, a default message will be displayed." about message.

Thanks.

Reviewing c4c188c

• When I visit Workbench using the inactive_but_signed_user_agreement token, I get prompted to fill out my profile, and then I can use Workbench. Is this expected behavior? I would've expected to get the "You're not active" page.
• I don't think this was introduced by any of your changes, but since I tested it: if I have an active session, then I visit Workbench with an expired API token parameter, it falls back to my active session. This seems a little surprising, but maybe it's expected behavior? Maybe the best thing to do is to add an item to the backlog to explore it.
• You can remove the style for div.rounded from application.css.scss, since you've removed all its users.
• Please remove the now-unneeded ['/collections'].each do |path| loop in CollectionsTest.
• In assert page.has_text? profile_message.gsub(/<.*?>/,'')[0,25], I think you can drop the slice. I believe that was the quick way to avoid trouble from tags.
• For what it's worth, I think it'd be worthwhile to give the API Group model a class method that returns the "All users" group. Right now we have the finder code written in at least five different places (git grep -Fe '-f+$'). But it's okay if you want to punt this.

Thanks.

Radhika Chippada wrote:

remove div.rounded

Done.

Can you please update the workbench application.default.yml to no longer say "If this [message] is not provided, a default message will be displayed." about message.

Done.

Brett Smith wrote:

• When I visit Workbench using the inactive_but_signed_user_agreement token, I get prompted to fill out my profile, and then I can use Workbench. Is this expected behavior? I would've expected to get the "You're not active" page.

Yes, that's expected: inactive_but_signed_user_agreement satisfies the criteria for activation.

• member of All Users group ("inactive_user_member_of_all_users_group" link) -- i.e., "setup" has happened
• user agreement signature ("user_agreement_signed_by_inactive" link)

• I don't think this was introduced by any of your changes, but since I tested it: if I have an active session, then I visit Workbench with an expired API token parameter, it falls back to my active session. This seems a little surprising, but maybe it's expected behavior? Maybe the best thing to do is to add an item to the backlog to explore it.

Hm, indeed. If I'm following you correctly, the bug is

1. Be logged in.
2. Visit with ?api_token=123abc (or in fact a valid token).
3. The new token is ignored. You're still logged in as if you had never provided an api_token.
4. As a bonus, the ?api_token=123abc is displayed in the location bar instead of being redirected away.

It seems like it would be preferable, if a token is explicitly provided this way, to throw away the existing token (if any) and use the new one, come what may. Sound right?

(And yes, I think this should be a separate issue.)

• You can remove the style for div.rounded from application.css.scss, since you've removed all its users.

Done.

• Please remove the now-unneeded ['/collections'].each do |path| loop in CollectionsTest.

Done.

• In assert page.has_text? profile_message.gsub(/<.*?>/,'')[0,25], I think you can drop the slice. I believe that was the quick way to avoid trouble from tags.

Done.

• For what it's worth, I think it'd be worthwhile to give the API Group model a class method that returns the "All users" group. Right now we have the finder code written in at least five different places (git grep -Fe '-f+$'). But it's okay if you want to punt this.

Good idea, I think we should punt and give system_user, system_group, et al. similar treatment. Started #3622

Now at f6634dd

Tom Clegg wrote:

• I don't think this was introduced by any of your changes, but since I tested it: if I have an active session, then I visit Workbench with an expired API token parameter, it falls back to my active session. This seems a little surprising, but maybe it's expected behavior? Maybe the best thing to do is to add an item to the backlog to explore it.

Hm, indeed. If I'm following you correctly, the bug is

1. Be logged in.
2. Visit with ?api_token=123abc (or in fact a valid token).
3. The new token is ignored. You're still logged in as if you had never provided an api_token.
4. As a bonus, the ?api_token=123abc is displayed in the location bar instead of being redirected away.

It seems like it would be preferable, if a token is explicitly provided this way, to throw away the existing token (if any) and use the new one, come what may. Sound right?

(And yes, I think this should be a separate issue.)

You follow the bug correctly, and we're on the same page about the expected behavior. A new story sounds good.

I think f6634dd is good to merge. Thanks.