Skip to content

Enqueue a letter every day to a designated receiver (LG-3939)#4558

Merged
zachmargolis merged 5 commits intomasterfrom
margolis-gpo-designated-receiver
Jan 5, 2021
Merged

Enqueue a letter every day to a designated receiver (LG-3939)#4558
zachmargolis merged 5 commits intomasterfrom
margolis-gpo-designated-receiver

Conversation

@zachmargolis
Copy link
Contributor

This will create a letter every day to be sent out

I made some assumptions, will drop comments on the diff to highlight questions I have

timeout: 300,
callback: lambda {
callback: lambda do
UspsDailyTestSender.new.run
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed easier to add this step to the existing daily job, rather than worry about race conditions across multiple daily jobs? I did add the top-level rescue to minimize the chances that this job will break the actual upload run

Comment on lines +39 to +46
def profile
@profile ||= user.profiles.first || user.profiles.create
end

def user
email = designated_receiver_pii[:email]
@user ||= User.find_with_email(email) || User.create(email: email)
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our DB schema doesn't allow a nil profile_id so it seemed easier to make up a profile than to try to relax that requirement?

My assumption is that most of the time, the designated receiver will be a login.gov team member with a valid email/user/profile, so these lines to create DB objects are more of a safety. I don't expect us to create many empty profiles/users.

Open to changing this approach if anybody feels strongly, I didn't feel super great about it

Copy link
Contributor

Choose a reason for hiding this comment

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

What are the implications if the designated receiver signs in with an empty profile? Or if they try to proof? It seems like there could be some weird behavior when we try to do things like decrypt nil PII.

Could we dodge the null constraint by setting profile id to -1 or something ugly like that?

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 to use profile_id = -1 in c69f1d3

# @return [Hash]
def designated_receiver_pii
@designated_receiver_pii ||= JSON.parse(
AppConfig.env.gpo_designated_receiver_pii,
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to make sure we go over this with privacy / security before we add PII to app config.

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, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zachmargolis zachmargolis requested a review from jmhooper January 5, 2021 18:19
@zachmargolis zachmargolis merged commit e82740a into master Jan 5, 2021
@zachmargolis zachmargolis deleted the margolis-gpo-designated-receiver branch January 5, 2021 21:22
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