-
Notifications
You must be signed in to change notification settings - Fork 2
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
Approve responder #27
Conversation
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.
# Delete current assignment | ||
reviewer_entry = airtable_revs.all(filter: "{github} = '#{reviewer}'").first | ||
if reviewer_entry | ||
reviewer_entry["current_assignment"] = "" |
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.
Just thinking this might create a problem if someone has two current assignments but this should not happen, though. (i.e. too rare of an edge case to handle)
if submission_type == "stats" | ||
statsgrade = read_value_from_body("statsgrade").downcase | ||
if ["bronze", "silver", "gold"].include?(statsgrade) | ||
labels_to_add << "6/approved-#{statsgrade}" |
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.
This bit will need a slight adjustment, which I am unsure how best to implement. The badges are graded, and the labels will be like 6/approved-bronze-v0.0.1
. At the time the command is issued, the version will be the current version which can be obtained from a fairly straightforward URL query. Alternatively, it could easily be incorporated within an actual external service and delivered that way. @xuanxu Let me know if that would be best, and I'll implement once I'm back from vacation after 16th Aug. And then, to complicate matters a bit further, we'll later have some kind of "downgrade" command which will change the grade (like gold
-> silver
, while retaining version on current badge and not aligning with current version if that differs. But hopefully that can be left to work on once this initial responder is up and running.
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.
Maybe instead of adding the label this responder could ping a simple external service passing the grade so the external service can just add the proper label. That would be easy and simple I think.
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.
Okay, shall do. Could you please open an issue on ropensci-review-tools/roreviewapi
and link back here? Thanks!
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.
Done! ropensci-review-tools/roreviewapi#9
I guess this can be merged and the responder updated once the external service is working, I don't see this a stopper to merge this PR, WDYT?
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.
Yeah, it'll be fine to merge because the badges with those (non-versioned) labels still don't exist, so command will just fail to add badge, which is fine for the moment.
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.
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.
@maelle This can be merged, yes. But the labeling with badge grades for the stats-type submissions won't work.
4994d9e
to
c42144c
Compare
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've modified the labeling of the stats
submissions to use the external service.
This should be ready now to merge or test-deploy.
This PR adds a new
ApproveResponder
listening to a@bot approve package-name
command.Features of the responder:
date-accepted
to the body of the issuestats
it checks if stasgrade is present and if so adds the proper labelCloses #8
Closes #10
Closes #20
Ref: #9