Fix #19264: Fixed server exception when there are no contributions for the given time range #20261

mgporter · 2024-05-04T03:06:07Z

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.

I have linked the issue that this PR fixes in the "Development" section of the sidebar.
I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
I have written tests for my code.
The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I can't assign them directly).

Proof that changes are correct

PR Pointers

Never force push! If you do, your PR will be closed.
To reply to reviewers, follow these instructions: https://github.com/oppia/oppia/wiki/Make-a-pull-request#step-5-address-review-comments-until-all-reviewers-approve
Some e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.
See the Code Owner's wiki page for what code owners will expect.

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.

oppiabot · 2024-05-04T03:06:19Z

Assigning @chris7716 for the first pass review of this PR. Thanks!

chris7716

Hey @mgporter Thanks for the PR! I have a couple of comments.

chris7716 · 2024-05-04T08:49:11Z

core/domain/suggestion_services.py

@@ -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


Could you please update the backend tests for this change. I think backend tests in the CI are failing due to this.

Got it! suggestion_services.py has had its tests updated.

Hi @chris7716, would you mind taking a look at this again?

chris7716 · 2024-05-04T08:49:53Z

...tes/pages/contributor-dashboard-page/modal-templates/certificate-download-modal.component.ts

-        this.createCertificate(response);
+        if (response) {
+          this.createCertificate(response);
+        } else {


I believe you will have to add frontend tests for this as well.

Frontend tests have been added! Thank you for the feedback!

mgporter · 2024-05-05T04:39:37Z

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

mgporter · 2024-05-06T03:50:27Z

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.

seanlip

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!

seanlip · 2024-05-09T18:01:03Z

core/domain/suggestion_services_test.py

@@ -7343,7 +7343,7 @@ def test_create_translation_contributor_certificate(self) -> None:
            self.to_date,
        )

-        self.assertIsNotNone(response)
+        assert response is not None


Why did this change? If the change isn't necessary I suggest keeping it how it was before.

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).

Ah ok, thanks. Please see the first bullet point of #20261 (review), it might change this somewhat (but what you say makes sense!)

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!

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!

Ok, these changes are done!

seanlip · 2024-05-09T18:01:19Z

core/domain/suggestion_services_test.py

-            'There are no contributions for the given time range.'
-        ):
-            suggestion_services.generate_contributor_certificate_data(
+        data = suggestion_services.generate_contributor_certificate_data(
                self.username,


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.

seanlip

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.

seanlip · 2024-05-13T14:23:09Z

core/controllers/contributor_dashboard.py

@@ -1052,6 +1052,15 @@ def get(
        self.render_json(self.values)


+class CertificateDataResponse(TypedDict):
+    """Dict holding info.


This needs a clearer docstring.

Got it! That was my mistake.

seanlip · 2024-05-13T14:24:10Z

.../templates/pages/contributor-dashboard-page/services/contribution-and-review.service.spec.ts

-          expect(response.language).toEqual('Hindi');
+          const data = response.certificate_data;
+          expect(data).toBeDefined();
+          expect(data?.from_date).toEqual('1 Nov 2022');


Why do you need the question marks here and below?

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!

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

mgporter · 2024-05-13T15:23:41Z

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

Thank you for clearing this up!

@kevintab95 PTAL
@chris7716 PTAL

seanlip · 2024-05-17T16:48:27Z

@mgporter I just realized I forgot to assign you while reviewing. I'm doing so now since there's still an open comment -- thanks!

mgporter · 2024-05-17T20:16:15Z

Thanks for following up. I just pushed the new changes.

oppiabot · 2024-05-25T02:39:46Z

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.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

mgporter · 2024-05-25T02:48:09Z

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
@seanlip PTAL
@kevintab95 PTAL

oppiabot · 2024-05-25T02:51:13Z

Unassigning @mgporter since a re-review was requested. @mgporter, please make sure you have addressed all review comments. Thanks!

seanlip

Thanks @mgporter! Sorry for the late reply. No significant concerns from my end, just one minor nit.

@kevintab95 @chris7716 PTAL? Thanks!

seanlip · 2024-05-25T07:49:53Z

core/domain/suggestion_services_test.py

-        ):
-            suggestion_services.generate_contributor_certificate_data(
+        certificate_data = (
+                suggestion_services.generate_contributor_certificate_data(


This line should be deindented by 4

Got it, fixing it right now

kevintab95

LGTM for codeowner files. Thanks @mgporter!

mgporter · 2024-05-28T20:49:18Z

Ok! Looks like we just need Chris7716 to take a look at this, and then it can be merged.

@chris7716 PTAL

Thank you!

mgporter

Submitting final changes for review. I'm just learning this workflow, so sorry about the back-and-forth.

mgporter · 2024-05-28T20:53:16Z

core/domain/suggestion_services.py

@@ -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


Hi @chris7716, would you mind taking a look at this again?

mgporter · 2024-05-28T20:54:13Z

...tes/pages/contributor-dashboard-page/modal-templates/certificate-download-modal.component.ts

-        this.createCertificate(response);
+        if (response) {
+          this.createCertificate(response);
+        } else {


Frontend tests have been added! Thank you for the feedback!

mgporter · 2024-05-31T12:57:16Z

@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.

oppiabot · 2024-05-31T13:00:21Z

Unassigning @mgporter since a re-review was requested. @mgporter, please make sure you have addressed all review comments. Thanks!

seanlip · 2024-05-31T14:16:15Z

@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!)

mgporter requested a review from a team as a code owner May 4, 2024 03:06

mgporter requested a review from chris7716 May 4, 2024 03:06

oppiabot bot assigned chris7716 May 4, 2024

chris7716 requested changes May 4, 2024

View reviewed changes

chris7716 assigned mgporter and unassigned chris7716 May 4, 2024

mgporter requested review from a team as code owners May 6, 2024 01:42

mgporter requested a review from kevintab95 May 6, 2024 01:42

Minor changes to spacing to pass lint checks

b36a4a7

Also changes to docstrings

mgporter requested a review from chris7716 May 7, 2024 13:30

seanlip requested changes May 9, 2024

View reviewed changes

mgporter added 6 commits May 10, 2024 22:04

Addressing review comments for oppia#19264

67a0240

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.

Merge updated upstream into fixcertificate

470a819

Merging updated remote-tracking branch 'upstream/develop' into topic branch after code review changes

Removed null return type from downloadContributorCertificateAsync

3d1c671

Changes to contribution-and-review.service and its tests

b9ce49c

removed null value from downloadContributorCertificateAsync return type. Updated tests to reflect new ContributorCertificateResponse schema and added a test for when contributions are not found.

Merge remote-tracking branch 'upstream/develop' into fixcertificate

5e94393

Fixed code formatting in contribution-and-review.service.spec.ts

f37154d

mgporter requested a review from seanlip May 12, 2024 15:54

seanlip requested changes May 13, 2024

View reviewed changes

mgporter added 3 commits May 13, 2024 11:11

Clearer docstring and changed format of test

603e7be

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

Merge remote-tracking branch 'upstream/develop' into fixcertificate

b82ac61

code style change

125988f

oppiabot bot assigned chris7716 May 13, 2024

Added type assertion, and changed test callback to async

87f25a2

mgporter requested a review from seanlip May 18, 2024 02:35

oppiabot bot added the stale label May 25, 2024

oppiabot bot removed the stale label May 25, 2024

oppiabot bot assigned seanlip and unassigned mgporter May 25, 2024

seanlip approved these changes May 25, 2024

View reviewed changes

seanlip assigned mgporter and unassigned seanlip May 25, 2024

kevintab95 approved these changes May 28, 2024

View reviewed changes

kevintab95 enabled auto-merge May 28, 2024 18:42

kevintab95 removed their assignment May 28, 2024

mgporter added 2 commits May 28, 2024 14:57

Fixed indentation in suggestion_services_test

2a52c25

Merge remote-tracking branch 'upstream/develop' into fixcertificate

f9ddd69

auto-merge was automatically disabled May 28, 2024 19:01
Head branch was pushed to by a user without write access

mgporter commented May 28, 2024

View reviewed changes

oppiabot bot assigned kevintab95 and seanlip and unassigned mgporter May 31, 2024

seanlip unassigned seanlip and kevintab95 May 31, 2024

Fix #19264: Fixed server exception when there are no contributions for the given time range #20261

Are you sure you want to change the base?

Fix #19264: Fixed server exception when there are no contributions for the given time range #20261

Conversation

mgporter commented May 4, 2024 • edited

Overview

Essential Checklist

Proof that changes are correct

PR Pointers

oppiabot bot commented May 4, 2024

chris7716 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgporter commented May 5, 2024

mgporter commented May 6, 2024

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgporter commented May 13, 2024

seanlip commented May 17, 2024

mgporter commented May 17, 2024

oppiabot bot commented May 25, 2024

mgporter commented May 25, 2024

oppiabot bot commented May 25, 2024

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevintab95 left a comment

Choose a reason for hiding this comment

mgporter commented May 28, 2024

mgporter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgporter commented May 31, 2024

oppiabot bot commented May 31, 2024

seanlip commented May 31, 2024

mgporter commented May 4, 2024 •

edited