Redesign review assignment such that each students gets a separate review issue

[slarse]

GitLab CE and GitHub free do not support multiple assignees per issue (#532 #696). This causes trouble for the current review assigning scheme, which assigns all reviewers for a single repository to a single issue.

A workaround for this is to open one issue per reviewer.

[Adamih]

We are working on this issue.

[slarse]

Cool, assigning you to it!

How far have you progressed? I'm currently refactoring the peer.assign_peer_reviews function as it's an absolute monster and the new pylint rules I'm introducing (see #1003) lit up like a Christmas tree on it. It will probably conflict with any code you've written.

[ZinoKader]

Cool, assigning you to it!

How far have you progressed? I'm currently refactoring the peer.assign_peer_reviews function as it's an absolute monster and the new pylint rules I'm introducing (see #1003) lit up like a Christmas tree on it. It will probably conflict with any code you've written.

Me and Adam are in a project group for a course in KTH (fellow alum!), so we are both working on it. We have just started out laying out the requirements, but have not written any code yet. Do you have an estimate for when you will be done rewriting it? We might just continue on the current version of the function and resolve the conflicts afterwards if the refactoring won't be ready for some time.

[slarse]

Me and Adam are in a project group for a course in KTH (fellow alum!), so we are both working on it. We have just started out laying out the requirements, but have not written any code yet.

Ah, I see, DD2480?

Do you have an estimate for when you will be done rewriting it? We might just continue on the current version of the function and resolve the conflicts afterwards if the refactoring won't be ready for some time.

Probably tomorrow, I started picking it apart today but got a bit sidetracked about halfway through.

[Adamih]

@slarse Great! Please assign @ZinoKader as well. Looking forward to seeing your changes.

[Adamih]

Our initial solution involves simply creating multiple issues tagging each member of the reviewing team. Very easy fix.

for member in review_team.members:
    issue_title = f"{issue.title} ({member})"
    api.create_issue(
        issue_title,
        issue.body,
        reviewed_repo,
        assignees=[member]
        if not isinstance(api, _repobee.ext.gitea.GiteaAPI)
        else None,
    )

[slarse]

@Adamih That seems reasonable. Just off the top of my head, I'm a little bit uncertain how that plays with the double-blind review functionality. I frankly don't remember how that even works. There may be a need to add a new test when running in double-blind mode to ensure that there's still only one (anonymous) issue created when in double blind mode.

For context, see the double-blind review docs here

[Adamih]

@Adamih That seems reasonable. Just off the top of my head, I'm a little bit uncertain how that plays with the double-blind review functionality. I frankly don't remember how that even works. There may be a need to add a new test when running in double-blind mode to ensure that there's still only one (anonymous) issue created when in double blind mode.

For context, see the double-blind review docs here

We will try double blind and create some rudimentary unit tests. Thanks for the input.

[slarse]

@Adamih Actually, now I'm just confusing myself. This works transparently for double-blind review as the "owner" of the reviewed repo doesn't actually have access to the repo copy (it would of course not be double-blind otherwise). So there's no need for any special considerations for double-blind review, your implementation looks reasonable.

The only real effort will be to test the new functionality, which will probably amount to adapting existing tests.

[slarse]

There's now a PR with a moderately successful refactor of the assign_peer_reviews function. It's still a bit of a mess, but the part that's relevant to this issue should now be contained in the _assign_review function.

Will have to merge tomorrow (granted that the system tests pass), it took me a wee bit too many hours to figure out a halfway decent split of the functionality. That module was very much written with the mindset of "this is tomorrow me's problem". And it sure is.

[Adamih]

There's now a PR with a moderately successful refactor of the assign_peer_reviews function. It's still a bit of a mess, but the part that's relevant to this issue should now be contained in the _assign_review function.

Will have to merge tomorrow (granted that the system tests pass), it took me a wee bit too many hours to figure out a halfway decent split of the functionality. That module was very much written with the mindset of "this is tomorrow me's problem". And it sure is.

I'm very much under the controversial opinion that long functions are totally ok, so it doesn't affect me that much. I will check out your PR though and whip up a test or two.

[slarse]

I'm very much under the controversial opinion that long functions are totally ok, so it doesn't affect me that much. I will check out your PR though and whip up a test or two.

It's merged :)

I'm not a zealot about function length, but from experience, a long function is either littered with inline comments explaining what the different blocks do (and at that point you might as well create a function with a good name), or it isn't and then it's hard to understand the intent of the different parts of the function.

There's of course also a downside to factoring a long function into smaller components. As is evident from my (now merged) refactoring, there are some intricate couplings between the functions that are handled with callbacks, which adds complexity of its own. Which I intend to massage away, I just ran out of time.

But, now that the initial refactoring is done, you can happily chop away at your isolated part of the functionality while I can keep refactoring, and the chances that our edits conflict is reduced compared to when everything is in the same function, which importantly has the same scope. Scopes in Python are a little bit devious as well as only functions create new scopes, whereas stuff like loops and conditional blocks don't.