Bug #3604

[Workbench] theming support for login and inactive user page is broken (maybe elsewhere too)

Added by Ward Vandewege about 5 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench
Target version:
Start date:
08/18/2014
Due date:
% Done:

100%

Estimated time:
(Total: 1.00 h)
Story points:
0.5

Subtasks

Task #3615: Review 3604-theme-and-new-user-processResolvedTom Clegg


Related issues

Related to Arvados - Bug #3622: [API] [refactor] Move system_group, system_user, etc to class methods in Group and UserNew

Associated revisions

Revision aa2adbca
Added by Tom Clegg about 5 years ago

Merge branch '3604-theme-and-new-user-process' closes #3604

History

#1 Updated by Ward Vandewege about 5 years ago

  • Story points set to 0.5

#2 Updated by Tom Clegg about 5 years ago

  • Status changed from New to In Progress

#3 Updated by Tom Clegg about 5 years ago

  • Category set to Workbench
  • Assigned To set to Tom Clegg

Branch 3604-theme-and-new-user-process at c4c188c

  • Fix theme by redirecting to /users/welcome and /user_agreements instead of rendering them from other actions

Along with a bunch of other fixes encountered while testing:

  • Hide the red "2 notifications" indicator for inactive users (the notifications don't show up when you dropdown and you can't do anything about them anyway)
  • Remove secret default profile error message from /users/profile view (there's already a default in application.default.yml)
  • Clean up redirect logic in profile page
  • Clean up styles on profile page (use bootstrap alert-info/alert-success styles instead of making up new stuff)
  • Remove filter exception for non-existent method update_profile
  • (My favorite) Fix permission cache bug that caused users to auto-activate after being unsetup by admin. (This was an ill-advised use of "delete" instead of "destroy" in unsetup: "delete" circumvents the Link filters that take care of cache consistency.)

#4 Updated by Radhika Chippada about 5 years ago

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.

#5 Updated by Brett Smith about 5 years ago

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.

#6 Updated by Tom Clegg about 5 years ago

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

#7 Updated by Brett Smith about 5 years ago

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.

#8 Updated by Anonymous about 5 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:aa2adbcaa06c4f5dc7a6e54f3d9a5148b20fbbb1.

Also available in: Atom PDF