feat(rollups): add rollup_member mapping

[Carbonhell]

Closes #494.

I wasn't sure whether to use IDs from the pull_request table to represent PRs in the mapping, or just the GitHub PR number - I went with the latter as there would be no record for the rollup at the time of its creation, as that gets populated via GitHub events, if I understood correctly.

I haven't experienced rollups at all as I haven't (yet?) contributed to a project with bors, but I noticed there's no safety check for multiple rollups containing the same PR(s) being opened. I found a related comment here, but I kept it out of this PR to reduce its scope and simplify review. I think such a safety check could be added easily by checking for rollups associated to each candidate PR, and verifying the status of the rollup PR to ignore closed ones. That would still allow one to recreate the same scenario by reopening a previously closed rollup, though.

One thing I'm unsure about is how to handle the error when writing the rollup_contents records in the DB. I think that should only happen either in case of DB connection errors, or if the list of PRs contains duplicates. I assume the former can be propagated to the caller, whereas I'm not sure if the latter is realistic and should be accounted for. I'm thinking we can just ignore the conflicting records in such case - let me know if I should proceed with that.

Edit: sorry, I forgot the migration test files. Will push them tomorrow as soon as possible

[Kobzol]

Hi, thanks for the PR! I'll take a look, but I don't know when I'll be able to get to it, since it's Holiday season :)

I wasn't sure whether to use IDs from the pull_request table to represent PRs in the mapping, or just the GitHub PR number - I went with the latter as there would be no record for the rollup at the time of its creation, as that gets populated via GitHub events, if I understood correctly.

Yeah, that makes sense.

but I noticed there's no safety check for multiple rollups containing the same PR(s) being opened.

This is not a problem on its own, there are often multiple rollups that contain a single PR, because if a rollup fails, you create a new one, often before the old one is closed. I don't think that we need to think about that right now.

if the list of PRs contains duplicates. I assume the former can be propagated to the caller, whereas I'm not sure if the latter is realistic and should be accounted for.

Also a good point! I'll ignore duplicate PR numbers in rollups.

[Kobzol]

I think that the main query that we will want to execute on the queue page would look something like this:

SELECT
    rollup_pr_number, member_pr_number
FROM rollup_member
WHERE repository = $1 AND
      rollup_pr_number IN (SELECT number
                           FROM pull_request AS pr
                           WHERE pr.repository = $1
                             AND pr.status IN ('open', 'draft'))
ORDER BY rollup_pr_number;

And we get back something like HashMap<PrNum, HashSet<PrNum>>, which maps rollups to their member PRs. Because we assume that there will be only a couple of open rollups, so it seems easier and faster to do it this way, rather than try to find a corresponding rollup for each PR in the queue.

[Carbonhell]

Sorry for the long delay (and the force-pushes 👀), the PR should be ready for another review now. Thank you in advance :)

[Kobzol]

Thank you, it looks good!

[Kobzol]

I wouldn't actually mind having some basic usage of the new queries in the same PR - it often turns out that a new API will need to be changed once it's actually used 😆 Would you like to take a stab at that?

Some ideas:

1. If a rollup is currently being tested (it has state pending), it should not be possible to add its members to a new rollup (the rollup checkboxes should be disabled for the members)
2. Each rollup in the queue could have some color associated to it, and we could show some small rectangle or circle before/after the rollup PR number or title, in the queue table. And then we could also add this rectangle/circle at the same spot for the rollup members, to make it visually obvious which members belong to which rollups.
3. When you hover on a rollup, its members could be highlighted.

I think that 3. might be the simplest to implement, and it would be enough to test that the queries that we have here make sense. The rest could be done in follow-up PRs.

[Carbonhell]

I wouldn't actually mind having some basic usage of the new queries in the same PR - it often turns out that a new API will need to be changed once it's actually used 😆 Would you like to take a stab at that?

Some ideas:

1. If a rollup is currently being tested (it has state pending), it should not be possible to add its members to a new rollup (the rollup checkboxes should be disabled for the members)
2. Each rollup in the queue could have some color associated to it, and we could show some small rectangle or circle before/after the rollup PR number or title, in the queue table. And then we could also add this rectangle/circle at the same spot for the rollup members, to make it visually obvious which members belong to which rollups.
3. When you hover on a rollup, its members could be highlighted.

I think that 3. might be the simplest to implement, and it would be enough to test that the queries that we have here make sense. The rest could be done in follow-up PRs.

Honestly I agree on implementing the usage of the new queries in this PR. I've been sticking to small PRs mainly to facilitate the reviews and to avoid making it too time consuming for maintainers, as I'll probably do a few mistakes since I just started contributing :) I'd be more than happy to handle it! I'll look into it in this weekend

[Kobzol]

Yeah ofc small PRs are good in general. On the other hand, I tend to be a bit wary of PRs that just add new functionality without using it, because very often when we start using it we realize that we need to make changes to it anyway 😆

[Carbonhell]

As agreed on Zulip, for now I've only implemented the hover functionality, as seen in the following screenshot (mouse hovering on a rollup not shown):

I've also fixed the issue with the multiple_rollups_same_pr test from the last review.
The PR should be ready for review. Thanks in advance :)

[Kobzol]

Left a couple of small nits, but otherwise this looks really awesome! The hover also looks and works great ❤️ Thank you!

[Kobzol]

Hi, we would like to land this and build a bunch of additional features on top. Would you mind if I rebased and finished the PR for you?

[Carbonhell]

Hey, no, feel free to! I'm really sorry for not coming back to this, I've been quite busy with work. Feel free to message me on Zulip if I can help anyhow

[Kobzol]

No problem! You already did 99% of the work here, so I'll just try to finish it :)

[Kobzol]

Thank you very much for working on this! It will unlock a lot of useful functionality.