Skip to content

LG-7086-log-whether-account-is-in-reproofing#7279

Merged
gsa-manish merged 5 commits intomainfrom
LG-7086-log-whether-account-is-in-reproofing
Nov 7, 2022
Merged

LG-7086-log-whether-account-is-in-reproofing#7279
gsa-manish merged 5 commits intomainfrom
LG-7086-log-whether-account-is-in-reproofing

Conversation

@gsa-manish
Copy link
Contributor

🎫 Ticket

Link to the relevant ticket.

https://cm-jira.usa.gov/browse/LG-7086

🛠 Summary of changes

For the flow, we track if it's a reproof or not.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Step 1
  • Step 2
  • Step 3

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Before:
After:

🚀 Notes for Deployment

Include any special instructions for deployment.

changelog: Improvements, Logging, Log if reproofing for IRS
@gsa-manish gsa-manish force-pushed the LG-7086-log-whether-account-is-in-reproofing branch 2 times, most recently from 77b2363 to 17fef57 Compare November 3, 2022 18:59
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@gsa-manish gsa-manish force-pushed the LG-7086-log-whether-account-is-in-reproofing branch from 17fef57 to a34037e Compare November 3, 2022 19:19
@gsa-manish gsa-manish requested review from jmhooper and solipet and removed request for jmhooper November 3, 2022 19:45
step: current_step,
step_count: current_flow_step_counts[current_step_name],
analytics_id: @analytics_id,
irs_reproofing: current_user&.decorate&.reproof_for_irs?(
Copy link
Contributor

@aduth aduth Nov 4, 2022

Choose a reason for hiding this comment

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

current_user might not work for the hybrid document capture path (sending a link to a phone), since there's not a true user session in those steps. In other places we'd use effective_user to get the current user or the user associated with the hybrid session. Should we do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will there always be an effective user? Or should we do something like current_user || effective_user?

Copy link
Contributor

Choose a reason for hiding this comment

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

effective_user will be the same as current_user if current_user exists.
If current_user doesn't exist, it will be the user associated with a hybrid user if one exists.
If neither of those exist, it will be nil.

You shouldn't have to do current_user || effective_user since effective_user is already handling current_user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Let me update it, that sounds like a good approach.

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

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

One comment, but otherwise LGTM

analytics_id: @analytics_id,
irs_reproofing: effective_user&.decorate&.reproof_for_irs?(
service_provider: current_sp,
).present?,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is .present? necessary? Would reproof_from_irs? ever return anything other than true or false?

Suggested change
).present?,
),

Copy link
Contributor Author

@gsa-manish gsa-manish Nov 7, 2022

Choose a reason for hiding this comment

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

It can return nil also.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, reproof_for_irs? wouldn't, but I see now that with the safe navigation it might be possible that effective_user could.

@gsa-manish gsa-manish merged commit 7ff4bea into main Nov 7, 2022
@gsa-manish gsa-manish deleted the LG-7086-log-whether-account-is-in-reproofing branch November 7, 2022 20:24
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.

3 participants