Story #3296

[Workbench] Configuration option requires new users to complete a survey/form before using Workbench

Added by Tom Clegg over 5 years ago. Updated about 5 years ago.

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

100%

Estimated time:
(Total: 8.00 h)
Story points:
2.0

Description

Form fields we want to collect:
  • Institution/Company (required)
  • Lab
  • Website URL
  • Title
  • What best describes your role (dropdown):
    Bio-informatician
    Computational Biologist
    Biologist/Geneticist
    Software Developers
    IT
    Other
New behavior and implementation notes:
  • Add a link in the email address dropdown menu in Workbench called 'Edit profile'.
  • Add a new page (/users/{my_uuid}/profile) with a form allowing the user to add/edit profile data.
  • Get the list of form fields from the user_profile_form_fields configuration value.
  • Use standard bootstrap form markup (see http://getbootstrap.com/css/#forms-horizontal).
  • Store the values in user prefs: current_user.prefs["profile"]["title"], etc.
  • On the "edit profile" page, display (but not offer to edit) the user fields that get populated automatically during the OpenID login process (email, first name, last name, identity_url).
  • Add comments to application.defaults.yml showing how to add profile fields.
  • The default configuration should be an empty hash (i.e., no form fields are shown, and the link to the "edit profile" page is removed from the email address dropdown menu).
  • If any of the required profile keys indicated by the user_profile_form_fields configuration hash are missing from the current user's prefs["profile"]:
    • Redirect most1 dashboard pages to the "edit profile" page.
    • Add an alert panel to the "edit profile" page asking the user to please provide the following information.
    • Highlight the required fields so it's not too hard to find "which one did I miss?" (especially if the profile already existed but the config has added a new required key).
  • Trap to watch out for: If admin adds a new required field, next time a tab pane gets refreshed for some unsuspecting user, we probably don't want the tab pane to end up with a "manage profile" link. Perhaps we should skip the "redirect if any missing" logic for requests with params[:partials]? (And methods other than GET)

1 Avoid constructing a catch-22 where (for example) the user agreement cannot be signed until the profile is edited, and vice versa.

Structure and example yaml encoding for user_profile_form_fields:

  user_profile_form_fields:
    -
      key: organization
      type: text
      form_field_title: Institution/Company
      form_field_description: ~
      required: true
    -
      key: role
      type: select
      form_field_title: Your role
      form_field_description: Choose the category that best describes your role in your lab.
      options:
        - Bio-informatician
        - Computational biologist
        - Biologist or geneticist
        - Software developer
        - IT
        - Other

Example encoding for user.prefs[:profile]:

{"organization":"Laidlaw Lab","role":"Software developer"}

Subtasks

Task #3155: redirect regular workbench pages to the "edit profile" page if any required fields (as decided by application.(defaults.)yml) are not filled in.Resolved

Task #3154: Provide an "edit profile" page for editing the current_user.prefs[:profile] hashResolved

Task #3156: [API] Send e-mail to admin the first time a user's profile data is savedResolved

Task #3474: Review branch 3296-user-profileResolvedRadhika Chippada

Task #3477: When an admin does "login as" a different user, skip profile check.Resolved

Task #3512: Add a "message" field to configResolvedRadhika Chippada

Task #3560: Redirect user back to the profile page after the user fills in all required fields. Include a message saying profile is all set and the user can now access workbench.ResolvedRadhika Chippada

Associated revisions

Revision 7d598997
Added by Radhika Chippada about 5 years ago

closes #3296
Merge branch '3296-user-profile'

History

#1 Updated by Tom Clegg over 5 years ago

  • Subject changed from [Workbench] Configuration option to require new users to complete a survey/form before activation to [Workbench] Configuration option requires new users to complete a survey/form before using Workbench
  • Description updated (diff)

#2 Updated by Ward Vandewege over 5 years ago

  • Description updated (diff)
  • Category set to Workbench
  • Assigned To set to Peter Amstutz

#3 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#4 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#5 Updated by Peter Amstutz about 5 years ago

  • Assigned To deleted (Peter Amstutz)

#6 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#7 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#8 Updated by Radhika Chippada about 5 years ago

  • Status changed from New to In Progress

#9 Updated by Radhika Chippada about 5 years ago

Branch ready for review.

All items listed are addressed.

In addition, addressed one more issue not listed. This is when an admin user logs in as another user, skip profile check to allow the admin user to navigate.

To test: copy and paste the commented out "user_profile_form_fields:" block from "application.default.yml" to "application.yml", and un-comment the lines.

#10 Updated by Radhika Chippada about 5 years ago

  • Story points changed from 1.0 to 2.0

Adjusted the story point assignment to reflect the true amount of work involved in implementing this story.

#11 Updated by Tom Clegg about 5 years ago

  • Assigned To set to Radhika Chippada

#12 Updated by Brett Smith about 5 years ago

Comments from reviewing ff2e08f:

Generally speaking, I feel like the current branch does a lot of work to translate/provide indirection between Workbench and the API server (Workbench prepares profile_ parameters which go to a dedicated profile route that get filled into user.prefs), and it's not clear to me what it accomplishes. Because user.prefs is serialized as a simple hash, Workbench can prepare a JSON blob that maps the prefs' profile key to an appropriate hash, and send that to the API server directly. The API server's UsersController could have an after_update hook to check if a profile has been newly set in @object.prefs, and send the notification e-mail if so. An approach like this would reduce the amount of code that needs to be maintained, and feels more Rails-y to me. It would also be more robust because it would allow users to do less common tasks like remove fields from their profile. Is there an architectural reason I'm missing for the translation work that goes on here?

I would also like to see a couple of tests for this branch. At least one to show that a normal Workbench request for a profile-less user is prompted to edit their profile, and one to show that a profile-less Workbench user can send a POST request without interference. I know it's a lot of overhead, because you'll probably have to write profiles for most of the existing users in the fixtures, but I feel like the effort is justified given how this branch affects Workbench routing at a very core level. It would go a long way to prove this branch's safety to add a profile to the test configuration, and see that the existing tests all still pass. If it helps, I don't think the new tests need to be integration tests written with Capybara; I think functional controller tests would be sufficient to demonstate that the redirect works as intended.

As a general style point, Ruby hashes have methods each_key, each_value, and each_pair to iterate over the hash in different ways. Using these as appropriate may help improve readability over using a plain each and indexing the resulting array for keys and values.

More specific bullet point comments:

  • This branch adds two routes, one for /profile which brings up the HTML form, and another /users/update_profile to handle the AJAX form submission. The story specifies that profile management should be done at /users/UUID/profile. I think it would be cleaner to support that, with support for both GET (to render the HTML form) and POST (to handle AJAX form submissions).
  • When the redirect checks params[:skip_profile], this makes an opportunity for the user to bypass filling in their profile, by setting that parameter themselves. I think you can set this directly in the session when needed, and that would be a safer approach.
  • The new Workbench methods set a lot of instance variables to share data with the view. However, because views can access globals and helper methods, I believe they can get at a lot of this information directly, in this case by calling Rails.configuration.user_profile_form_fields and current_user. I think this approach would be preferable, because it reduces the number of things you have to remember to work on the view, and reduces the opportunities for instance variables to hold on to stale data.
  • In the profile update view:
    • As suggested above, I believe that the form should use the POST method, rather than GET. This better matches HTML semantics and helps ensure that, e.g., web-crawling bots don't try to update a user profile.
    • There are several spelling errors in the text.
    • HTML5 does not have the <font> tag. You can just use <span>.
    • Since users aren't allowed to edit OpenID information, I think it would be better to display it outside the form completely. Disabled form inputs can be hard to read (low contrast), and may lead the user to wonder if there's something they can do to be allowed to edit the fields.
    • Is there some architectural reason why this is a partial? From my reading, the partial only seems to be wrapped in the primary template. It's not clear to me what this separation gets us, but maybe I'm missing something in the bigger picture.
  • As you mentioned in e-mail, the story specifies that this redirect should not interfere with tab loads, and right now that's not the case. I believe this should be doable by checking params[:partial] in the same place where you check session[:skip_filter] and so on.

I was wondering about a couple of requirements details while I was working on this review. These might be questions for Tom:

  • Should we validate that values submitted for option fields actually correspond to an option in the configuration?
  • Would the notification e-mail be more helpful if it included information about the profile values that were set?

Thanks.

#13 Updated by Radhika Chippada about 5 years ago

  • Target version changed from 2014-08-06 Sprint to 2014-08-27 Sprint

Still working on some tweaks.

#14 Updated by Radhika Chippada about 5 years ago

- Added a “message” to profile config (per Adam’s suggestion during Sprint review)

- In the unlikely scenario where profile config is set to a string (instead of expected array), added handling to enable site functioning

- In the event where a profile is setup with only text boxes, and someone hits save with none of them filled in, check updated profile before making an unnecessary api call to save profile

- In the unlikely event that someone configures the profile config with only the message, but no input fields, not show save button. It would be redundant in that scenario.

- Added logic to redirect the user back to the profile page after saving a completed profile. Tom proposed this for a better user experience.

- Added updated profile to email notification message.

- Added partial handling (requires params[:tab_pane], not params[:partial]) . During this testing I noticed that modal popups fail due to redirect to profile. Hence, I also added params[:action_method] to address this issue.

- Addressed the other comments / feedback “mostly” from the review feedback, except the following: “should we validate the values submitted in select options”: It is not hard to validate. But do we throw an error or ignore if validation fails? Also, if a user has a previously saved profile, and the admin modifies the profile and deletes a previously configured option - is it an error condition? I felt this is complicating things way too much and felt it is simpler to assume that what the UI sent is agreeable.

- Added integration tests, to manage profile. Also added tests to address other recent changes to topnav such as manage account, help menu, system menu, and search box. Also added tests on the api server for the profile email notifier.

#15 Updated by Brett Smith about 5 years ago

  • Target version deleted (2014-08-27 Sprint)

Reviewing 9e5af4a. I found this version much easier to follow—thanks for spending so much continued time with it.

Some general comments:

  • The current branch removes the session user cache from Workbench, so that Workbench doesn't keep redirecting a user after they update their profile. I know Peter has put a fair bit of work into developing and maintaining this cache—on any page, it saves an API call for the page itself plus each tab on the page. Would it be possible to instead update the cached user object after a user updates their profile?
  • Would it maybe be cleaner to make the profile form message a separate configuration value (something like user_profile_message) outside of user_profile_form_fields? That way, you don't have to introspect form field elements to see whether they're real elements or the message, and there's no risk of having more than one message.
  • I think the code to check whether a user profile is missing a required field can be moved to a helper method. That way, you can call it from both ApplicationController and the profile view, to reduce code duplication.
  • There's trailing whitespace in apps/workbench/app/views/users/profile.html.erb and apps/workbench/test/integration/application_layout_test.rb.

In the profile form:

  • I'm guessing we probably want to render the form header message using the raw method. It's under the server administrator's control, so there's no security risk, and it seems likely that we'll want to put HTML in here—to highlight required fields, or link to a privacy policy, stuff like that.
  • After submitting a profile, I wonder if all users will understand what "my page" refers to on the return button. What do you think about making "access Arvados Workbench" a link that goes to the target page?
  • The static form controls use the col-sm-9 class for their grid width, while the dynamic ones use col-sm-8. I feel like these should be consistent, although I'm not sure which one should change.

In the Workbench tests:

  • I'm very glad to see so many tests added in this branch. I know some of these features were added previously, so thanks for taking the time to go back and tackle those. I like that all this functionality is being tested. However, I'm concerned that the new tests are "too big" in a couple of ways.
    • Every test that calls verify_homepage_with_profile tests a lot of functionality: the login page, profile functionality, the help menu, the system menu, the account page, and the search bar. It's good to test all this, but bundling it all together in one method is less than ideal. When I work on any of this functionality in the future, it's harder to run the tests that are immediately relevant to my work. If a test fails, you have to dig pretty deep in the error report to figure out exactly what went wrong. I would like to see these tests broken up so that different pieces of funcionality are checked in separate test calls.
    • Sometimes the tests make a lot of assertions about a single page or element. For example, early on, verify_homepage_with_profile checks that the login page has three different pieces of text. Searching for so much exact text can make it difficult to make non-functional changes later. For example, the test checks for the text 'The "Log in" button below will show you a Google sign-in page.' In the future, if we keep the same login functionaliy but switch to a provider besides Google, we'll have to update this test just to match our new login text. I think it would be helpful to take a pass and try to limit assertions to those that check proper functioning, and trim redundant ones.
  • In verify_homepage_with_profile, some of the has_link?/has_no_link? checks are missing the call to assert.
  • In Capybara tests, you can call click_link text to click on a link with text. Or you can call click_on text to click on a link or button with that text. This is usually easier to read and write than the pattern of find('a', text: "something").click.
  • This is admittedly very nit-picky, but I think it would be better if the test message said "Welcome to Arvados." instead of "Welcome to Curoverse." We don't want people thinking that their test installs are endorsed by the company or something like that. :)

In the API server:

  • Rails provides some magic methods that might be helpful for send_profile_created_notification. You can call prefs_changed? to find out if the value changed, and prefs_was to get the previous value, instead of navigating through the return value of changes.
  • The API User model still has the profile method. It looks like this is left over from the previous implementation and can be removed.

Thanks again.

#16 Updated by Brett Smith about 5 years ago

Brett Smith wrote:

  • The current branch removes the session user cache from Workbench, so that Workbench doesn't keep redirecting a user after they update their profile. I know Peter has put a fair bit of work into developing and maintaining this cache—on any page, it saves an API call for the page itself plus each tab on the page. Would it be possible to instead update the cached user object after a user updates their profile?

Tom mentioned during standup that he requested this change. Please disregard this comment.

#17 Updated by Brett Smith about 5 years ago

  • Target version set to 2014-08-27 Sprint

#18 Updated by Radhika Chippada about 5 years ago

Addressed, one more time, the review feedback. The only thing skipped out of these suggestions is the minor changes suggested such as changing click_link.

#19 Updated by Brett Smith about 5 years ago

Thanks for all the changes. At this point, I just have small concerns in the Workbench integration tests:

  • In order to reduce code duplication and prevent test data from getting inconsistent, I think it would be helpful to define each array of test data once, then define all the appropriate tests inside its each loop. So it would look roughly like this:
      [
        [nil, nil, false, false],
        ['inactive', api_fixture('users')['inactive'], true, false],
        …
      ].each do |token, user, invited, has_profile|
        test "visit home page when profile not configured for user #{token}" doend
        test "check help for user #{token}" doendend
    
      [
        ['active', api_fixture('users')['active']],
        ['admin', api_fixture('users')['admin']],
      ].each do |token, user|
        test "test system menu for user #{token}" doend
        test "test manage account for user #{token}" doendend
    
  • There are still cases where verify_homepage_with_profile calls has_link? or has_no_link? without @assert@ing the result. Please double-check these.
  • The end of verify_manage_account asserts that a key has been added to the page, even if the user passed in is not active and we haven't gone through the add procedure. Please move the assertion inside the big if condition.

#20 Updated by Radhika Chippada about 5 years ago

addressed these concerns. the last one about "manage account" is no longer invoked if the use is not active.

#21 Updated by Brett Smith about 5 years ago

Reviewing 5a3d5ac7

Radhika Chippada wrote:

the last one about "manage account" is no longer invoked if the use is not active.

I understand that, but I think a future reader looking at the function by itself (which is likely to happen if someone jumps to the line of a failing assertion) may be confused by the internal inconsistency. If the function is going to introspect and change its behavior based on the user's active status, it should follow that philosophy through completely, and only make this assertion under the same conditions.

With that change, I think this is ready to merge. Thanks!

#22 Updated by Radhika Chippada about 5 years ago

Thanks Brett, for that pointer. I failed to see the lapse and since the method was only used for active users, was not caught by the tests. I fixed it now.

#23 Updated by Radhika Chippada about 5 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:7d598997ce1851f37ac0ec21c47abc76d5e84277.

Also available in: Atom PDF