Skip to content

Fix some nil session data errors (LG-4810)#5215

Merged
zachmargolis merged 4 commits intomainfrom
margolis-nil-session-data-errors
Jul 15, 2021
Merged

Fix some nil session data errors (LG-4810)#5215
zachmargolis merged 4 commits intomainfrom
margolis-nil-session-data-errors

Conversation

@zachmargolis
Copy link
Contributor

No description provided.

def download
data = user_session[:personal_key] + "\r\n"
send_data data, filename: 'personal_key.txt'
if user_session[:personal_key].present?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although unlikely in the real-world, the new test case seems to illustrate the root cause here. Maybe worth filing separately, but I'd wonder if we could change the logic to not rely on #show having previously been visited, i.e. call something similar to finish_idv_session as a before_action, or set to user_session from ReviewController#create as part of the profile creation, or change this to call personal_key, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I filed https://cm-jira.usa.gov/browse/LG-4847 to follow up on that.

Part of my thinking is, there shouldn't be a code to download if the user hasn't viewed it first, so the ordering does make some sense to me. We can noodle on it and follow up

data = user_session[:personal_key] + "\r\n"
send_data data, filename: 'personal_key.txt'
else
Rails.logger.warn('no personal_key in user_session')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make an analytics event for the pkey download? That way we'd get all the goodies that come along with that, like session and user IDs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will update that and then do success: true/false instead of separate events

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated in 8ba4ebf

@zachmargolis zachmargolis merged commit 65663cd into main Jul 15, 2021
@zachmargolis zachmargolis deleted the margolis-nil-session-data-errors branch July 15, 2021 17:17
mitchellhenke pushed a commit that referenced this pull request Jul 22, 2021
* Fix NoMethodError + on nil in MfaConfirmationController
* Fix NoMethodError + on nil in IdvConfirmationsController
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants