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!

nikitaevg · 2024-05-18T08:17:23Z

core/controllers/contributor_dashboard_admin.py

@@ -516,7 +516,7 @@ class ContributorDashboardAdminStatsHandlerNormalizedPayloadDict(TypedDict):
    language_code: Optional[str]
    sort_by: Optional[str]
    topic_ids: Optional[List[str]]
-    max_days_since_last_activity: Optional[int]
+    last_date: str


last_date is not a very intuitive name. Suppose you haven't been working on this handler for the last few weeks, would you understand what this parameter represents without looking into the implementation?

start_date would be a more canonical name.

nikitaevg · 2024-05-18T08:18:26Z

core/storage/suggestion/gae_models.py

-                if not result_models:
+
+        next_offset = offset
+        while len(sorted_results) < page_size:


This still stands, please address

nikitaevg · 2024-05-18T08:23:11Z

core/controllers/contributor_dashboard_admin.py

@@ -516,7 +516,7 @@ class ContributorDashboardAdminStatsHandlerNormalizedPayloadDict(TypedDict):
    language_code: Optional[str]
    sort_by: Optional[str]
    topic_ids: Optional[List[str]]
-    max_days_since_last_activity: Optional[int]
+    last_date: str


Ok, I'm not sure now we need to change anything on the backend side. The parameters are almost the same, it's essentially a refactoring, not a new functionality. We can allow users to choose a date, but then we could calculate the number of days between now and that date and then pass it to the backend.

Unless somebody disagrees with that, and unless you are fixing some bugs, I'd prefer keeping the backend as it is right now.

Hello @nikitaevg, as per suggestion I will be leaving the backend as it is for now is the field name max_days_since_last_activity good to use ATM or should it be changed.

oppiabot · 2024-05-18T08:26:21Z

Unassigning @nikitaevg since the review is done.

oppiabot · 2024-05-18T08:26:24Z

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

…e-filter

masterboy376 · 2024-05-21T14:39:35Z

@nikitaevg , @chris7716 , @vojtechjelinek PTAL

oppiabot · 2024-05-21T14:42:39Z

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

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!

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

nikitaevg requested changes May 18, 2024

View reviewed changes

oppiabot bot unassigned nikitaevg May 18, 2024

oppiabot bot assigned masterboy376 May 18, 2024

masterboy376 added 3 commits May 19, 2024 19:06

addressed Nik's comments

8af8752

fixed a bug

00372a0

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

bc34376

…e-filter

masterboy376 requested a review from nikitaevg May 21, 2024 14:23

oppiabot bot assigned nikitaevg and unassigned masterboy376 May 21, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

masterboy376 commented Mar 17, 2024 •

edited

oppiabot bot commented Mar 17, 2024

oppiabot bot commented Apr 5, 2024

nikitaevg May 18, 2024

masterboy376 May 21, 2024

nikitaevg May 18, 2024

nikitaevg May 18, 2024

masterboy376 May 18, 2024

masterboy376 May 21, 2024

oppiabot bot commented May 18, 2024

oppiabot bot commented May 18, 2024

masterboy376 commented May 21, 2024

oppiabot bot commented May 21, 2024

chris7716 left a comment

chris7716 May 22, 2024

masterboy376 May 23, 2024

chris7716 May 22, 2024

masterboy376 May 23, 2024

chris7716 May 22, 2024

masterboy376 May 23, 2024 •

edited

oppiabot bot commented May 22, 2024

masterboy376 commented May 23, 2024

nikitaevg commented May 23, 2024

vojtechjelinek left a comment

vojtechjelinek May 24, 2024

masterboy376 May 24, 2024

vojtechjelinek May 24, 2024

masterboy376 May 24, 2024

vojtechjelinek May 24, 2024

masterboy376 May 24, 2024 •

edited

oppiabot bot commented May 24, 2024

oppiabot bot commented May 24, 2024

masterboy376 commented May 24, 2024

oppiabot bot commented May 24, 2024

		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

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 18, 2024

oppiabot bot commented May 18, 2024

masterboy376 commented May 21, 2024

oppiabot bot commented May 21, 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

masterboy376 commented Mar 17, 2024 •

edited

masterboy376 May 23, 2024 •

edited

masterboy376 May 24, 2024 •

edited