Arvados

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.

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.

- 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.

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.

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.

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}" do
        …
      end
      test "check help for user #{token}" do
        …
      end
      …
    end
  
    [
      ['active', api_fixture('users')['active']],
      ['admin', api_fixture('users')['admin']],
    ].each do |token, user|
      test "test system menu for user #{token}" do
        …
      end
      test "test manage account for user #{token}" do
        …
      end
      …
    end
  

• 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.

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

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!

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.