Implement match and match cycle repos - #74
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
5470d2c to
dbbd19b
Compare
Graphite Automations"Request reviewers once CI passes" took an action on this PR • (08/10/26)2 reviewers were added to this PR based on Henry Chen's automation. |
|
Code looks good, although I wonder if the match cycle repo could use some per-field update methods like match has. Also, we planning on writing any tests for these? Not sure if there's another ticket for them |
dbbd19b to
b38d8dc
Compare
b38d8dc to
1eefc69
Compare
thanks for the feedback il include tests in the next PR. Will update this one with the per field methods |
7421818 to
6e2e2a1
Compare
adb4bd4 to
3df3274
Compare
3df3274 to
b63a2c8
Compare
|
One other thing is that you're building out everything together layer by layer, which makes sense with how we planned it out and with AI implementing it, but it would be easier to build it out feature by feature once we get it to a little more stable state. Like having the basics set, and then adding filtering and feedback later down, instead of including it from the start. |
b63a2c8 to
dedfb71
Compare
dedfb71 to
33ce420
Compare
66cc820 to
5294cd8
Compare
33ce420 to
3c16f9e
Compare
3c16f9e to
081d755
Compare
5294cd8 to
8156404
Compare
|
❌ The last analysis has failed. |
| } | ||
|
|
||
| @Override | ||
| public Optional<MatchCycle> setMatchCycleDraft(Integer id, boolean isDraft) { |
There was a problem hiding this comment.
Nit: better titled as setMatchCycleIsDraft, this way it's clearer that it's a Boolean switch.
| public MatchCycle createMatchCycle(MatchCycle matchCycle) { | ||
| String sql = """ | ||
| INSERT INTO "match_cycles" ( | ||
| "period", |
There was a problem hiding this comment.
Postgres has range types we can use to do range searches, that would probably be a better type fit for this field. This would probably be more effective than shoehorning the range into a single format. Unsure how much we'll need that in the future though.
| String sql = """ | ||
| INSERT INTO "matches" ( | ||
| "id", | ||
| "member_a_id", |
There was a problem hiding this comment.
A few thoughts here:
- would we benefit from including member emails so the queries would be simpler? I'm assuming member ids are not indexed right now so it probably wouldn't make a difference performance wise. We could also include both in the table
- We should have some kind of unique constraint or at least index on the pairing of member an and b's ids/emails. So we can easily look up "have a and b matched before? Or have b and a matched before"? Because that would be an instantly disqualifying factor.
8156404 to
f007db1
Compare
081d755 to
7fe64eb
Compare
7fe64eb to
fd90b93
Compare
b2bfd00 to
a722b4c
Compare
| String sql = """ | ||
| INSERT INTO "match_cycles" ( | ||
| "period", | ||
| "run_at", | ||
| "is_draft", | ||
| "total_members" | ||
| ) | ||
| VALUES( | ||
| :period, | ||
| :run_at, | ||
| :is_draft, | ||
| :total_members | ||
| ) | ||
| RETURNING | ||
| *, | ||
| %s | ||
| """.formatted(TOTAL_MATCHED_SQL); | ||
|
|
||
| return jdbc.sql(sql) | ||
| .param("period", matchCycle.getPeriod()) | ||
| .param("run_at", matchCycle.getRunAt()) | ||
| .param("is_draft", matchCycle.isDraft()) | ||
| .query((rs, rowNum) -> parseResultSetToMatchCycle(rs)) | ||
| .single(); | ||
| } |
There was a problem hiding this comment.
Critical Bug: Missing parameter binding for total_members
The INSERT statement on line 37 includes "total_members" as a column and references :total_members as a parameter on line 43, but this parameter is never bound in the JDBC client call (lines 50-55).
The MatchCycle model also doesn't have a totalMembers field, and based on the interface documentation changes (lines 15-20 in MatchCycleRepo.java), this field was intentionally removed.
Impact: This will cause a SQL parameter binding error at runtime when attempting to create a match cycle.
Fix: Remove the total_members column and parameter from the INSERT statement:
String sql = """
INSERT INTO "match_cycles" (
"period",
"run_at",
"is_draft"
)
VALUES(
:period,
:run_at,
:is_draft
)
RETURNING
*,
%s
""".formatted(TOTAL_MATCHED_SQL);| String sql = """ | |
| INSERT INTO "match_cycles" ( | |
| "period", | |
| "run_at", | |
| "is_draft", | |
| "total_members" | |
| ) | |
| VALUES( | |
| :period, | |
| :run_at, | |
| :is_draft, | |
| :total_members | |
| ) | |
| RETURNING | |
| *, | |
| %s | |
| """.formatted(TOTAL_MATCHED_SQL); | |
| return jdbc.sql(sql) | |
| .param("period", matchCycle.getPeriod()) | |
| .param("run_at", matchCycle.getRunAt()) | |
| .param("is_draft", matchCycle.isDraft()) | |
| .query((rs, rowNum) -> parseResultSetToMatchCycle(rs)) | |
| .single(); | |
| } | |
| String sql = """ | |
| INSERT INTO "match_cycles" ( | |
| "period", | |
| "run_at", | |
| "is_draft" | |
| ) | |
| VALUES( | |
| :period, | |
| :run_at, | |
| :is_draft | |
| ) | |
| RETURNING | |
| *, | |
| %s | |
| """.formatted(TOTAL_MATCHED_SQL); | |
| return jdbc.sql(sql) | |
| .param("period", matchCycle.getPeriod()) | |
| .param("run_at", matchCycle.getRunAt()) | |
| .param("is_draft", matchCycle.isDraft()) | |
| .query((rs, rowNum) -> parseResultSetToMatchCycle(rs)) | |
| .single(); | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
4caac08 to
edba339
Compare
|




No description provided.