Fix part of #19570: Adding time range filter #19979

masterboy376 · 2024-03-17T17:42:17Z

Overview

This PR fixes part of Fix key release blockers of the contributor admin dashboard #19570.
This PR does the following: Adds a filter to contributor admin dashboard that enable users to fetch reviewer and submitter stats for translation and questions from last selected dates to today.

Essential Checklist

The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
I have followed the instructions for making a code change.
I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I don't have permissions to assign reviewers directly).
The linter/Karma presubmit checks pass on my local machine, and my PR follows the coding style guide).
"Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)

Proof that changes are correct

screen-recorder-thu-may-16-2024-09-09-36.webm

Proof of changes on desktop with slow/throttled network

screen-recorder-thu-may-16-2024-09-11-32.webm

Proof of changes on mobile phone

Proof of changes in Arabic language

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.

…e-filter

oppiabot · 2024-03-17T17:42:29Z

Hi @masterboy376 please assign the required reviewer(s) for this PR. Thanks!

…e-filter

oppiabot · 2024-04-05T09:21:44Z

Hi @masterboy376, 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!

chris7716

Thanks! Some minor questions. Otherwise it looks good.

chris7716 · 2024-05-22T08:30:42Z

...mplates/pages/contributor-dashboard-admin-page/contributor-admin-dashboard-page.component.ts

@@ -240,23 +256,46 @@ export class ContributorAdminDashboardPageComponent implements OnInit {
    this.languageDropdownShown = !this.languageDropdownShown;
  }

-  toggleActivityDropdown(): void {
-    this.activityDropdownShown = !this.activityDropdownShown;
+  getDateThatIsDaysBeforeToday(numberOfDaysBeforeToday: number): Date {


Could you please explain what this function does?

This function gives the date corresponding to the give number of days before today. For example, today is 23 may 2024, so for number of days =7 this function returns 16 may 2024.

chris7716 · 2024-05-22T08:31:07Z

...mplates/pages/contributor-dashboard-admin-page/contributor-admin-dashboard-page.component.ts

+    );
+  }
+
+  getNumberOfDaysForDateBeforeToday(date: Date): number {


Could you please explain what this function does and why it needs?

This function returns number of days before today corresponding to the give given date. For example, today is 23 may 2024, so for date = 16 may 2024 this function returns 7 .

chris7716 · 2024-05-22T08:31:49Z

...mplates/pages/contributor-dashboard-admin-page/contributor-admin-dashboard-page.component.ts

    );

    if (this.filter === undefined || !isEqual(tempFilter, this.filter)) {
      this.filter = tempFilter;
    }
  }

+  changeLastDate(value: Date): void {


Why this is required? Please add a comment.

This function acts as an event handler for date picker. When ever we select a new date in the date picker it update the variable that stores date and create filter corresponding to the newly selected date.

oppiabot · 2024-05-22T08:35:33Z

Unassigning @chris7716 since they have already approved the PR.

masterboy376 · 2024-05-23T15:30:39Z

Just a friendly ping, @nikitaevg PTAL

nikitaevg · 2024-05-23T15:36:50Z

Sorry, on a vacation, will review on Monday

vojtechjelinek

Thanks! Left a few comments.

vojtechjelinek · 2024-05-24T14:39:10Z

core/storage/suggestion/gae_models.py

+        total_contribution_stats_helper = TotalContributionStatsHelper[
+        TranslationSubmitterTotalContributionStatsModel]()


Suggested change

total_contribution_stats_helper = TotalContributionStatsHelper[

TranslationSubmitterTotalContributionStatsModel]()

total_contribution_stats_helper = TotalContributionStatsHelper[

TranslationSubmitterTotalContributionStatsModel

]()

ditto below

vojtechjelinek · 2024-05-24T14:40:10Z

core/storage/suggestion/gae_models.py

+T = TypeVar(
+    'T',
+    TranslationSubmitterTotalContributionStatsModel,
+    TranslationReviewerTotalContributionStatsModel,
+    QuestionSubmitterTotalContributionStatsModel,
+    QuestionReviewerTotalContributionStatsModel)


Suggested change

T = TypeVar(

'T',

TranslationSubmitterTotalContributionStatsModel,

TranslationReviewerTotalContributionStatsModel,

QuestionSubmitterTotalContributionStatsModel,

QuestionReviewerTotalContributionStatsModel)

T = TypeVar(

'T',

TranslationSubmitterTotalContributionStatsModel,

TranslationReviewerTotalContributionStatsModel,

QuestionSubmitterTotalContributionStatsModel,

QuestionReviewerTotalContributionStatsModel

)

Do not use T, give this type variable a more meaningful name.

vojtechjelinek · 2024-05-24T14:40:52Z

index.yaml

-# AUTOGENERATED
-
-# This index.yaml is automatically updated whenever the dev_appserver
-# detects that a new type of query is run.  If you want to manage the
-# index.yaml file manually, remove the above marker line (the line
-# saying "# AUTOGENERATED").  If you want to manage some indexes
-# manually, move them above the marker line.  The index.yaml file is
-# automatically uploaded to Cloud Datastore when you next deploy
-# your application using 'gcloud app deploy' or create the indexes
-# using 'gcloud datastore indexes create'.
-


Why is this removed?

hello @vojtechjelinek Actually I didn't remove it explicitly it was removed in a commit earlier when I was reverting some file. Added it back.

oppiabot · 2024-05-24T14:44:36Z

Unassigning @vojtechjelinek since the review is done.

oppiabot · 2024-05-24T14:44:39Z

Hi @masterboy376, it looks like some changes were requested on this pull request by @vojtechjelinek. PTAL. Thanks!

…e-filter

masterboy376 · 2024-05-24T16:23:16Z

@vojtechjelinek PTAL

oppiabot · 2024-05-24T16:26:20Z

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

nikitaevg

Try to keep your PR terse. The less moving parts you touch the less chances you break anything. Please remove all unnecessary changes from this PR

nikitaevg · 2024-05-27T17:22:52Z

core/controllers/contributor_dashboard_admin_test.py

@@ -1320,7 +1321,7 @@ def test_get_translation_reviewer_stats_for_last_activity_filter(
    ) -> None:
        self.login(self.CONTRIBUTOR_EMAIL)

-        # Test with max_days_since_last_activity filter and pagination.


Please read your PRs before sending them and get rid of the unnecessary or stale changes like this one

Sorry @nikitaevg, fixed all such stale code.

nikitaevg · 2024-05-27T17:24:49Z

core/controllers/contributor_dashboard_admin_test.py

@@ -845,7 +846,7 @@ def setUp(self) -> None:
            rejected_translations_count=self.REJECTED_TRANSLATIONS_COUNT,
            rejected_translation_word_count=(
                self.REJECTED_TRANSLATION_WORD_COUNT),
-            first_contribution_date=datetime.datetime.utcnow(),


Are these changes needed for this PR? Please leave only changes related to the issue you are fixing.

These are just the enhancements and not the absolute requirements. I real world scenario first_contribution_date will always be less than or equal to last_contribution_date. For this reason instead of setting first_contribution_date as .utcnow(), I decided to set it as some date older than last_contribution_date.

nikitaevg · 2024-05-27T17:27:09Z

core/templates/services/custom-datepicker-adapter.service.ts

+@Injectable({
+  providedIn: 'root',
+})
+export class CustomDatepickerAdapter extends NativeDateAdapter {


ISODatePickerAdapter is fine

nikitaevg · 2024-05-27T17:34:00Z

core/storage/suggestion/gae_models.py

-                        sorted_results.append(result_model)
-                        if len(sorted_results) == page_size:
-                            break
+            (sorted_results, next_offset) = (


Even this change, while it looks positive it might break something. Let's not touch this, or if you want to do so, extract it into a separate PR

Done, will create a new PR for this if required.

oppiabot · 2024-05-27T17:38:08Z

Unassigning @nikitaevg since the review is done.

oppiabot · 2024-05-27T17:38:11Z

Hi @masterboy376, it looks like some changes were requested on this pull request by @nikitaevg. PTAL. Thanks!

masterboy376 · 2024-05-28T02:15:07Z

@nikitaevg PTAL, addressed all previous comments.

oppiabot · 2024-05-28T02:18:11Z

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

…e-filter

masterboy376 added 6 commits March 15, 2024 18:55

created a workingfeature

92a6df1

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

2c9e18c

…e-filter

improved frontend design

385e0bb

convert date to dd-MMM-yyyy format

a73bea1

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

f7bc999

…e-filter

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

af53044

…e-filter

oppiabot bot assigned masterboy376 Mar 17, 2024

masterboy376 added 14 commits March 18, 2024 22:39

backend coverage test passed

3c98fb9

achived full coverage in core/storage/suggestion/gae_model

25446c9

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

e797542

…e-filter

fixing mypy type checks

f5cc817

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

8317a29

…e-filter

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

ef0d15e

…e-filter

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

35a1987

…e-filter

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

9c7d193

…e-filter

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

8b34610

…e-filter

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

6cffbdc

…e-filter

added e2e test

0523ac7

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

018a2a4

…e-filter

modified test

cf478d7

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

8856b42

…e-filter

oppiabot bot added the stale label Apr 5, 2024

masterboy376 added 3 commits April 5, 2024 17:53

updated e2e test

86403f4

some corrections to the test

da567ac

--amend

3c0174b

oppiabot bot removed the stale label Apr 5, 2024

removed the bug that occured during selecting date on mobile

2a13c8c

masterboy376 marked this pull request as ready for review April 5, 2024 14:57

chris7716 approved these changes May 22, 2024

View reviewed changes

oppiabot bot unassigned chris7716 May 22, 2024

vojtechjelinek requested changes May 24, 2024

View reviewed changes

oppiabot bot unassigned vojtechjelinek May 24, 2024

oppiabot bot assigned masterboy376 May 24, 2024

masterboy376 added 2 commits May 24, 2024 21:32

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

db5d784

…e-filter

addressed vojtech's comments

dce0370

masterboy376 requested a review from vojtechjelinek May 24, 2024 16:23

oppiabot bot assigned vojtechjelinek and unassigned masterboy376 May 24, 2024

nikitaevg requested changes May 27, 2024

View reviewed changes

oppiabot bot unassigned nikitaevg May 27, 2024

oppiabot bot assigned masterboy376 May 27, 2024

addressed Nik's comments

cfcfd56

masterboy376 requested a review from nikitaevg May 28, 2024 02:14

oppiabot bot assigned nikitaevg and unassigned masterboy376 May 28, 2024

masterboy376 added 2 commits May 29, 2024 19:54

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

7ff9152

…e-filter

Merge remote-tracking branch 'upstream/develop' into adding-time-rang…

c56ad66

…e-filter

		total_contribution_stats_helper = TotalContributionStatsHelper[
		TranslationSubmitterTotalContributionStatsModel]()

Fix part of #19570: Adding time range filter #19979

Are you sure you want to change the base?

Fix part of #19570: Adding time range filter #19979

Conversation

masterboy376 commented Mar 17, 2024 • edited

Overview

Essential Checklist

Proof that changes are correct

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

oppiabot bot commented Mar 17, 2024

oppiabot bot commented Apr 5, 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

masterboy376 May 23, 2024 • edited

Choose a reason for hiding this comment

oppiabot bot commented May 22, 2024

masterboy376 commented May 23, 2024

nikitaevg commented May 23, 2024

vojtechjelinek 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

masterboy376 May 24, 2024 • edited

Choose a reason for hiding this comment

oppiabot bot commented May 24, 2024

oppiabot bot commented May 24, 2024

masterboy376 commented May 24, 2024

oppiabot bot commented May 24, 2024

nikitaevg 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

oppiabot bot commented May 27, 2024

oppiabot bot commented May 27, 2024

masterboy376 commented May 28, 2024

oppiabot bot commented May 28, 2024

masterboy376 commented Mar 17, 2024 •

edited

masterboy376 May 23, 2024 •

edited

masterboy376 May 24, 2024 •

edited