-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Fix #19264: Fixed server exception when there are no contributions for the given time range #20261
Conversation
Allowed suggestion_services.py to return None when no contributions are found for the given date range. On the client side, the certificate-download-modal component now checks for this None value (which is stringified to "null") and displays the appropriate warning message to the user.
Assigning @chris7716 for the first pass review of this PR. 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.
Hey @mgporter Thanks for the PR! I have a couple of comments.
@@ -4001,8 +4001,7 @@ def _generate_translation_contributor_certificate_data( | |||
hours_contributed = round(words_count / 300, 2) | |||
|
|||
if words_count == 0: | |||
raise Exception( | |||
'There are no contributions for the given time range.') | |||
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.
Could you please update the backend tests for this change. I think backend tests in the CI are failing due to this.
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.
Got it! suggestion_services.py has had its tests updated.
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 @chris7716, would you mind taking a look at this again?
this.createCertificate(response); | ||
if (response) { | ||
this.createCertificate(response); | ||
} else { |
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 believe you will have to add frontend tests for this as well.
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.
Frontend tests have been added! Thank you for the feedback!
Thanks for the feedback! I'll try to do this right. Tomorrow I'll take a look at it. |
Created tests for PR oppia#19264 for the frontend and backend. generate_contributor_certificate_data in suggestion_services.py can now return None, so some function's return values were changed to Optional[ContributorCertificateInfoDict]. Tests were added on the frontend, and tests on the backend had to be changed to "assert response is not None" rather than "self.assertIsNotNone(response)" in order to satisfy the type checker. Notably, "None" also had to be added to the union type parameter in the render_json function in controllers/base.py for this reason as well.
Also changes to docstrings
Alright, finally got all checks to pass. One issue was that to satisfy the type checker, I had to also add "None" to the Union type of the parameter "values" of the "render_json" method in the BaseHandler. This is because generating a certificate now may produce a None value, and that will get sent to the client through the render_json method. If there is another way you prefer to deal with this, just let me know. |
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 @mgporter, thanks for taking a look at this! Some notes:
- Let's not introduce None in render_json. Instead, update the handler in controllers/contributor_dashboard.py to render something like {certificate_data: ...} where the value can potentially be None.
- When you are returning None, please update the corresponding docstring to explain what a return value of None represents.
- When responding to comments, please follow step 5 in the PR guide. In particular, reply to all reviewer comments, don't resolve comments, and request a review (see the last checkbox in the "Essential Checklist" on the PR). Otherwise your PR might not ever get reviewed since reviewers check things based on their being assigned.
Hope this is useful -- thanks again for the changes!
@@ -7343,7 +7343,7 @@ def test_create_translation_contributor_certificate(self) -> None: | |||
self.to_date, | |||
) | |||
|
|||
self.assertIsNotNone(response) | |||
assert response is not 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.
Why did this change? If the change isn't necessary I suggest keeping it how it was before.
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 change is required because the response could be None. Because of that, Mypy issues a warning whenever we try to index into the variable 'response', i.e., response['contribution_hours'] . By using assert, this cues the typechecker that the value will not be None in the following lines. Apparently, the typechecker is not able to get this information from self.assertIsNotNone(response).
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.
Ah ok, thanks. Please see the first bullet point of #20261 (review), it might change this somewhat (but what you say makes sense!)
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.
Speaking of the first bullet point above, in the new commits I am going to push later, contributor_dashboard.py looks like this:
The certificateDataResponse is a 'wrapper' that can hold 'response', which is the certificate data, or None. I think this is what you were saying in the first bullet point. Please correct me if I am wrong!
If correct, then the suggestion_services.generate_contributor_certificate_data() method above may still return None, which would make the "assert response is not None" necessary in the relevant tests.
As a side note, if you wanted to avoid None results entirely, we could just tell the front end to render an error if the contribution_hours field (from the certificate data) is equal to 0. It's a different way to solve the same problem, so I thought I'd just put that out there.
By the way, thanks for all of the 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.
Yup, what you have looks right, thanks! Maybe call the local response
variable certificate_data
instead.
OK to keep None as a return value. Let's also rename response
to certificate_data
in the tests as well, and add a comment above the "assert is not None" to explain this is needed for Mypy.
And yup, sure thing!
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.
Ok, these changes are done!
'There are no contributions for the given time range.' | ||
): | ||
suggestion_services.generate_contributor_certificate_data( | ||
data = suggestion_services.generate_contributor_certificate_data( | ||
self.username, |
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.
Indent this and the following by 4 rather than 8 from the previous line; ditto below.
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!
Added CertificateDataResponse datatype to hold the certificate data or None. Added documentation for python asserts in tests. Some variable names, docstrings, and indentation were also updated.
Merging updated remote-tracking branch 'upstream/develop' into topic branch after code review changes
removed null value from downloadContributorCertificateAsync return type. Updated tests to reflect new ContributorCertificateResponse schema and added a test for when contributions are 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.
Thanks @mgporter, just two notes.
Also, I just wanted to note I'm not a codeowner of this PR. Please make sure to follow the last checkbox in the "Essential checklist" to assign the codeowners as reviewers. You'll want to ensure that they show up in the assignees field -- see the instructions for making a code change, step 5, for more details.
@@ -1052,6 +1052,15 @@ def get( | |||
self.render_json(self.values) | |||
|
|||
|
|||
class CertificateDataResponse(TypedDict): | |||
"""Dict holding info. |
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 needs a clearer docstring.
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.
Got it! That was my mistake.
expect(response.language).toEqual('Hindi'); | ||
const data = response.certificate_data; | ||
expect(data).toBeDefined(); | ||
expect(data?.from_date).toEqual('1 Nov 2022'); |
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 do you need the question marks here and below?
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 data == null, then the lines expect(data.from_date).toEqual('1 Nov 2022')
and below give a type error.
I was considering adding the lines below, which would satisfy the type checker for the expect expressions, but I later went with the other approach.
if (!data) {
fail("some fail message here");
return;
}
I couldn't find another example of this situation in the code, so it was hard to tell what would be best stylistically. Please give me your thoughts!
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 changed the test to use the if (!data) ...
block to satisfy the type checker. There might be a different way you'd want an optional value such as this to be handled within the test. Just let me know.
Wrote clearer docstring for CertificateDataResponse class in contributor_dashboard.py. Also made slight change to the way that a test handles null values in contribution-and-review.service.spec.ts
Thank you for clearing this up! @kevintab95 PTAL |
Head branch was pushed to by a user without write access
Ok! Looks like we just need Chris7716 to take a look at this, and then it can be merged. @chris7716 PTAL Thank you! |
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.
Submitting final changes for review. I'm just learning this workflow, so sorry about the back-and-forth.
@@ -4001,8 +4001,7 @@ def _generate_translation_contributor_certificate_data( | |||
hours_contributed = round(words_count / 300, 2) | |||
|
|||
if words_count == 0: | |||
raise Exception( | |||
'There are no contributions for the given time range.') | |||
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.
Hi @chris7716, would you mind taking a look at this again?
this.createCertificate(response); | ||
if (response) { | ||
this.createCertificate(response); | ||
} else { |
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.
Frontend tests have been added! Thank you for the feedback!
@seanlip @kevintab95 Thank you for approving the changes. I think I did something wrong-- when I made a commit to address Seanlip's last suggestion, the system went back to "Merging is blocked" status, and now is waiting on @chris7716's approval . However, it's been 27 days since he has responded. What should I do? @chris7716 PTAL Sorry for the trouble. |
@mgporter -- I'm not sure but I'll ping @chris7716 and see if he can take a look. Thanks for flagging this! (Btw, feel free to start another PR for a different issue in the meantime if you like -- just make sure to branch it off of develop and not off this PR so that things remain clean on your local git setup!) |
Thank you Sean, you have been so helpful through my first PR. I have been looking through the other issues recently, and there are indeed a few I want to try. |
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.
LGTM! Sorry for the late review—I was(and still am) away due to being sick.
Unassigning @chris7716 since they have already approved the PR. |
Hi @mgporter, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks! |
@chris7716 No worries, thank you for coming back to this. I wish for you the best! |
Queueing this for merge. And @mgporter, congrats on your first PR to Oppia! 😄 🎉 🍾 |
…ns for the given time range (oppia#20261) * Fixed server exception when generating certificates Allowed suggestion_services.py to return None when no contributions are found for the given date range. On the client side, the certificate-download-modal component now checks for this None value (which is stringified to "null") and displays the appropriate warning message to the user. * Fixed issues with tests for PR oppia#19264 Created tests for PR oppia#19264 for the frontend and backend. generate_contributor_certificate_data in suggestion_services.py can now return None, so some function's return values were changed to Optional[ContributorCertificateInfoDict]. Tests were added on the frontend, and tests on the backend had to be changed to "assert response is not None" rather than "self.assertIsNotNone(response)" in order to satisfy the type checker. Notably, "None" also had to be added to the union type parameter in the render_json function in controllers/base.py for this reason as well. * Minor changes to spacing to pass lint checks Also changes to docstrings * Addressing review comments for oppia#19264 Added CertificateDataResponse datatype to hold the certificate data or None. Added documentation for python asserts in tests. Some variable names, docstrings, and indentation were also updated. * Removed null return type from downloadContributorCertificateAsync * Changes to contribution-and-review.service and its tests removed null value from downloadContributorCertificateAsync return type. Updated tests to reflect new ContributorCertificateResponse schema and added a test for when contributions are not found. * Fixed code formatting in contribution-and-review.service.spec.ts * Clearer docstring and changed format of test Wrote clearer docstring for CertificateDataResponse class in contributor_dashboard.py. Also made slight change to the way that a test handles null values in contribution-and-review.service.spec.ts * code style change * Added type assertion, and changed test callback to async * Fixed indentation in suggestion_services_test
Overview
This PR fixes There are no contributions for the given time range #19264 .
This PR does the following: Allowed suggestion_services.py to return None when no contributions are found for the given date range. On the client side, the certificate-download-modal component now checks for this None value (which is stringified to "null") and displays the appropriate warning message to the user.
Essential Checklist
Please follow the instructions for making a code change.
Proof that changes are correct
PR Pointers