-
Notifications
You must be signed in to change notification settings - Fork 132
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
2037 add review date #2044
2037 add review date #2044
Conversation
cf3c965
to
7611263
Compare
status = review.status | ||
comment = review.get_note_text() | ||
review_date = review.date_created | ||
return status, comment, review_date | ||
except SubmissionReview.DoesNotExist: | ||
return None |
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.
Seems the None portion is not being tested, it could be a source of potential bug, I would expect 3 None's to be returned.
def get_review_status_and_comment(self): | ||
def get_review_details(self): |
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 tend to be hesitant in renaming functions that have been around for a long time, in this case about 3 years. I would perhaps recommend a new method:
def get_latest_review(self):
try:
return self.reviews.latest('date_modified')
except SubmissionReview.DoesNotExist:
return None
Which then we can use as
review = self.get_latest_review()
if review:
doc[REVIEW_STATUS] = review.status
doc[REVIEW_COMMENT] = review.get_note_text() or ""
doc[REVIEW_DATE] = review.date_created.strftime(MONGO_STRFTIME)
I think this approach is much cleaner and clearer.
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 also means you do not end up having to rename old functions, you could mark the old function deprecated so that we eventually change its reference everywhere.
c209646
to
782c31d
Compare
Return a tuple of review status and comment | ||
Return a tuple of review status and comment. | ||
Deprecated in favour of `get_latest_review` | ||
TODO: Clean code of this unused function. |
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 wonder if we should use something like the Deprecated library or a custom decorator like this; to mark deprecated functions ?
TODOs usually get forgotten IMO
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 for this.
I've used the pip deprecated package for this. Seemed cleaner.
451f818
to
8b5af51
Compare
e213203
to
fc0c9dc
Compare
Use deprecated pip package to deprecate `get_review_status_and_comment` function
fc0c9dc
to
db37f0d
Compare
Changes / Features implemented
Adds submission review dates to data endpoint response
Steps taken to verify this change does what is intended
Updated tests
QA
Closes #2037