Handle open/close PR events in PR tracking #1905
Conversation
| record_username(db, user.id, &user.login) | ||
| .await | ||
| .context("failed to record username")?; |
There was a problem hiding this comment.
Here there is now a "hidden" a second SQL query where I expect to only have one. upsert_pr_into_workqueue says to just "Add a PR to the workqueue of a team member", we are sort of breaking this promise.
Maybe call record_username() just before (at the cost of calling everywhere)?
There was a problem hiding this comment.
I think that it's better if the function works in a self-contained way, rather than not working if you don't call a specific other function before it. We don't call it in a loop outside tests, it's always called incrementally, and the code that calls it always called record_username before it anyways, so there wouldn't be any change.
But mainly, I want to nuke this whole code in a follow-up PR, so.. 😆
src/handlers/pr_tracking.rs
Outdated
| record_username(client, user.id, &user.login).await.unwrap(); | ||
| for &pr in prs { | ||
| upsert_pr_into_workqueue(&ctx.db_client().await.client(), user.id, pr as u64) | ||
| .await | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
given my other comment, here you would end with multiple redundant SQL INSERT being run.
(ok this is just a test, but still)
This should improve the robustness of PR tracking, as before it was not taking (re)open/close/merge PR events into account. This PR also extends the test infrastructure. We still don't test the input/output GitHub API calls, but even just testing the DB is better than nothing.
Best reviewed commit by commit.
r? @apiraino :)