Skip to content
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 part of #19570: Adding time range filter #19979

Open
wants to merge 96 commits into
base: develop
Choose a base branch
from

Conversation

masterboy376
Copy link
Collaborator

@masterboy376 masterboy376 commented Mar 17, 2024

Overview

  1. This PR fixes part of Fix key release blockers of the contributor admin dashboard #19570.
  2. 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

Copy link

oppiabot bot commented Mar 17, 2024

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

Copy link

oppiabot bot commented Apr 5, 2024

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!

@oppiabot oppiabot bot added the stale label Apr 5, 2024
@oppiabot oppiabot bot removed the stale label Apr 5, 2024
@masterboy376 masterboy376 marked this pull request as ready for review April 5, 2024 14:57
@masterboy376
Copy link
Collaborator Author

Hello, @nikitaevg, I have removed all the unnecessary code from the PR to the best of my knowledge PTAL.

@oppiabot oppiabot bot assigned nikitaevg and unassigned masterboy376 May 30, 2024
Copy link

oppiabot bot commented May 30, 2024

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

Copy link
Contributor

@nikitaevg nikitaevg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better, thanks

<b *ngIf="isSubmitterTab()" class="filter-label">Filter by Last Contributed after:</b>
<b *ngIf="isReviewerTab()" class="filter-label">Filter by Last Reviewed after:</b>
<mat-form-field>
<input matInput class="e2e-test-last-date-picker-input" [matDatepicker]="lastDatepicker" (dateChange)="changeLastDate($event.value)" [(ngModel)]="lastDateToFilterUsersActivity" [max]="today">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lastDatepicker -> lastDatePicker. Otherwise it seems like a last datepicker, not last date picker

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -422,19 +414,51 @@ describe('Contributor dashboard Admin page', () => {
expect(component.selectedLanguage.id).toBe(nonDefaultLanguage.id);
}));

it('should select last activity from dropdown', fakeAsync(() => {
it(
'should initially filters users by whether their last activity occurred ' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should...filter, not should...filters. Also please check other test names

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

})
);

it("should changes filter by users' last activity when end date changes", fakeAsync(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and previous tests could be merged, no need to make them that small

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 234 to 235
this.activeTab === this.TAB_NAME_QUESTION_REVIEWER ||
this.activeTab === this.TAB_NAME_QUESTION_REVIEWER
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, Thanks for highlighting. Fixed it.

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

toggleActivityDropdown(): void {
this.activityDropdownShown = !this.activityDropdownShown;
getDateThatIsDaysBeforeToday(numberOfDaysBeforeToday: number): Date {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming here could be improved a bit. Maybe getDateNDaysAgo

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

);
}

getNumberOfDaysForDateBeforeToday(date: Date): number {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getDaysSince?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link

oppiabot bot commented May 31, 2024

Unassigning @nikitaevg since the review is done.

Copy link

oppiabot bot commented May 31, 2024

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

@masterboy376
Copy link
Collaborator Author

@nikitaevg, addressed all the latest comments. PTAL.

@oppiabot oppiabot bot assigned nikitaevg and unassigned masterboy376 May 31, 2024
Copy link

oppiabot bot commented May 31, 2024

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

@oppiabot oppiabot bot unassigned nikitaevg Jun 2, 2024
Copy link

oppiabot bot commented Jun 2, 2024

Unassigning @nikitaevg since they have already approved the PR.

@masterboy376
Copy link
Collaborator Author

@vojtechjelinek, PTAL

@vojtechjelinek vojtechjelinek removed their request for review June 5, 2024 11:59
Copy link
Member

@vojtechjelinek vojtechjelinek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@oppiabot oppiabot bot added the PR: LGTM label Jun 5, 2024
Copy link

oppiabot bot commented Jun 5, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants