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
base: develop
Are you sure you want to change the base?
Fix part of #19570: Adding time range filter #19979
Conversation
Hi @masterboy376 please assign the required reviewer(s) for this PR. Thanks! |
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. |
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
if not result_models: | ||
|
||
next_offset = offset | ||
while len(sorted_results) < page_size: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still stands, please address
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Unassigning @nikitaevg since the review is done. |
Hi @masterboy376, it looks like some changes were requested on this pull request by @nikitaevg. PTAL. Thanks! |
@nikitaevg , @chris7716 , @vojtechjelinek PTAL |
Unassigning @masterboy376 since a re-review was requested. @masterboy376, please make sure you have addressed all review comments. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Some minor questions. Otherwise it looks good.
@@ -240,23 +256,46 @@ export class ContributorAdminDashboardPageComponent implements OnInit { | |||
this.languageDropdownShown = !this.languageDropdownShown; | |||
} | |||
|
|||
toggleActivityDropdown(): void { | |||
this.activityDropdownShown = !this.activityDropdownShown; | |||
getDateThatIsDaysBeforeToday(numberOfDaysBeforeToday: number): Date { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain what this function does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
); | ||
} | ||
|
||
getNumberOfDaysForDateBeforeToday(date: Date): number { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain what this function does and why it needs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 .
); | ||
|
||
if (this.filter === undefined || !isEqual(tempFilter, this.filter)) { | ||
this.filter = tempFilter; | ||
} | ||
} | ||
|
||
changeLastDate(value: Date): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is required? Please add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Unassigning @chris7716 since they have already approved the PR. |
Just a friendly ping, @nikitaevg PTAL |
Sorry, on a vacation, will review on Monday |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Left a few comments.
total_contribution_stats_helper = TotalContributionStatsHelper[ | ||
TranslationSubmitterTotalContributionStatsModel]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
total_contribution_stats_helper = TotalContributionStatsHelper[ | |
TranslationSubmitterTotalContributionStatsModel]() | |
total_contribution_stats_helper = TotalContributionStatsHelper[ | |
TranslationSubmitterTotalContributionStatsModel | |
]() |
ditto below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
T = TypeVar( | ||
'T', | ||
TranslationSubmitterTotalContributionStatsModel, | ||
TranslationReviewerTotalContributionStatsModel, | ||
QuestionSubmitterTotalContributionStatsModel, | ||
QuestionReviewerTotalContributionStatsModel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
index.yaml
Outdated
# 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'. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Unassigning @vojtechjelinek since the review is done. |
Hi @masterboy376, it looks like some changes were requested on this pull request by @vojtechjelinek. PTAL. Thanks! |
@vojtechjelinek PTAL |
Unassigning @masterboy376 since a re-review was requested. @masterboy376, please make sure you have addressed all review comments. Thanks! |
Overview
Essential Checklist
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