Repository navigation
fix(backend): validate Display start_at must not exceed expires_at in Announcement ($25 bounty proposal) - #21510
aiwithrajan wants to merge 1 commit into
Conversation
… Announcement ($25 bounty proposal)
|
Thanks @aiwithrajan — reviewed and verified. backend/models/announcement.py: the new model_validator(mode="after") on Display only fires when both start_at and expires_at are set and start_at > expires_at, and it runs after _normalize_display_datetimes, so the comparison is always on UTC-normalized values (no naive/aware pitfalls). Inverted display sub-documents read through Announcement.from_dict are caught by the existing _optional_submodel guard and dropped, which matches how this model already tolerates malformed sub-documents (bad enum, bad type, bad datetime) so one bad document can't 500 the announcements list. backend/tests/unit/test_announcement_display_bounds.py: good coverage — valid bounds, equal timestamps, one-sided bounds, the ValidationError message, and the from_dict drop path. I ran this suite plus the sibling announcement suites locally: all green. One behavior note for the record rather than a change request: any stored document that already has an inverted display window will now have its display sub-document dropped and fall back to get_effective_display's unbounded default (show immediately, no expiry) instead of a broken-but-bounded window. That seems like the right default and is consistent with the model's existing malformed-subdoc policy, but maintainers should be aware legacy docs with inverted windows change visible behavior. Nice, tight contribution. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
|
Hey @aiwithrajan 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Before your next PR, please skim:
If this was declined for direction or taste, maintainers should cite an invariant ID or open a proposed one — ask if that citation is missing. Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! |
Description
Validates that
Displayinbackend/models/announcement.pyenforcesstart_at <= expires_at.Currently,
Displaynormalizesstart_atandexpires_atto UTC but lacks bounds validation. Ifstart_atis afterexpires_at, an inverted display window is accepted, which causes temporal schedule anomalies. Furthermore,Announcement.from_dictuses_optional_submodel(Display, display_data)to guard against malformed display configurations; with bounds validation in place, documents with inverted display timestamps are safely caught asValidationErrorand dropped rather than causing schedule corruption.This change:
model_validatortoDisplayto ensurestart_at <= expires_atwhen both timestamps are specified.backend/tests/unit/test_announcement_display_bounds.pycovering valid bounds, equal timestamps, one-sided bounds,ValidationErroron inverted dates, and graceful sub-document dropping inAnnouncement.from_dict.Test Plan