-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: Add PeerReviewer model for improved peer review support #748
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.
Thanks Karthik, all of the basic functionality seems to be working. See the inline comments for remaining stuff
django/library/migrations/0030_alter_peerreviewer_subject_areas.py
Outdated
Show resolved
Hide resolved
66b67b1
to
b20b9a8
Compare
59bc08d
to
efa73a8
Compare
efa73a8
to
0fa16a9
Compare
Co-authored-by: Karthik Bandagonda <Karthik99999@users.noreply.github.com>
Co-authored-by: sgfost <sgfost@users.noreply.github.com>
- use get_or_create to update a reviewer when trying to create while one already exists - allow users to create or update their own reviewer record. this allows anyone to insert themselves into the reviewer pool at any time, should consider whether this is desirable or we should add a pending status Co-authored-by: Karthik Bandagonda <Karthik99999@users.noreply.github.com>
This prevents the reviewer object on the parent component from being mutated before the form is submitted
0fa16a9
to
2aa9678
Compare
2aa9678
to
fd51071
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.
Nice job, everything seems to work well, besides a small issue with the reviewers data migration. I have a comment below about this along with a few other things
const { updateReviewer: update } = useReviewEditorAPI(); | ||
|
||
async function changeActiveState(reviewer: Reviewer, isActive: boolean) { | ||
// FIXME: Make server accept partial reviewer object without defining memberProfileId |
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.
does this still need to happen?
687b514
to
eec026b
Compare
e3ef06d
to
036afdd
Compare
permission class was not set up properly, now correctly drops through to has_object_permission for retrieve, update, etc. * add a profile link to reviewer card
The pagination issue and not being able to retrieve/update your own reviewer record on an unprivileged account are fixed and on staging |
provide string representation of PeerReviewer
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.
Thanks Karthik & Scott! LGTM with some minor comments
I'll fix these up to get it moving. There's also a small oversight in the browser tests with dates which is causing that to fail today |
thanks @sgfost ! I was wondering about the e2e tests referring to dates with scalar integers, comses.net/e2e/cypress/fixtures/data.json Line 45 in ff5c42d
|
the least fragile way I can think of doing it (i.e. not relying on this specific datepicker too much) is to keep the scalar day-of-the-month input but always using the following month. It'd probably be ideal if there was a way to input a string date with the keyboard but I'll just fix the test for now rather than hack at the datepicker |
* order peer review invitations by date sent (most recent first) * fix an issue with invalid dates in the e2e tests
see comses/planning#256
This PR currently does not include the functionality of users being able to request to become peer reviewers.