Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

waivers/+filtered API endpoint: handle scenarios (#431) #105

Merged
merged 1 commit into from
Aug 14, 2023

Conversation

AdamWill
Copy link
Contributor

@AdamWill AdamWill commented Aug 8, 2023

The FilteredWaiversResource method which backs this endpoint tries to find only 'current' waivers by grouping all waivers by subject_type, subject_identifier and testcase, then taking only the highest-numbered waiver from each group. This breaks if we have multiple waivers for the same subject and testcase but with different scenarios - the endpoint will only return one waiver, the one with the highest ID, and will not return the valid and current waivers for other scenarios as it should.

The FilteredWaiversResource method which backs this endpoint
tries to find only 'current' waivers by grouping all waivers
by subject_type, subject_identifier and testcase, then taking
only the highest-numbered waiver from each group. This breaks if
we have multiple waivers for the same subject and testcase but
with different scenarios - the endpoint will only return one
waiver, the one with the highest ID, and will not return the
valid and current waivers for other scenarios as it should.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@AdamWill
Copy link
Contributor Author

AdamWill commented Aug 8, 2023

I'm pretty sure it's safe to do this even for waivers with no scenario, as the scenario will just be considered to be None in that case. If this was a problem I believe some other tests would fail (e.g. test_filtering_waivers_with_post, which creates waivers with no explicit scenario).

@hluk hluk merged commit cb2b0e2 into release-engineering:master Aug 14, 2023
9 checks passed
@hluk
Copy link
Member

hluk commented Aug 14, 2023

Good catch. Thanks for the fix! I'll deploy the fix after testing this internally.

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.

None yet

2 participants