Skip to content

ENG-2720: Add units to privacy_request_delay_timeout#7415

Merged
tvandort merged 2 commits intomainfrom
ENG-2720
Feb 18, 2026
Merged

ENG-2720: Add units to privacy_request_delay_timeout#7415
tvandort merged 2 commits intomainfrom
ENG-2720

Conversation

@tvandort
Copy link
Copy Markdown
Contributor

@tvandort tvandort commented Feb 18, 2026

Ticket ENG-2720

Description Of Changes

Adds units to configuration setting.

Code Changes

Created a variable so that code intent is clear with less thinking.

Steps to Confirm

  1. None, just read. :)

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@tvandort tvandort requested a review from a team as a code owner February 18, 2026 18:41
@tvandort tvandort requested review from thabofletcher and removed request for a team February 18, 2026 18:41
@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Feb 18, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Feb 18, 2026 6:42pm
fides-privacy-center Ignored Ignored Feb 18, 2026 6:42pm

Request Review

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Feb 18, 2026

Greptile Summary

This PR adds unit documentation ("in minutes") to the privacy_request_delay_timeout configuration field and improves readability of the is_callback_token_expired function in oauth/utils.py.

  • Updated privacy_request_delay_timeout field description to explicitly state the unit is minutes
  • Extracted inline timedelta calculation into a named variable minutes_since_issuance for clarity
  • Removed inaccurate "(24 hours)" claim from the is_callback_token_expired docstring

No behavioral changes — this is purely a documentation and readability improvement.

Confidence Score: 5/5

  • This PR is safe to merge — it contains only documentation and variable naming improvements with zero behavioral changes.
  • The changes are minimal, limited to a field description update and extracting an inline expression into a named variable. No logic, control flow, or configuration defaults were modified. Both files are well-understood and the changes are trivially verifiable.
  • No files require special attention.

Important Files Changed

Filename Overview
src/fides/api/oauth/utils.py Extracts inline timedelta calculation into a descriptively named minutes_since_issuance variable and removes an inaccurate "24 hours" docstring claim. No behavioral changes.
src/fides/config/execution_settings.py Adds "in minutes" unit clarification and default value to the privacy_request_delay_timeout field description. No behavioral changes.

Last reviewed commit: 7f4f994

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment on lines 14 to 17
privacy_request_delay_timeout: int = Field(
default=3600,
description="The amount of time to wait for actions which delay privacy requests (e.g., pre- and post-processing webhooks).",
description="The amount of time to wait, in minutes, for actions which delay privacy requests (e.g., pre- and post-processing webhooks). Default: 3600 minutes",
)
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.

Default of 3600 minutes seems suspect

The description now states the unit is minutes with a default of 3600 — that's 60 hours (2.5 days). The old docstring on is_callback_token_expired claimed this was "24 hours," which would be 1440 minutes, not 3600.

Is it possible this value was originally intended to be in seconds (3600 seconds = 1 hour)? If the unit truly is minutes, the default seems very large and the previous "24 hours" comment was wrong. Could you confirm the intended timeout duration and adjust either the default value or the unit accordingly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, the original comment was wrong. It always would have been wrong since the value has always been configurable. I'm not going to change it from 60 hours given that it's now expected behavior by anyone relying on these.

@tvandort
Copy link
Copy Markdown
Contributor Author

@greptileai pls rereview

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Copy Markdown
Contributor

@Linker44 Linker44 left a comment

Choose a reason for hiding this comment

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

Finally some clarity.

@tvandort tvandort added this pull request to the merge queue Feb 18, 2026
Merged via the queue into main with commit 89eaab6 Feb 18, 2026
53 of 54 checks passed
@tvandort tvandort deleted the ENG-2720 branch February 18, 2026 20:20
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.

2 participants