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! |
| @@ -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.
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.
Got it! suggestion_services.py has had its tests updated.
| this.createCertificate(response); | ||
| if (response) { | ||
| this.createCertificate(response); | ||
| } else { |
There was a problem hiding this comment.
I believe you will have to add frontend tests for this as well.
|
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.
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.
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.
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.
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.
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.
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 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.
Indent this and the following by 4 rather than 8 from the previous line; ditto below.
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.
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.
This needs a clearer docstring.
| 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.
Why do you need the question marks here and below?
There was a problem hiding this comment.
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.
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 |
|
One question for the reviewers. Should I |
| expect(downloadContributorCertificateAsyncSpy).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should return null when contributor certificate data is unavailable', () => { |
There was a problem hiding this comment.
You probably need to have an "async" before the function since you have long-running operations in this test? Ditto above. See e.g. line 335 of translate-text-backend-api.service.spec.ts.
In its current state, does the test actually fail if your Promise.resolve() in line 335 doesn't resolve to null?
There was a problem hiding this comment.
That is a good observation! Currently, the test does fail when it is supposed to, i.e., when the promise is resolved with data rather than null.
Looking at the asynchronous tests in general, it seems there are a few different formats (line numbers are from contribution-and-review.service.spec.ts):
- Some pass an async function to "it" and use "await" (line 470)
- Some pass an async function and use "await" with "expectAsync" (lines 280 and 309 from translate-text-backend-api.service.spec.ts)
- some pass an async function but don't use await/return values (lines 437)
- others pass a synchronous function and put the 'expect' expressions within the callback to "then" (lines 294, 333, 358, 383, 418)
- still others use fakeAsync so they can sue the tick()/flushMicrotasks() methods (lines 166, 241, 739, 768, 815, 844, 887, 912, 1023, 1051, most other tests in translate-text-backend-api.service.spec.ts)
The tests relevant to this PR belong to case 4 listed above. I can change just those tests, or all of the tests of the 4th case, or just keep it as it is. I'm not sure which ways are considered more standard practice (cases 1-4 seem more or less interchangeable (?), obviously case 5 is different because more fine-grained control was needed).
There was a problem hiding this comment.
@seanlip It looks like you were on to something! Although the test for expecting null works as expected, there is indeed an issue with our asynchronous tests. It seems the tests in case 4 mentioned in my previous post do not actually run the expect clauses in their then callback functions.
Like you mentioned, one way to fix this is by adding async; however, apparently we must also add await to the asynchronous operation we are testing so that the test knows to wait for the promise to resolve.
Since this issue may affect multiple files outside of this PR, I opened a new issue about it (#20317).
There was a problem hiding this comment.
Ok, for now I added async to the test callback function, and await on for the asynchronous operation. This gives the desired behavior in all circumstances so far.
| expect(response.language).toEqual('Hindi'); | ||
| const data = response.certificate_data; | ||
|
|
||
| if (!data) { |
There was a problem hiding this comment.
I think what you have is probably fine, but in general we try not to have "if" logic in tests. If the issue is with TypeScript then perhaps this approach (type assertions) might work? https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#type-assertions
|
@mgporter I just realized I forgot to assign you while reviewing. I'm doing so now since there's still an open comment -- thanks! |
|
Thanks for following up. I just pushed the new changes. |
|
Hi @mgporter, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
|
Hello again! Would you mind reviewing this PR and leaving feedback? I think I got everything, but please let me know what else you would like to see changed. Thank you! @chris7716 PTAL |
There was a problem hiding this comment.
Thanks @mgporter! Sorry for the late reply. No significant concerns from my end, just one minor nit.
@kevintab95 @chris7716 PTAL? Thanks!
| ): | ||
| suggestion_services.generate_contributor_certificate_data( | ||
| certificate_data = ( | ||
| suggestion_services.generate_contributor_certificate_data( |
There was a problem hiding this comment.
This line should be deindented by 4
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