Skip to content

LG-7814: Cache USPS auth token as class member#7147

Merged
sheldon-b merged 9 commits intomainfrom
sbachstein/lg-7814-cache-usps-auth-token-as-class-member
Oct 18, 2022
Merged

LG-7814: Cache USPS auth token as class member#7147
sheldon-b merged 9 commits intomainfrom
sbachstein/lg-7814-cache-usps-auth-token-as-class-member

Conversation

@sheldon-b
Copy link
Contributor

@sheldon-b sheldon-b commented Oct 14, 2022

🎫 Ticket

LG-7814

🛠 Summary of changes

  • Cache the USPS API authorization token as a class variable instead of an instance variable

@sheldon-b sheldon-b marked this pull request as ready for review October 18, 2022 13:50
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.

It might be a good idea to test this against live APIs before it's deployed to production, but implementation LGTM 👍

Sheldon Bachstein and others added 3 commits October 18, 2022 10:54
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>
@sheldon-b
Copy link
Contributor Author

@aduth will definitely test it out in a lower environment, I'm thinking I'll go ahead and merge it and do some testing in dev. I added it as an A/C to the ticket

@sheldon-b sheldon-b merged commit 8f9bfba into main Oct 18, 2022
@sheldon-b sheldon-b deleted the sbachstein/lg-7814-cache-usps-auth-token-as-class-member branch October 18, 2022 15:39
@@ -1,5 +1,6 @@
module UspsInPersonProofing
class Proofer
mattr_reader :token, :token_expires_at
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be safe to use Rails.cache for this to share across instances?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so but is there a benefit to that over a class member?

Copy link
Contributor

Choose a reason for hiding this comment

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

Class members can be hard to test/reason about sometimes mattr_reader vs cattr_reader and then have to be reset explicitly in tests, but Rails.cache has easy methods for clearing it.

Also with like 10+ boxes, with probably 4+ processes each, each one will have to cache separately, so having a shared cache (we use Redis to back our Rails.cache) might save extra requests to the API overall

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh cool it's shared across processes and instances? Yeah I'll go ahead and switch it to Rails.cache

Copy link
Contributor

Choose a reason for hiding this comment

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

in prod, yes!, locally it might just be in-memory

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, seeing this conversation after PRing an identical approach. @sheldon-b will this be updated in a future PR? maybe we can pair on this and this one ...

Copy link
Contributor Author

@sheldon-b sheldon-b Oct 26, 2022

Choose a reason for hiding this comment

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

Yeah the original ticket is already marked Done and shipped to prod. I've created LG-7918 to move the caching to the rails cache

Happy to pair! I'll bring LG-7918 to refinement today

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