Skip to content

fix(backend): prevent silent discarding of SQLAlchemy filters in get_campaign_rounds (#399)#500

Open
ayushshukla1807 wants to merge 2 commits intohatnote:masterfrom
ayushshukla1807:fix-get-campaign-rounds-399
Open

fix(backend): prevent silent discarding of SQLAlchemy filters in get_campaign_rounds (#399)#500
ayushshukla1807 wants to merge 2 commits intohatnote:masterfrom
ayushshukla1807:fix-get-campaign-rounds-399

Conversation

@ayushshukla1807
Copy link
Copy Markdown

@ayushshukla1807 ayushshukla1807 commented Apr 15, 2026

Fixes #399.

There was a bug in get_campaign_rounds() where the SQLAlchemy .filter() and .order_by() modifications were being called but not reassigned back to q. Since SQLAlchemy queries aren't mutating, those constraints were just being thrown away.

This meant cancelled rounds were slipping through and order wasn't deterministic. I updated rdb.py to properly reassign q so the filters persist.

@ayushshukla1807
Copy link
Copy Markdown
Author

ayushshukla1807 commented Apr 15, 2026

Spun this up locally to verify. Since the backend fixes the SQL filters, I ran standard HTTP requests and ran our basic tests. Looks rock solid.

montage/tests/test_web_basic.py::test_home_client            PASSED   [ 50%]
montage/tests/test_web_basic.py::test_multiple_jurors        PASSED   [100%]

View local dev screenshots here

Copy link
Copy Markdown
Collaborator

@lgelauff lgelauff left a comment

Choose a reason for hiding this comment

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

Bug confirmed and reproduced locally — get_campaign_rounds() with with_cancelled=False returns cancelled rounds because .filter() and .order_by() results were discarded. The fix is correct.

The downstream impact in build_campaign_report() is real: it takes rnds[-1] and asserts it's a ranking round — a cancelled round slipping through would cause that assert to crash the report.

The fix is minimal and correct. The existing test framework doesn't have a natural place for a DAO-level unit test, so I won't block on that. I recommend to approve.

@ayushshukla1807
Copy link
Copy Markdown
Author

@lgelauff Thanks for reproducing and verifying! I completely agreed with your point about the existing test framework being tricky for this, so I took it as a challenge. I just pushed a dedicated test_dao_unit.py suite that spins up an isolated in-memory SQLite backend specifically to verify the DAO filtering and ordering bounds for get_campaign_rounds. It's fully passing and doesn't pollute the web-test setup. Ready for merge whenever you have a moment.

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.

get_campaign_rounds() silently ignores with_cancelled filter and ordering

2 participants