@kivinen@iki.fi commented

Actually it should be possible for the reviewers themselves to put those in, i.e., in the document fill in the results of the unsolicited review to the system. This could also be used by the reviewer to indicate that he has reviewed the new version of the draft and he now wants to indicate that he is happy with changes, i.e., mark summary as Ready or similar.

@rjsparks@nostrum.com commented

To be clear, how is this different than any other community member who sends review comments by email. Why should this be tracked as a directorate review rather than just a community member review?

@kivinen@iki.fi commented

There is no fundamental technical difference, but mostly it is that review team members have been selected by the ADs to be knowledgeable about the security issues, and security area directors have some kind of trust of their ability to find useful things from the documents. I.e., security AD might be more interested in finding out the security area review team member reviews for the document than some other review team members review or review by someone not in any team. If all reviews by security area team members are flagged as such for the security AD, he/she can find them easier.

Also I think we at one point talked about ability to add non review team reviews to the system also, but as that is bigger change, and would require more work, this is something that can be done easier, and we already do have authorization and accounts ready for doing this.

Another use for this (which actually triggered my comment) is that review team member wants to change his review after the document has been modified. I.e., reviewer comments something and then document authors agree, and submit new version, and now the reviewer does another review and wants to submit new review text with perhaps of different summary.

One way of doing that would be to go to the old review and "correct" it. I.e., change the version number to new one, change summary, and the text or link to review. I think that is wrong way of doing that, and I think it would be better to do new review document in that case, and not modify the original, as that would keep the history easier to find.

Currently reviewers do not have option to do that. I as an secretary can do that, but it is not very simple.

@sasha@dashcare.nl commented

So, this can definitely be done, but there are some slightly hairy parts. With the current datamodel, it does not seem possible to simply register an unsolicited review. It requires creating/using a review request and a review assignment in the database, as that is how the review will be linked to a team and document.

The process to post an unsolicited review would be as follows:

1. On top of the usual review completion form, the user has to select which document they are reviewing, and for which team, as a reviewer can be in multiple teams.
2. When submitting the review, a review request will be created. This request will be attached to the selected team, and shown as having been requested by the reviewer. If there is currently an open review request, perhaps that could be used. However, I don't know whether a pre-existing review request status should then be updated to 'assigned', or left as is, and am not immediately sure whether there are some catches with either creating a new review request or using an existing one for that team.
3. A review assignment will be created, assigning the reviewer to the review request that they just "requested". This assignment will immediately be marked as completed and filled with the review information.
4. Deadline, assignment date and completion date will all be set to the date on which the review is posted.

One possible concern is that requesting a review and assigning a review are currently features restricted to certain roles. With this feature, any reviewer can essentially trigger the creation of a review request which they would usually not be permitted to, provided they also immediately post the review. Is that desirable?

In any case, it is possible, but it feels like stretching the current design which was built for a specific workflow by people with specific roles. I don't know whether this can cause undesirable confusion or side effects in the workflow. However, I don't really see an alternative design without major changes to the data model. Thoughts?

@rjsparks@nostrum.com commented

So the intent is to not let any random reviewer bring these objects into existence.

Rather, the idea is that a team secretary sees someone has done a review they were not assigned.
The unexpected reviewer would have sent this by email, not submitted it through the datatracker interface. The team secretary can then capture the review as if it had been assigned without having to manually go through the steps to create a request, make an assignment, complete the assignment.

So this should really be optimization of existing workflows rather than creating truly new ones.

@sasha@dashcare.nl commented

Ah, I was confused by this comment:

Actually it should be possible for the reviewers themselves to put those in, i.e., in the document fill in the results of the unsolicited review to the system.

Having the team secretaries enter the review, as you say, seems closer to the current workflow, indeed.

@rjsparks@nostrum.com commented

Hrmm. Yes, and the conversation in the ticket after it.

I think we should stay with the secretary for the moment (that's what I had in mind when I added this ticket to the project).

We can look different workflows, with the different permissions needed later. (I'll open a new ticket framing the problem when this on closes).

Capturing some thoughts that I'll move there at that time. Again, don't try to handle these now.

• We can probably safely accept volunteer reviews from arbitrary team members while a review request is open.
• We can probably safely accept a new review from someone who has previously reviewed this document even if the review request is no longer open. But we should look to see if this should be really modeled as a new request/review, or if we need to better model the notion of a follow-up review.
• Completely arbitrary reviews against arbitrary documents are something different than team reviews, and while we might leverage some of the existing code, if we were to pursue it, it would be a separate app.

@sasha@dashcare.nl changed status from new to closed

@sasha@dashcare.nl changed resolution from `` to fixed

@sasha@dashcare.nl commented

Fixed in 871a4b6:

Fix #2217 - Allow submission of unsolicited reviews by secretaries.

• For team secretaries, a button "Submit unsolicited review" will now
  appear next to "Request review" on the document's main page.
• If the secretary is a secretary for multiple teams, they are taken
  through an intermediate page to select for which team they are
  submitting their review.
• The form is similar (and using the same code) as the usual review
  completion, with a few extra fields for the review type and reviewer,
  which would usually already be known.
• When submitting the review, a ReviewRequest and ReviewAssignment are
  automatically created. The assignment is then immediately closed in
  the usual way.
• Other workflows are unchanged.

The issues with the review form in #2061 are slightly worse for the
unsolicited review scenario, but that will be improved when #2061 is
fixed.

Commit ready for merge.

@sasha@dashcare.nl commented

From 730e64d:

Refs #2217 - Small cleanup from changeset 871a4b6

Commit ready for merge.

@henrik@levkowetz.com commented

Fixed in c606461:

Merged in 871a4b6 from sasha@dashcare.nl:
Fix #2217 - Allow submission of unsolicited reviews by secretaries.

• For team secretaries, a button 'Submit unsolicited review' will now
  appear next to 'Request review' on the document's main page.
• If the secretary is a secretary for multiple teams, they are taken
  through an intermediate page to select for which team they are
  submitting their review.
• The form is similar (and using the same code) as the usual review
  completion, with a few extra fields for the review type and reviewer,
  which would usually already be known.
• When submitting the review, a ReviewRequest and ReviewAssignment are
  automatically created. The assignment is then immediately closed in
  the usual way.
• Other workflows are unchanged.
  The issues with the review form in "Complete review" workflow for secretary should be optimized #2061 are slightly worse for the
  unsolicited review scenario, but that will be improved when "Complete review" workflow for secretary should be optimized #2061 is
  fixed.

@henrik@levkowetz.com commented

From 0f12d47:

Merged in 730e64d from sasha@dashcare.nl:
Refs #2217 - Small cleanup from changeset 871a4b6