-
Notifications
You must be signed in to change notification settings - Fork 28
Post Decision Stage: do not change the venueid for submissions that don't have a decision posted #2596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
if venueid: | ||
return self.client.get_all_notes(content={ 'venueid': venueid}, sort=sort, details=details) | ||
|
||
venueids = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@celestemartinez this could be a breaking change
self.get_rejected_submission_venue_id() | ||
] | ||
|
||
return self.client.get_all_notes(content={ 'venueid': ','.join(venueids)}, sort=sort, details=details) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know we could do this 😮
The change looks good! One thing I noticed is that we still run the process if the decision is deleted. This might be out of the scope of this PR, but I wonder if we should do this in the case an accept decision is changed to reject or deleted:
|
I believe 2 is already happening.
|
@@ -31,8 +51,7 @@ def process(client, edit, invitation): | |||
) | |||
|
|||
if (authors_accepted_id): | |||
accept_options = domain.get_content_value('accept_decision_options') | |||
if openreview.tools.is_accept_decision(decision.content['decision']['value'], accept_options): | |||
if is_accepted_decision: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@celestemartinez this is add or remove based on the decision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, the authors are removed when the decision is changed. I believe they are not removed when the decision is deleted. Not sure how often this happens or if this is a big issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohhhh, good point, I will fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be fixed, in another PR we can update the venueid when a decision is deleted. I think we need to use invitations to edit the venueids instead of doing it with meta invitations
I tested Post Decision with different configurations, running it multiple times, etc. and it looks good. The only thing I noticed is that I guess when we run Post Decision we expire the Post Submission Stage button in the request form, so it seems the only way to change submission readers is through Post Decision. But changing readers there only affects the non-active papers. |
@enrubio good point. @celestemartinez should I fix this in this PR? Can I remove the expiration of the post submission PR? what I can do is that the post submission should only be applied to submissions that are active AND do not have a decision, would that work? |
I realized that Post_Decision_Stage has the field |
I am not sure it is worth it to fix this now. I might be wrong, but I don't think this has been an issue very often. This should also be easier in the new configuration since we won't be reading |
ok, I won't make any change, please feel free to merge it 🙏 |
Currently submissions that don't have a decision posted are being treated as rejected submission when the post decision stage runs.
This PR ignores these submissions and it doesn't change the venueid, readers not send any notification.
It creates the Camera Ready Revision invitation for a new decisions posted.