Fix method on_label_removal #10
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Test ProtocolThe first commit of sagemath#36292 is succsessful. This can be seen from the following lines in https://github.com/soehms/sage/actions/runs/6240998225/job/16942284373 : Item 1. of this PR is successful. This can be seen from the following lines in https://github.com/soehms/sage/actions/runs/6231889391/job/16914121219 Item 2. amd 3. are successful. This can be seen from the following lines in https://github.com/soehms/sage/actions/runs/6257958176/job/16991122087 The fix in |
5 tasks
soehms
pushed a commit
that referenced
this pull request
Oct 9, 2023
Removed use of "copy" and condensed multiple lines to one
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Oct 28, 2023
…tion
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
Unfortunately the fix in sagemath#36213 has
not been completed properly. There is still a `return` from the
`get_review_decision` method which is not `None` where it should be.
This is fixed here. Sorry for this unnecessary trouble and my lack of
attention!
In a next step this PR fixes a bug which has been noticed by an
accidentally activation of the `unlabeled` trigger (see
sagemath#36292 (comment)).
Simultanously the `labeled`-trigger has been disabled. This is an
unrealistic setting which was not covered by the code.
Explicitely, a state label could not be removed even though other state
labels where set (examples sagemath#36128
and sagemath#36020). This PR leads to the
following changes of behavior:
1. The reaction on the `unlabeled`-event is reduced to the case of the
last label
2. The `unlabeled`-event will not lead to a rejection of the removal any
more
3. Instead of a warning comment a hint comment is posted.
We found a way to test the bot a little more realistically (see
soehms#11). Additional problems emerged and
were also resolved. This led to the following changes in the workflow:
4. A bug in `actor_valid` is fixed (see
soehms#10 (comment))
5. Preparing to use our own bot user (e.g.
[sagemathadmins](https://github.com/sagemathadmins)) (see
soehms#11 (comment))
6. Waiver of observing the `reviewDecision` feature provided by GitHub
(see 2. in
soehms#11 (comment))
7. Allow the user to revert his decision of label selection (see
soehms#11 (comment))
8. Don't reject label addition any more, except in the case where the
author tries to set `s: positive review` to his own PR which has no
reviews from others (see
soehms#11 (comment))
9. Dismiss stale reviews of the bot after a push to the branch and on
submission of a new review which is more than just a comment (see
soehms#11 (comment)).
Despite the fact that the testing was now more realistic, it is still
not guaranteed that the bot's behavior in `sagemath/sage` will be
completely covered by our testing.
The changes in this PR cannot be activated immediately after merging
into the develop branch due to a bug in the GitHub web interface
observed during testing in soehms#11 (see
soehms#11 (comment) and
soehms#11 (comment)). The
problem is that the panel at the top right of the webpage that contains
the labels does not update immediately after the bot changes the labels.
Since this might cause confusion, I'll create a bug report about it. The
going-live of the sync bot will be stalled at least until we received an
answer.
This PR changes the following flowcharts from
sagemath#35172 :
##### What happens when adding `s: needs review` to a PR?
```mermaid
---
title: add the needs review label
---
flowchart LR
%% vertices
trigger([label\n 's: needs review'\n added])
mark_as_ready([mark as\n ready for\n review])
remove_other_labels([remove other\n state labels])
warning_about_label_addition([warning\n about label\n addition])
needs_review_valid[[needs review\n valid?]]
is_draft{is PR\n a draft?}
%% edges
trigger --> needs_review_valid
needs_review_valid -- true ---> remove_other_labels
needs_review_valid -- false ---> is_draft
is_draft -- yes ---> mark_as_ready
is_draft -- no ---> warning_about_label_addition
mark_as_ready --> remove_other_labels
```
The warning is posted as a comment which will be deleted after 5
minutes.
```mermaid
---
title: needs review valid?
---
flowchart LR
%% vertices
needs_review_valid([needs review\n valid?])
true([true])
false([false])
latest_review_by_actor{latest review\n by actor}
needs_work_valid[[needs work\n valid?]]
positive_review_valid[[positive review\n valid?]]
is_draft{is PR\n a draft?}
%% edges
needs_review_valid ---> is_draft
is_draft -- yes ---> false
is_draft -- no ---> latest_review_by_actor
latest_review_by_actor -- yes ---> true
latest_review_by_actor -- no ---> needs_work_valid
needs_work_valid -- true ---> false
needs_work_valid -- false ---> positive_review_valid
positive_review_valid -- true ---> false
positive_review_valid -- false ---> true
```
```mermaid
---
title: needs work valid?
---
flowchart LR
%% vertices
needs_work_valid([needs work\n valid?])
true([true])
false([false])
latest_review_request_changes{latest proper\n review requests\n
changes?}
%% edges
needs_work_valid --> latest_review_request_changes
latest_review_request_changes -- yes ---> true
latest_review_request_changes -- no ---> false
```
```mermaid
---
title: positive review valid?
---
flowchart LR
%% vertices
positive_review_valid([positive review\n valid?])
true([true])
false([false])
latest_proper_review_approved{latest proper\n review\n approved?}
%% edges
positive_review_valid --> latest_proper_review_approved
latest_proper_review_approved -- yes ---> true
latest_proper_review_approved -- no ---> false
```
Here, proper means that the review is more than a comment.
##### What happens when adding s: needs work to a PR?
```mermaid
---
title: add the needs work label
---
flowchart LR
%% vertices
trigger([label\n 's: needs work'\n added])
request_changes([request changes])
remove_other_labels([remove other\n state labels])
warning_about_label_addition([warning\n about label\n addition])
needs_work_valid[[needs work\n valid?]]
is_draft{is PR\n a draft?}
%% edges
trigger --> needs_work_valid
needs_work_valid -- true ---> remove_other_labels
needs_work_valid -- false ---> is_draft
is_draft -- yes ---> warning_about_label_addition
is_draft -- no ---> request_changes
request_changes --> remove_other_labels
```
##### What happens when adding `s: positive review` to a PR?
```mermaid
---
title: add the positive review label
---
flowchart LR
%% vertices
trigger([label\n 's: positive review'\n added])
positive_review_valid[[positive review\n valid?]]
approve_pr([approve])
remove_other_labels([remove other\n state labels])
actor_valid[[actor valid?]]
approve_allowed[[approve\n allowed?]]
warning_about_label_addition([warning\n about label\n addition])
reject_label_addition[[reject\n label\n addition]]
%% edges
trigger --> positive_review_valid
positive_review_valid -- yes ---> remove_other_labels
positive_review_valid -- no ---> actor_valid
actor_valid -- yes ---> approve_allowed
actor_valid -- no ---> reject_label_addition
approve_allowed -- yes ---> approve_pr
approve_allowed -- no ---> warning_about_label_addition
approve_pr --> remove_other_labels
```
The boxes `actor_valid` and `reject_label_addition` are unchanged.
```mermaid
---
title: approve allowed?
---
flowchart LR
%% vertices
trigger([approve\n allowed?])
true([true])
false([false])
review_of_others_request_changes{changes\n requested by\n someone\n
else exists?}
%% edges
trigger --> review_of_others_request_changes
review_of_others_request_changes -- yes ---> false
review_of_others_request_changes -- no ---> true
```
Here, only reviews of someone else are considered which are more recent
than any commit.
##### What happens when a state label is added to an issue?
```mermaid
---
title: add a state label to an issue
---
flowchart LR
%% vertices
trigger([label added])
warning_about_label_addition([warning\n about label\n addition])
nothing([do nothing])
is_needs_info{is label\n 's: needs info'?}
%% edges
trigger --> is_needs_info
is_needs_info -- yes ---> nothing
is_needs_info -- no ---> warning_about_label_addition
```
##### What happens when removing a state label from a PR?
```mermaid
---
title: remove a state label
---
flowchart LR
%% vertices
trigger([label removed])
nothing([do nothing])
hint_about_label_removal([hint\n about label\n removal])
other_state_label{other state\n labels exist?}
is_needs_info{is label\n 's: needs info'?}
%% edges
trigger --> other_state_label
other_state_label -- yes ---> nothing
other_state_label -- no ---> is_needs_info
is_needs_info -- yes ---> nothing
is_needs_info -- no ---> hint_about_label_removal
```
The hint is postet as a comment which will be deleted after 5 minutes.
##### What happens when adding a priority label?
```mermaid
---
title: add a priority label
---
flowchart LR
%% vertices
trigger([label added])
remove_other_labels([remove other\n priority labels])
%% edges
trigger --> remove_other_labels
```
##### What happens when removing a priority label?
```mermaid
---
title: remove a priority label
---
flowchart LR
%% vertices
trigger([label removed])
nothing([do nothing])
hint_about_label_removal([hint\n about label\n removal])
other_prio_label{other priority\n labels exist?}
%% edges
trigger --> other_prio_label
other_prio_label -- yes ---> nothing
other_prio_label -- no ---> hint_about_label_removal
```
##### What happens when a PR is approved?
```mermaid
---
title: approve
---
flowchart LR
%% vertices
trigger([approve\n])
select_positive_review(['s: positive review'\n label selected])
nothing([do nothing])
positive_review_valid[[positive review\n valid?]]
pending_review_requests{pending\n review requests\n exists?}
actor_authorized{is actor\n a member of\n Triage?}
%% edges
trigger ---> pending_review_requests
pending_review_requests -- yes ---> nothing
pending_review_requests -- no ---> actor_authorized
actor_authorized -- yes ---> positive_review_valid
actor_authorized -- no ---> nothing
positive_review_valid -- true ---> select_positive_review
positive_review_valid -- false ---> nothing
```
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->
- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36292
Reported by: Sebastian Oehms
Reviewer(s): Kwankyu Lee
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Oct 28, 2023
…tion
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
Unfortunately the fix in sagemath#36213 has
not been completed properly. There is still a `return` from the
`get_review_decision` method which is not `None` where it should be.
This is fixed here. Sorry for this unnecessary trouble and my lack of
attention!
In a next step this PR fixes a bug which has been noticed by an
accidentally activation of the `unlabeled` trigger (see
sagemath#36292 (comment)).
Simultanously the `labeled`-trigger has been disabled. This is an
unrealistic setting which was not covered by the code.
Explicitely, a state label could not be removed even though other state
labels where set (examples sagemath#36128
and sagemath#36020). This PR leads to the
following changes of behavior:
1. The reaction on the `unlabeled`-event is reduced to the case of the
last label
2. The `unlabeled`-event will not lead to a rejection of the removal any
more
3. Instead of a warning comment a hint comment is posted.
We found a way to test the bot a little more realistically (see
soehms#11). Additional problems emerged and
were also resolved. This led to the following changes in the workflow:
4. A bug in `actor_valid` is fixed (see
soehms#10 (comment))
5. Preparing to use our own bot user (e.g.
[sagemathadmins](https://github.com/sagemathadmins)) (see
soehms#11 (comment))
6. Waiver of observing the `reviewDecision` feature provided by GitHub
(see 2. in
soehms#11 (comment))
7. Allow the user to revert his decision of label selection (see
soehms#11 (comment))
8. Don't reject label addition any more, except in the case where the
author tries to set `s: positive review` to his own PR which has no
reviews from others (see
soehms#11 (comment))
9. Dismiss stale reviews of the bot after a push to the branch and on
submission of a new review which is more than just a comment (see
soehms#11 (comment)).
Despite the fact that the testing was now more realistic, it is still
not guaranteed that the bot's behavior in `sagemath/sage` will be
completely covered by our testing.
The changes in this PR cannot be activated immediately after merging
into the develop branch due to a bug in the GitHub web interface
observed during testing in soehms#11 (see
soehms#11 (comment) and
soehms#11 (comment)). The
problem is that the panel at the top right of the webpage that contains
the labels does not update immediately after the bot changes the labels.
Since this might cause confusion, I'll create a bug report about it. The
going-live of the sync bot will be stalled at least until we received an
answer.
This PR changes the following flowcharts from
sagemath#35172 :
##### What happens when adding `s: needs review` to a PR?
```mermaid
---
title: add the needs review label
---
flowchart LR
%% vertices
trigger([label\n 's: needs review'\n added])
mark_as_ready([mark as\n ready for\n review])
remove_other_labels([remove other\n state labels])
warning_about_label_addition([warning\n about label\n addition])
needs_review_valid[[needs review\n valid?]]
is_draft{is PR\n a draft?}
%% edges
trigger --> needs_review_valid
needs_review_valid -- true ---> remove_other_labels
needs_review_valid -- false ---> is_draft
is_draft -- yes ---> mark_as_ready
is_draft -- no ---> warning_about_label_addition
mark_as_ready --> remove_other_labels
```
The warning is posted as a comment which will be deleted after 5
minutes.
```mermaid
---
title: needs review valid?
---
flowchart LR
%% vertices
needs_review_valid([needs review\n valid?])
true([true])
false([false])
latest_review_by_actor{latest review\n by actor}
needs_work_valid[[needs work\n valid?]]
positive_review_valid[[positive review\n valid?]]
is_draft{is PR\n a draft?}
%% edges
needs_review_valid ---> is_draft
is_draft -- yes ---> false
is_draft -- no ---> latest_review_by_actor
latest_review_by_actor -- yes ---> true
latest_review_by_actor -- no ---> needs_work_valid
needs_work_valid -- true ---> false
needs_work_valid -- false ---> positive_review_valid
positive_review_valid -- true ---> false
positive_review_valid -- false ---> true
```
```mermaid
---
title: needs work valid?
---
flowchart LR
%% vertices
needs_work_valid([needs work\n valid?])
true([true])
false([false])
latest_review_request_changes{latest proper\n review requests\n
changes?}
%% edges
needs_work_valid --> latest_review_request_changes
latest_review_request_changes -- yes ---> true
latest_review_request_changes -- no ---> false
```
```mermaid
---
title: positive review valid?
---
flowchart LR
%% vertices
positive_review_valid([positive review\n valid?])
true([true])
false([false])
latest_proper_review_approved{latest proper\n review\n approved?}
%% edges
positive_review_valid --> latest_proper_review_approved
latest_proper_review_approved -- yes ---> true
latest_proper_review_approved -- no ---> false
```
Here, proper means that the review is more than a comment.
##### What happens when adding s: needs work to a PR?
```mermaid
---
title: add the needs work label
---
flowchart LR
%% vertices
trigger([label\n 's: needs work'\n added])
request_changes([request changes])
remove_other_labels([remove other\n state labels])
warning_about_label_addition([warning\n about label\n addition])
needs_work_valid[[needs work\n valid?]]
is_draft{is PR\n a draft?}
%% edges
trigger --> needs_work_valid
needs_work_valid -- true ---> remove_other_labels
needs_work_valid -- false ---> is_draft
is_draft -- yes ---> warning_about_label_addition
is_draft -- no ---> request_changes
request_changes --> remove_other_labels
```
##### What happens when adding `s: positive review` to a PR?
```mermaid
---
title: add the positive review label
---
flowchart LR
%% vertices
trigger([label\n 's: positive review'\n added])
positive_review_valid[[positive review\n valid?]]
approve_pr([approve])
remove_other_labels([remove other\n state labels])
actor_valid[[actor valid?]]
approve_allowed[[approve\n allowed?]]
warning_about_label_addition([warning\n about label\n addition])
reject_label_addition[[reject\n label\n addition]]
%% edges
trigger --> positive_review_valid
positive_review_valid -- yes ---> remove_other_labels
positive_review_valid -- no ---> actor_valid
actor_valid -- yes ---> approve_allowed
actor_valid -- no ---> reject_label_addition
approve_allowed -- yes ---> approve_pr
approve_allowed -- no ---> warning_about_label_addition
approve_pr --> remove_other_labels
```
The boxes `actor_valid` and `reject_label_addition` are unchanged.
```mermaid
---
title: approve allowed?
---
flowchart LR
%% vertices
trigger([approve\n allowed?])
true([true])
false([false])
review_of_others_request_changes{changes\n requested by\n someone\n
else exists?}
%% edges
trigger --> review_of_others_request_changes
review_of_others_request_changes -- yes ---> false
review_of_others_request_changes -- no ---> true
```
Here, only reviews of someone else are considered which are more recent
than any commit.
##### What happens when a state label is added to an issue?
```mermaid
---
title: add a state label to an issue
---
flowchart LR
%% vertices
trigger([label added])
warning_about_label_addition([warning\n about label\n addition])
nothing([do nothing])
is_needs_info{is label\n 's: needs info'?}
%% edges
trigger --> is_needs_info
is_needs_info -- yes ---> nothing
is_needs_info -- no ---> warning_about_label_addition
```
##### What happens when removing a state label from a PR?
```mermaid
---
title: remove a state label
---
flowchart LR
%% vertices
trigger([label removed])
nothing([do nothing])
hint_about_label_removal([hint\n about label\n removal])
other_state_label{other state\n labels exist?}
is_needs_info{is label\n 's: needs info'?}
%% edges
trigger --> other_state_label
other_state_label -- yes ---> nothing
other_state_label -- no ---> is_needs_info
is_needs_info -- yes ---> nothing
is_needs_info -- no ---> hint_about_label_removal
```
The hint is postet as a comment which will be deleted after 5 minutes.
##### What happens when adding a priority label?
```mermaid
---
title: add a priority label
---
flowchart LR
%% vertices
trigger([label added])
remove_other_labels([remove other\n priority labels])
%% edges
trigger --> remove_other_labels
```
##### What happens when removing a priority label?
```mermaid
---
title: remove a priority label
---
flowchart LR
%% vertices
trigger([label removed])
nothing([do nothing])
hint_about_label_removal([hint\n about label\n removal])
other_prio_label{other priority\n labels exist?}
%% edges
trigger --> other_prio_label
other_prio_label -- yes ---> nothing
other_prio_label -- no ---> hint_about_label_removal
```
##### What happens when a PR is approved?
```mermaid
---
title: approve
---
flowchart LR
%% vertices
trigger([approve\n])
select_positive_review(['s: positive review'\n label selected])
nothing([do nothing])
positive_review_valid[[positive review\n valid?]]
pending_review_requests{pending\n review requests\n exists?}
actor_authorized{is actor\n a member of\n Triage?}
%% edges
trigger ---> pending_review_requests
pending_review_requests -- yes ---> nothing
pending_review_requests -- no ---> actor_authorized
actor_authorized -- yes ---> positive_review_valid
actor_authorized -- no ---> nothing
positive_review_valid -- true ---> select_positive_review
positive_review_valid -- false ---> nothing
```
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->
- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36292
Reported by: Sebastian Oehms
Reviewer(s): Kwankyu Lee
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Oct 29, 2023
…tion
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
Unfortunately the fix in sagemath#36213 has
not been completed properly. There is still a `return` from the
`get_review_decision` method which is not `None` where it should be.
This is fixed here. Sorry for this unnecessary trouble and my lack of
attention!
In a next step this PR fixes a bug which has been noticed by an
accidentally activation of the `unlabeled` trigger (see
sagemath#36292 (comment)).
Simultanously the `labeled`-trigger has been disabled. This is an
unrealistic setting which was not covered by the code.
Explicitely, a state label could not be removed even though other state
labels where set (examples sagemath#36128
and sagemath#36020). This PR leads to the
following changes of behavior:
1. The reaction on the `unlabeled`-event is reduced to the case of the
last label
2. The `unlabeled`-event will not lead to a rejection of the removal any
more
3. Instead of a warning comment a hint comment is posted.
We found a way to test the bot a little more realistically (see
soehms#11). Additional problems emerged and
were also resolved. This led to the following changes in the workflow:
4. A bug in `actor_valid` is fixed (see
soehms#10 (comment))
5. Preparing to use our own bot user (e.g.
[sagemathadmins](https://github.com/sagemathadmins)) (see
soehms#11 (comment))
6. Waiver of observing the `reviewDecision` feature provided by GitHub
(see 2. in
soehms#11 (comment))
7. Allow the user to revert his decision of label selection (see
soehms#11 (comment))
8. Don't reject label addition any more, except in the case where the
author tries to set `s: positive review` to his own PR which has no
reviews from others (see
soehms#11 (comment))
9. Dismiss stale reviews of the bot after a push to the branch and on
submission of a new review which is more than just a comment (see
soehms#11 (comment)).
Despite the fact that the testing was now more realistic, it is still
not guaranteed that the bot's behavior in `sagemath/sage` will be
completely covered by our testing.
The changes in this PR cannot be activated immediately after merging
into the develop branch due to a bug in the GitHub web interface
observed during testing in soehms#11 (see
soehms#11 (comment) and
soehms#11 (comment)). The
problem is that the panel at the top right of the webpage that contains
the labels does not update immediately after the bot changes the labels.
Since this might cause confusion, I'll create a bug report about it. The
going-live of the sync bot will be stalled at least until we received an
answer.
This PR changes the following flowcharts from
sagemath#35172 :
##### What happens when adding `s: needs review` to a PR?
```mermaid
---
title: add the needs review label
---
flowchart LR
%% vertices
trigger([label\n 's: needs review'\n added])
mark_as_ready([mark as\n ready for\n review])
remove_other_labels([remove other\n state labels])
warning_about_label_addition([warning\n about label\n addition])
needs_review_valid[[needs review\n valid?]]
is_draft{is PR\n a draft?}
%% edges
trigger --> needs_review_valid
needs_review_valid -- true ---> remove_other_labels
needs_review_valid -- false ---> is_draft
is_draft -- yes ---> mark_as_ready
is_draft -- no ---> warning_about_label_addition
mark_as_ready --> remove_other_labels
```
The warning is posted as a comment which will be deleted after 5
minutes.
```mermaid
---
title: needs review valid?
---
flowchart LR
%% vertices
needs_review_valid([needs review\n valid?])
true([true])
false([false])
latest_review_by_actor{latest review\n by actor}
needs_work_valid[[needs work\n valid?]]
positive_review_valid[[positive review\n valid?]]
is_draft{is PR\n a draft?}
%% edges
needs_review_valid ---> is_draft
is_draft -- yes ---> false
is_draft -- no ---> latest_review_by_actor
latest_review_by_actor -- yes ---> true
latest_review_by_actor -- no ---> needs_work_valid
needs_work_valid -- true ---> false
needs_work_valid -- false ---> positive_review_valid
positive_review_valid -- true ---> false
positive_review_valid -- false ---> true
```
```mermaid
---
title: needs work valid?
---
flowchart LR
%% vertices
needs_work_valid([needs work\n valid?])
true([true])
false([false])
latest_review_request_changes{latest proper\n review requests\n
changes?}
%% edges
needs_work_valid --> latest_review_request_changes
latest_review_request_changes -- yes ---> true
latest_review_request_changes -- no ---> false
```
```mermaid
---
title: positive review valid?
---
flowchart LR
%% vertices
positive_review_valid([positive review\n valid?])
true([true])
false([false])
latest_proper_review_approved{latest proper\n review\n approved?}
%% edges
positive_review_valid --> latest_proper_review_approved
latest_proper_review_approved -- yes ---> true
latest_proper_review_approved -- no ---> false
```
Here, proper means that the review is more than a comment.
##### What happens when adding s: needs work to a PR?
```mermaid
---
title: add the needs work label
---
flowchart LR
%% vertices
trigger([label\n 's: needs work'\n added])
request_changes([request changes])
remove_other_labels([remove other\n state labels])
warning_about_label_addition([warning\n about label\n addition])
needs_work_valid[[needs work\n valid?]]
is_draft{is PR\n a draft?}
%% edges
trigger --> needs_work_valid
needs_work_valid -- true ---> remove_other_labels
needs_work_valid -- false ---> is_draft
is_draft -- yes ---> warning_about_label_addition
is_draft -- no ---> request_changes
request_changes --> remove_other_labels
```
##### What happens when adding `s: positive review` to a PR?
```mermaid
---
title: add the positive review label
---
flowchart LR
%% vertices
trigger([label\n 's: positive review'\n added])
positive_review_valid[[positive review\n valid?]]
approve_pr([approve])
remove_other_labels([remove other\n state labels])
actor_valid[[actor valid?]]
approve_allowed[[approve\n allowed?]]
warning_about_label_addition([warning\n about label\n addition])
reject_label_addition[[reject\n label\n addition]]
%% edges
trigger --> positive_review_valid
positive_review_valid -- yes ---> remove_other_labels
positive_review_valid -- no ---> actor_valid
actor_valid -- yes ---> approve_allowed
actor_valid -- no ---> reject_label_addition
approve_allowed -- yes ---> approve_pr
approve_allowed -- no ---> warning_about_label_addition
approve_pr --> remove_other_labels
```
The boxes `actor_valid` and `reject_label_addition` are unchanged.
```mermaid
---
title: approve allowed?
---
flowchart LR
%% vertices
trigger([approve\n allowed?])
true([true])
false([false])
review_of_others_request_changes{changes\n requested by\n someone\n
else exists?}
%% edges
trigger --> review_of_others_request_changes
review_of_others_request_changes -- yes ---> false
review_of_others_request_changes -- no ---> true
```
Here, only reviews of someone else are considered which are more recent
than any commit.
##### What happens when a state label is added to an issue?
```mermaid
---
title: add a state label to an issue
---
flowchart LR
%% vertices
trigger([label added])
warning_about_label_addition([warning\n about label\n addition])
nothing([do nothing])
is_needs_info{is label\n 's: needs info'?}
%% edges
trigger --> is_needs_info
is_needs_info -- yes ---> nothing
is_needs_info -- no ---> warning_about_label_addition
```
##### What happens when removing a state label from a PR?
```mermaid
---
title: remove a state label
---
flowchart LR
%% vertices
trigger([label removed])
nothing([do nothing])
hint_about_label_removal([hint\n about label\n removal])
other_state_label{other state\n labels exist?}
is_needs_info{is label\n 's: needs info'?}
%% edges
trigger --> other_state_label
other_state_label -- yes ---> nothing
other_state_label -- no ---> is_needs_info
is_needs_info -- yes ---> nothing
is_needs_info -- no ---> hint_about_label_removal
```
The hint is postet as a comment which will be deleted after 5 minutes.
##### What happens when adding a priority label?
```mermaid
---
title: add a priority label
---
flowchart LR
%% vertices
trigger([label added])
remove_other_labels([remove other\n priority labels])
%% edges
trigger --> remove_other_labels
```
##### What happens when removing a priority label?
```mermaid
---
title: remove a priority label
---
flowchart LR
%% vertices
trigger([label removed])
nothing([do nothing])
hint_about_label_removal([hint\n about label\n removal])
other_prio_label{other priority\n labels exist?}
%% edges
trigger --> other_prio_label
other_prio_label -- yes ---> nothing
other_prio_label -- no ---> hint_about_label_removal
```
##### What happens when a PR is approved?
```mermaid
---
title: approve
---
flowchart LR
%% vertices
trigger([approve\n])
select_positive_review(['s: positive review'\n label selected])
nothing([do nothing])
positive_review_valid[[positive review\n valid?]]
pending_review_requests{pending\n review requests\n exists?}
actor_authorized{is actor\n a member of\n Triage?}
%% edges
trigger ---> pending_review_requests
pending_review_requests -- yes ---> nothing
pending_review_requests -- no ---> actor_authorized
actor_authorized -- yes ---> positive_review_valid
actor_authorized -- no ---> nothing
positive_review_valid -- true ---> select_positive_review
positive_review_valid -- false ---> nothing
```
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->
- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36292
Reported by: Sebastian Oehms
Reviewer(s): Kwankyu Lee
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Oct 29, 2023
…tion
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
Unfortunately the fix in sagemath#36213 has
not been completed properly. There is still a `return` from the
`get_review_decision` method which is not `None` where it should be.
This is fixed here. Sorry for this unnecessary trouble and my lack of
attention!
In a next step this PR fixes a bug which has been noticed by an
accidentally activation of the `unlabeled` trigger (see
sagemath#36292 (comment)).
Simultanously the `labeled`-trigger has been disabled. This is an
unrealistic setting which was not covered by the code.
Explicitely, a state label could not be removed even though other state
labels where set (examples sagemath#36128
and sagemath#36020). This PR leads to the
following changes of behavior:
1. The reaction on the `unlabeled`-event is reduced to the case of the
last label
2. The `unlabeled`-event will not lead to a rejection of the removal any
more
3. Instead of a warning comment a hint comment is posted.
We found a way to test the bot a little more realistically (see
soehms#11). Additional problems emerged and
were also resolved. This led to the following changes in the workflow:
4. A bug in `actor_valid` is fixed (see
soehms#10 (comment))
5. Preparing to use our own bot user (e.g.
[sagemathadmins](https://github.com/sagemathadmins)) (see
soehms#11 (comment))
6. Waiver of observing the `reviewDecision` feature provided by GitHub
(see 2. in
soehms#11 (comment))
7. Allow the user to revert his decision of label selection (see
soehms#11 (comment))
8. Don't reject label addition any more, except in the case where the
author tries to set `s: positive review` to his own PR which has no
reviews from others (see
soehms#11 (comment))
9. Dismiss stale reviews of the bot after a push to the branch and on
submission of a new review which is more than just a comment (see
soehms#11 (comment)).
Despite the fact that the testing was now more realistic, it is still
not guaranteed that the bot's behavior in `sagemath/sage` will be
completely covered by our testing.
The changes in this PR cannot be activated immediately after merging
into the develop branch due to a bug in the GitHub web interface
observed during testing in soehms#11 (see
soehms#11 (comment) and
soehms#11 (comment)). The
problem is that the panel at the top right of the webpage that contains
the labels does not update immediately after the bot changes the labels.
Since this might cause confusion, I'll create a bug report about it. The
going-live of the sync bot will be stalled at least until we received an
answer.
This PR changes the following flowcharts from
sagemath#35172 :
##### What happens when adding `s: needs review` to a PR?
```mermaid
---
title: add the needs review label
---
flowchart LR
%% vertices
trigger([label\n 's: needs review'\n added])
mark_as_ready([mark as\n ready for\n review])
remove_other_labels([remove other\n state labels])
warning_about_label_addition([warning\n about label\n addition])
needs_review_valid[[needs review\n valid?]]
is_draft{is PR\n a draft?}
%% edges
trigger --> needs_review_valid
needs_review_valid -- true ---> remove_other_labels
needs_review_valid -- false ---> is_draft
is_draft -- yes ---> mark_as_ready
is_draft -- no ---> warning_about_label_addition
mark_as_ready --> remove_other_labels
```
The warning is posted as a comment which will be deleted after 5
minutes.
```mermaid
---
title: needs review valid?
---
flowchart LR
%% vertices
needs_review_valid([needs review\n valid?])
true([true])
false([false])
latest_review_by_actor{latest review\n by actor}
needs_work_valid[[needs work\n valid?]]
positive_review_valid[[positive review\n valid?]]
is_draft{is PR\n a draft?}
%% edges
needs_review_valid ---> is_draft
is_draft -- yes ---> false
is_draft -- no ---> latest_review_by_actor
latest_review_by_actor -- yes ---> true
latest_review_by_actor -- no ---> needs_work_valid
needs_work_valid -- true ---> false
needs_work_valid -- false ---> positive_review_valid
positive_review_valid -- true ---> false
positive_review_valid -- false ---> true
```
```mermaid
---
title: needs work valid?
---
flowchart LR
%% vertices
needs_work_valid([needs work\n valid?])
true([true])
false([false])
latest_review_request_changes{latest proper\n review requests\n
changes?}
%% edges
needs_work_valid --> latest_review_request_changes
latest_review_request_changes -- yes ---> true
latest_review_request_changes -- no ---> false
```
```mermaid
---
title: positive review valid?
---
flowchart LR
%% vertices
positive_review_valid([positive review\n valid?])
true([true])
false([false])
latest_proper_review_approved{latest proper\n review\n approved?}
%% edges
positive_review_valid --> latest_proper_review_approved
latest_proper_review_approved -- yes ---> true
latest_proper_review_approved -- no ---> false
```
Here, proper means that the review is more than a comment.
##### What happens when adding s: needs work to a PR?
```mermaid
---
title: add the needs work label
---
flowchart LR
%% vertices
trigger([label\n 's: needs work'\n added])
request_changes([request changes])
remove_other_labels([remove other\n state labels])
warning_about_label_addition([warning\n about label\n addition])
needs_work_valid[[needs work\n valid?]]
is_draft{is PR\n a draft?}
%% edges
trigger --> needs_work_valid
needs_work_valid -- true ---> remove_other_labels
needs_work_valid -- false ---> is_draft
is_draft -- yes ---> warning_about_label_addition
is_draft -- no ---> request_changes
request_changes --> remove_other_labels
```
##### What happens when adding `s: positive review` to a PR?
```mermaid
---
title: add the positive review label
---
flowchart LR
%% vertices
trigger([label\n 's: positive review'\n added])
positive_review_valid[[positive review\n valid?]]
approve_pr([approve])
remove_other_labels([remove other\n state labels])
actor_valid[[actor valid?]]
approve_allowed[[approve\n allowed?]]
warning_about_label_addition([warning\n about label\n addition])
reject_label_addition[[reject\n label\n addition]]
%% edges
trigger --> positive_review_valid
positive_review_valid -- yes ---> remove_other_labels
positive_review_valid -- no ---> actor_valid
actor_valid -- yes ---> approve_allowed
actor_valid -- no ---> reject_label_addition
approve_allowed -- yes ---> approve_pr
approve_allowed -- no ---> warning_about_label_addition
approve_pr --> remove_other_labels
```
The boxes `actor_valid` and `reject_label_addition` are unchanged.
```mermaid
---
title: approve allowed?
---
flowchart LR
%% vertices
trigger([approve\n allowed?])
true([true])
false([false])
review_of_others_request_changes{changes\n requested by\n someone\n
else exists?}
%% edges
trigger --> review_of_others_request_changes
review_of_others_request_changes -- yes ---> false
review_of_others_request_changes -- no ---> true
```
Here, only reviews of someone else are considered which are more recent
than any commit.
##### What happens when a state label is added to an issue?
```mermaid
---
title: add a state label to an issue
---
flowchart LR
%% vertices
trigger([label added])
warning_about_label_addition([warning\n about label\n addition])
nothing([do nothing])
is_needs_info{is label\n 's: needs info'?}
%% edges
trigger --> is_needs_info
is_needs_info -- yes ---> nothing
is_needs_info -- no ---> warning_about_label_addition
```
##### What happens when removing a state label from a PR?
```mermaid
---
title: remove a state label
---
flowchart LR
%% vertices
trigger([label removed])
nothing([do nothing])
hint_about_label_removal([hint\n about label\n removal])
other_state_label{other state\n labels exist?}
is_needs_info{is label\n 's: needs info'?}
%% edges
trigger --> other_state_label
other_state_label -- yes ---> nothing
other_state_label -- no ---> is_needs_info
is_needs_info -- yes ---> nothing
is_needs_info -- no ---> hint_about_label_removal
```
The hint is postet as a comment which will be deleted after 5 minutes.
##### What happens when adding a priority label?
```mermaid
---
title: add a priority label
---
flowchart LR
%% vertices
trigger([label added])
remove_other_labels([remove other\n priority labels])
%% edges
trigger --> remove_other_labels
```
##### What happens when removing a priority label?
```mermaid
---
title: remove a priority label
---
flowchart LR
%% vertices
trigger([label removed])
nothing([do nothing])
hint_about_label_removal([hint\n about label\n removal])
other_prio_label{other priority\n labels exist?}
%% edges
trigger --> other_prio_label
other_prio_label -- yes ---> nothing
other_prio_label -- no ---> hint_about_label_removal
```
##### What happens when a PR is approved?
```mermaid
---
title: approve
---
flowchart LR
%% vertices
trigger([approve\n])
select_positive_review(['s: positive review'\n label selected])
nothing([do nothing])
positive_review_valid[[positive review\n valid?]]
pending_review_requests{pending\n review requests\n exists?}
actor_authorized{is actor\n a member of\n Triage?}
%% edges
trigger ---> pending_review_requests
pending_review_requests -- yes ---> nothing
pending_review_requests -- no ---> actor_authorized
actor_authorized -- yes ---> positive_review_valid
actor_authorized -- no ---> nothing
positive_review_valid -- true ---> select_positive_review
positive_review_valid -- false ---> nothing
```
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->
- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36292
Reported by: Sebastian Oehms
Reviewer(s): Kwankyu Lee
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug which has been noticed by an accidentally activation of the
unlabeledtrigger (see sagemath#36292 (comment)). Simultanously thelabeled-trigger has been disabled. This is an unrealistic setting which is not covered by the code.Explicitely, a state label could not be removed even though other state labels where set (examples sagemath#36128 and sagemath#36020). This PR leads to the following changes of behavior:
unlabeled-event is reduced to the case of the last labelunlabeled-event will not lead to a rejection of the removal any moreFurthermore, a bug in
actor_validis fixed, which showed up while testing and some minor typo fixes noticed by the review are done.📝 Checklist
⌛ Dependencies