-
Notifications
You must be signed in to change notification settings - Fork 38
Review summary backend #1317
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
base: develop
Are you sure you want to change the base?
Review summary backend #1317
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.
Copilot reviewed 32 out of 33 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- webapp/public/locales/en/translation.json: Language not supported
|
|
||
| raise ValueError(errors.REVIEW_FORM_NOT_FOUND) | ||
|
|
Copilot
AI
Apr 15, 2025
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.
In the outcome POST method, raising a ValueError when review_form is not found interrupts the API flow; consider returning an appropriate error response to gracefully handle this case.
| raise ValueError(errors.REVIEW_FORM_NOT_FOUND) | |
| return errors.REVIEW_FORM_NOT_FOUND |
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
b5c242d to
111704a
Compare
| outcome_repository.add(outcome) | ||
| db.session.commit() | ||
|
|
||
| if (status == Status.REJECTED or status == Status.WAITLIST): # Email will be sent with offer for accepted candidates |
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.
In the list of code changes, it looks like this code is deleted, but it is shifted down. It might be cleaner to swap the "if" statements?
| event=event, | ||
| user=user, | ||
| ) | ||
| if (event.event_type==EventType.JOURNAL): |
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.
space around == ?
| ) | ||
|
|
||
| else: | ||
| if (status == Status.REJECTED or status == Status.WAITLIST): # Email will be sent with offer for accepted candidates |
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.
see earlier comment
api/app/outcome/models.py
Outdated
| REVIEW = "in review" | ||
| ACCEPT_W_REVISION = "accept with minor revision" | ||
| REJECT_W_ENCOURAGEMENT = "reject with encouragement to resubmit" | ||
| ACCEPTED = "Accepted" |
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 checking as "accepted" -> "Accepted" will affect UI for all of Deep Learning Indaba. Is there no major text implications, i.e. the word used in the middle of a sentence somewhere?
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.
Please undo these changes. These are primarily for the database and not for displaying in the UI. If a different text is needed on the UI then that mapping should happen there.
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!
Done!
api/app/outcome/repository.py
Outdated
| def get_latest_by_user_for_event(user_id, event_id,response_id=None): | ||
| outcome = (db.session.query(Outcome) | ||
| .filter_by(user_id=user_id, event_id=event_id, latest=True) | ||
| .filter_by(user_id=user_id, event_id=event_id, latest=True,response_id=response_id) |
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.
space after comma (style)?
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.
Clean
api/app/responses/api.py
Outdated
| responses = response_repository.get_all_for_user_application(user_id, application_form_id) | ||
|
|
||
| if not application_form.nominations and len(responses) > 0: | ||
| if not allow_multiple_submissions and len(responses) > 0: |
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.
if (double space) not?
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.
clean!
| def get_eligible_response_ids(self, event_id, reviewer_user_id, num_reviews, reviews_required, tags): | ||
| candidate_responses = review_repository.get_candidate_responses(event_id, reviewer_user_id, reviews_required) | ||
| # Filter by tags | ||
| # Filter by tags |
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.
tags space?
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.
Hi Yvan,
Thanks for the PR. It's generally looking good! Have finished reviewing the backend changes, but still busy with the front-end ones.
api/app/outcome/api.py
Outdated
|
|
||
| try: | ||
| outcome = outcome_repository.get_latest_by_user_for_event(user_id, event_id) | ||
| outcome = outcome_repository.get_latest_by_user_for_event(user_id, event_id,response_id) |
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.
General style comment (please fix everywhere): please ensure there is a space after a comma in an argument list.
api/app/outcome/api.py
Outdated
| req_parser.add_argument('user_id', type=int, required=True) | ||
| req_parser.add_argument('outcome', type=str, required=True) | ||
| req_parser.add_argument('response_id', type=int, required=True) | ||
| req_parser.add_argument('review_summary', type=str, required=True) |
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.
Having a review_summary being required for an outcome will break the Indaba process. For the Indaba, not all applications are reviewed, so there won't necessarily be a review summary. It would be better to keep review summaries associated with reviews and then have logic that picks that up for journal events when needed, instead of coupling reviews to outcomes as is done here.
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.
although i see in the outcome model it's actually not required, so it shouldn't be required here then.
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 turned it to False and I used args.get('review_summary') to prevent case when we are getting nothing
api/app/outcome/api.py
Outdated
| ) | ||
| if (event.event_type==EventType.JOURNAL): | ||
| response = response_repository.get_by_id_and_user_id(outcome.response_id, outcome.user_id) | ||
| submission_title=strings.answer_by_question_key('submission_title', response.application_form, response.answers) |
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.
Please fix spacing around = here and everywhere else
api/app/outcome/api.py
Outdated
| ) | ||
| if (event.event_type==EventType.JOURNAL): | ||
| response = response_repository.get_by_id_and_user_id(outcome.response_id, outcome.user_id) | ||
| submission_title=strings.answer_by_question_key('submission_title', response.application_form, response.answers) |
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.
Is there some logic that enforces a submission_title field in every journal application form? Otherwise this can easily break without people realising it, and probably when it's too late to fix it.
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 added this script
if not submission_title: raise errors.SUBMISSION_TITLE_NOT_FOUND
api/app/outcome/models.py
Outdated
| REVIEW = "in review" | ||
| ACCEPT_W_REVISION = "accept with minor revision" | ||
| REJECT_W_ENCOURAGEMENT = "reject with encouragement to resubmit" | ||
| ACCEPTED = "Accepted" |
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.
Please undo these changes. These are primarily for the database and not for displaying in the UI. If a different text is needed on the UI then that mapping should happen there.
|
|
||
| {this.props.t("Thank you for your submission of ")} | ||
| <strong>{submission_title}</strong> | ||
| {this.props.t(" to The Journal of Artificial Intelligence for Sustainable Development. Please find below a meta review summary, followed by the reviewer(s)’s feedback." |
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.
please replace with a placeholder for the event name rather than hardcoding this one
|
|
||
| <div className="application-status"> | ||
| <h5>{this.props.t("Meta-review")}</h5> | ||
| <p className="greeting"> |
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.
why is this className called "greeting" ?
|
|
||
| <div className="letter-footer"> | ||
| <p>{this.props.t("Kind Regards")},</p> | ||
| <p>{this.props.t("The JAISD editorial board")}</p> |
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.
Please replace with a placeholder
| </button> | ||
| </div> | ||
|
|
||
| <div className="modal-body"> |
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'm not sure what exactly this "letter" is for, but can the contents instead be loaded from an email template, rather than being hard-coded into the UI?
| renderSections() { | ||
| const applicationForm = this.state.applicationForm; | ||
| const applicationData = this.state.applicationData; | ||
| let html = []; |
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.
Please do this with a map which is more canonical react style (which favours immutable code), rather than a forEach pushing into an array. I know we have some code in the responsePage that works like this, but it shouldn't be there, and I don't want to add more code of a bad style
|
I will remove all useless css and translation |
New Patch – Baobab/Journal Updates
Response Page Enhancements:
Additional Notes: