Skip to content

chore(auth-server): minor code cleanup and fixes#19940

Closed
sreecharan-desu wants to merge 2 commits into
mozilla:mainfrom
sreecharan-desu:fix/auth-server-cleanups
Closed

chore(auth-server): minor code cleanup and fixes#19940
sreecharan-desu wants to merge 2 commits into
mozilla:mainfrom
sreecharan-desu:fix/auth-server-cleanups

Conversation

@sreecharan-desu

@sreecharan-desu sreecharan-desu commented Jan 25, 2026

Copy link
Copy Markdown
Contributor

This PR introduces a few targeted cleanups and improvements to the fxa-auth-server package:

  • Fix Typo: Corrected 'seperately' to 'separately' in the OAuth API test suite.
  • Unhardcode Session Token Expiry: Updated the SessionToken model to pull its expiry from the auth-server configuration instead of using a hardcoded static value.
  • Added Regression Tests: Included unit tests for the session token expiry filtering logic to ensure correctness.

These changes address the maintainer's feedback regarding targeted PRs and unhardcoding configuration values, while maintaining existing functionality.

@sreecharan-desu sreecharan-desu requested a review from a team as a code owner January 25, 2026 14:05
@github-actions

Copy link
Copy Markdown
Contributor

This pull request issue has been marked as stale due to inactivity. It will be closed in a week if it continues to be stale.

Comment thread packages/fxa-auth-server/lib/senders/emails/sass-compile-files.ts
Comment thread packages/fxa-auth-server/test/remote/oauth_api.in.spec.ts
Comment thread packages/fxa-auth-server/test/oauth/api.js Outdated
Comment thread packages/fxa-auth-server/test/oauth/api.js Outdated
Comment thread packages/fxa-shared/db/models/auth/session-token.ts

@dschom dschom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for these contributions. This PR has grown stale, but you have some nice clean up here. Please see comments and feel free to refile more targeted PRs. We will try to address them in a timely manner this time around.

@sreecharan-desu

sreecharan-desu commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback, @dschom! I've updated the PR to address your requests:

  • Reverted Sass Improvements: Removed the changes to sass-compile-files.ts as they are no longer applicable.
  • Fixed Typo: Corrected 'seperately' to 'separately' in the contemporary test file location without any formatting noise.
  • Unhardcoded Session Token Expiry: The expiry is now dynamically set from the auth-server configuration instead of being a static member. I've also added unit tests in fxa-shared to verify the logic.
  • Consolidated Effort: I've integrated the typo fix directly here and closed the redundant PR (fix(auth-server): correct typo 'seperately' to 'separately' in tests #20399) to keep the cleanup focused in this thread.

I hope this targeted version is much easier to review. Let me know if you need any further adjustments!

@github-actions

Copy link
Copy Markdown
Contributor

This pull request issue has been marked as stale due to inactivity. It will be closed in a week if it continues to be stale.

@sreecharan-desu sreecharan-desu force-pushed the fix/auth-server-cleanups branch from c866e48 to de06dc4 Compare June 28, 2026 08:14
@sreecharan-desu

sreecharan-desu commented Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

Following up on the earlier review feedback — all requested changes were already addressed, and I've now rebased this branch onto latest main to clear the stale status and re-trigger CI.

Summary of what's in this PR:

  • Typo fix: seperatelyseparately in oauth_api.in.spec.ts
  • Session token expiry wired to auth-server config with unit tests
  • Sass compile changes reverted (per review)
  • No formatting-only changes in unrelated lines

Would appreciate a re-review when you have time, @dschom.

@sreecharan-desu

Copy link
Copy Markdown
Contributor Author

Closing this in favor of two focused PRs per @dschom's review feedback:

Both are rebased on latest main, use the official PR template, and have Angular-format commit messages with bodies.

@sreecharan-desu

Copy link
Copy Markdown
Contributor Author

Superseded by #20800 and #20801.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants