Bug #3593

[Workbench] Workbench can't render exceptions

Added by Brett Smith about 5 years ago. Updated about 5 years ago.

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

100%

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

Description

render_exception was counting on being able to render user information from the session cache when the API server was unreachable. We recently excised that cache, which is causing render_exception to raise exceptions in most cases. We want to go back to setting the session cache, but only using it for render_exception (and not load_api_token).


Subtasks

Task #3595: Review 3593-workbench-exception-renderingResolvedBrett Smith

Associated revisions

Revision 8864cde0
Added by Brett Smith about 5 years ago

Merge branch '3593-workbench-exception-rendering'

Closes #3593, #3595.

Revision 301abeab (diff)
Added by Brett Smith about 5 years ago

3593: Add explanatory comment about Workbench's user cache.

Refs #3593.

History

#1 Updated by Brett Smith about 5 years ago

  • Description updated (diff)

#2 Updated by Radhika Chippada about 5 years ago

Review feedback:

The issue itself is resolved and also the profile handling works (the original issue where we removed the session cache for user).

However, I have one question.

The issue could have been resolved without resurrecting user session caching and making a change similar to the following (around line 82 of application_controller.rb:

What we have now:

      begin
        build_project_trees
      rescue ArvadosApiClient::ApiError 
      . . . 

Potentially updated as below (either rescue from all errors or add more to the list, especially the Errno::ECONNREFUSED)

      begin
        build_project_trees
      rescue 
      . . . 

I am wondering if updating the rescue statement (either by removing constraints or adding more) is a better solution than resurrecting the session cache.

Thanks.

#3 Updated by Brett Smith about 5 years ago

Radhika Chippada wrote:

The issue could have been resolved without resurrecting user session caching and making a change similar to the following (around line 82 of application_controller.rb:

When we did the error reporting story (#2891), it included this line in the story spec: "Error page should not give the appearance of being logged out if in fact the user is still logged in." Doing this requires storing basic user information in the session. Tom and I discussed this before I wrote the branch, and we came to an agreement that resurrecting the cache, and using it in this more limited fashion, was the best approach to preserve the original story's intent while avoiding the kinds of caching troubles that recently led to its removal.

Let me know if you have any other questions.

#4 Updated by Brett Smith about 5 years ago

  • Status changed from New to In Progress

#5 Updated by Radhika Chippada about 5 years ago

  • Status changed from In Progress to New

Brett, thanks for the clarification. I see the issue now.

That settled, LGTM to me. Thanks.

#6 Updated by Brett Smith about 5 years ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:8864cde017b9214b2e5383397b182f0b5f60334c.

#7 Updated by Radhika Chippada about 5 years ago

Brett, would you please add a comment at application_controller -> setup_user_session method line 439 with an explanation that session[:user] is being set only for exception handling and is not used for regular user interactions? If you could throw in this bug #, it would be great. Thanks.

Also available in: Atom PDF