Add preset views #320

nknguyenhc · 2024-03-27T13:39:27Z

Summary:

Fixes #300

Type of change:

✨ New Feature/ Enhancement

Changes Made:

Add deadline field to milestone.
Add preset views.
Apply preset views on filters.
Change header to contain a button to select preset view.

"Currently active" preset view depends on sorting by status, so changes in this PR will fully work when #318 is merged.

Any changes made to type, status and milestones filters will set the preset view value to "custom".

Screenshots:

Proposed Commit Message:

Add preset views

Currently, there are a lot of options of filters,
but there are no suggested useful preset views.

We add preset views for users to choose from,
and via a button on the header.

Checklist:

I have tested my changes thoroughly.
I have created tests for any new code files created in this PR or provided a link to a issue/PR that addresses this.
I have added or modified code comments to improve code readability where necessary.
I have updated the project's documentation as necessary.

NereusWB922 · 2024-03-28T05:45:22Z

In current implementation, seems like the preset view only include preset value for filters. What if we need preset view with different grouping in the future? Should we include groupby value in the preset value?

nknguyenhc · 2024-03-28T06:34:57Z

In current implementation, seems like the preset view only include preset value for filters. What if we need preset view with different grouping in the future? Should we include groupby value in the preset value?

Agree, but given the time constraint until the next release, I would be more inclined to just focus on the filters first, and then let this preset view handle the groupby feature later. Also since logic for groupby is in a separate service, I would create another PR for the additional logic.

Arif-Khalid

A great quality PR which highlights a few good SWE principles.
Some comments about the behaviour of preset views in relation to changing filters as well as a possible error in the preset view filters.
Overall a good PR, well done!

Arif-Khalid · 2024-03-28T14:16:57Z

src/app/core/services/filters.service.ts

+  readonly presetViews: {
+    [key: string]: () => Filter;
+  } = {
+    currentlyActive: () => ({
+      title: '',
+      status: ['open pullrequest', 'merged pullrequest', 'open issue', 'closed issue'],
+      type: 'all',
+      sort: { active: 'status', direction: 'asc' },
+      labels: [],
+      milestones: this.getMilestonesForCurrentlyActive().map((milestone) => milestone.title),
+      hiddenLabels: new Set<string>(),
+      deselectedLabels: new Set<string>()
+    }),
+    contributions: () => ({
+      title: '',
+      status: ['open pullrequest', 'merged pullrequest', 'open issue', 'closed issue'],
+      type: 'all',
+      sort: { active: 'id', direction: 'desc' },
+      labels: [],
+      milestones: this.milestoneService.milestones.map((milestone) => milestone.title),
+      hiddenLabels: new Set<string>(),
+      deselectedLabels: new Set<string>()
+    }),
+    custom: () => this.filter$.value
+  };


Since changing labels, hiddenLabels, deselectedLabels, sort and title do not make the preset view "custom", should we reset them or set them to a fixed value when setting the preset view?
To me, it is unintuitive that since it is "not part of the preset views" since changing these parameters did not affect the status of my preset view being put into custom, choosing a preset view should not reset these parameters.

Additionally, is it intentional to provide currentlyActive preset with an invalid sort id of status?

Since changing labels, hiddenLabels, deselectedLabels, sort and title do not make the preset view "custom", should we reset them or set them to a fixed value when setting the preset view?
To me, it is unintuitive that since it is "not part of the preset views" since changing these parameters did not affect the status of my preset view being put into custom, choosing a preset view should not reset these parameters.

I agree. But currently, there is a regular label polling, and everytime the labels are fetched, the filters are reset, meaning that the "preset view" will be set to Custom after 5 seconds. I can't really think of a good way to handle this, other than having to compare whether the new labels are the same as the current filter. I would think we should not handle this in filter service, but rather, stop calling filtersService::updateFilters whenever the labels are fetched. Don't really want to fix that at the risk of introducing more bug at this stage.

Additionally, is it intentional to provide currentlyActive preset with an invalid sort id of status?

I am assuming that #318 will be merged soon, so status will be a valid sorting ID.

I see, would a better implementation be to only update certain filters when choosing a preset view then? Simply leaving labels and those other "untracked" filters as they are when switching to a certain preset view.

I would be more inclined to standardise the preset views. The purpose of "currently active" preset view, for e.g., is to get the distribution of work between people within the current milestone. The user might forget to clear all label filters before setting the preset view to "currently active", and thereby get the wrong impression of the work distribution.

I see and that makes sense. Is there a reason the other filters aside from the labels are not "tracked" by the preset views?

I would argue that the more important part of the preset view is to get the total count. So for the other filters not being tracked:

Title: the user might want to search for a particular issue/PR. Technically, the user still stays in the same preset view, as the user is still in the same preset view when the search input is cleared.

Sort: arguably, sort is an important part of the filters, especially for currently active, but user still can choose their own sorting order, and the total count of each column does not change.

Labels, selected labels, deselected labels: as explained above, we can look into including these as one of the "tracked" filters.

Hidden labels: This only affects which label is shown on each PR/issue card and which is not, so total count of each column does not change.

These are just my reasoning, let me know if you think otherwise.

Your reasoning is sound. I am inclined to agree but still think there may be some confusion.
When I was testing it, I found it strange that my preset views were maintained when i did something like search by title yet my title was reset when i chose a preset view.
Perhaps a new feature could encompass making a (edited) status for the current preset view when these filters are changed while remaining in the preset view.
However, this would be in a separate PR, I have no issue with the current implementation thank you for clarifying your thought process.

I have found out an easy way to let the filters service track the labels fields as well. Because sanitiseLabels is only called whenever the labels are polled, I make sure that the update of filters from this method does not change the preset view value.

Arif-Khalid · 2024-03-28T14:18:26Z

src/app/core/services/filters.service.ts

+  getMilestonesForCurrentlyActive(): Milestone[] {
+    const earliestOpenMilestone = this.milestoneService.getEarliestOpenMilestone();
+    if (earliestOpenMilestone) {
+      return [earliestOpenMilestone];
+    }
+
+    const latestClosedMilestone = this.milestoneService.getLatestClosedMilestone();
+    if (latestClosedMilestone) {
+      return [latestClosedMilestone];
+    }
+
+    return this.milestoneService.milestones;
+  }


Good handling of edge cases

Arif-Khalid · 2024-03-28T14:19:26Z

src/app/core/services/milestone.service.ts

+  getEarliestOpenMilestone(): Milestone {
+    let earliestOpenMilestone: Milestone = null;
+    for (const milestone of this.milestones) {
+      if (!milestone.deadline || milestone.state !== 'open') {
+        continue;
+      }
+      if (earliestOpenMilestone === null) {
+        earliestOpenMilestone = milestone;
+      } else if (milestone.deadline < earliestOpenMilestone.deadline) {
+        earliestOpenMilestone = milestone;
+      }
+    }
+    return earliestOpenMilestone;
+  }
+
+  /**
+   * Gets the closed milestone with the latest deadline.
+   * Returns null if there is no closed milestone with deadline.
+   */
+  getLatestClosedMilestone(): Milestone {
+    let latestClosedMilestone: Milestone = null;
+    for (const milestone of this.milestones) {
+      if (!milestone.deadline || milestone.state !== 'closed') {
+        continue;
+      }
+      if (latestClosedMilestone === null) {
+        latestClosedMilestone = milestone;
+      } else if (milestone.deadline > latestClosedMilestone.deadline) {
+        latestClosedMilestone = milestone;
+      }
+    }
+    return latestClosedMilestone;
+  }


Functions are well written, documented and accomplishes its task in a simple way.
Perfect representation of KISS.

Arif-Khalid · 2024-03-28T14:20:33Z

src/app/shared/layout/header.component.html

  </span>

-  <div *ngIf="auth.isAuthenticated() && this.viewService.sessionData.sessionRepo.length > 1">
+  <!--div *ngIf="auth.isAuthenticated() && this.viewService.sessionData.sessionRepo.length > 1">


Should this redundant code be left in the code base?

We are still unsure whether to keep the activity dashboard. Currently, this is the only gateway to the activity dashboard, so I'm afraid it will be more difficult for the next batch of ppl to unhide activity dashboard, if we ever want to develop it.

I see. In that case could you add a comment highlighting its significance for other developers who might remove it on instinct?

NereusWB922

Just a minor suggestion on the default view.

src/app/core/services/filters.service.ts

Arif-Khalid

I like the changes made and the current behaviour.
Great PR, LGTM!

NereusWB922

LGTM!

Add preset views

8f465fc

Arif-Khalid reviewed Mar 28, 2024

View reviewed changes

NereusWB922 requested changes Mar 28, 2024

View reviewed changes

src/app/core/services/filters.service.ts Outdated Show resolved Hide resolved

src/app/core/services/filters.service.ts Outdated Show resolved Hide resolved

Apply currently active as default filter

b4a7c31

nknguyenhc requested a review from NereusWB922 March 29, 2024 01:00

nknguyenhc added 2 commits March 29, 2024 12:22

Remove tracking hiddenLabels

bf4cd5b

Add note on commented code

ee70a06

Arif-Khalid previously approved these changes Mar 29, 2024

View reviewed changes

NereusWB922 previously approved these changes Mar 29, 2024

View reviewed changes

Merge branch 'main' into preset-view

f53a718

nknguyenhc dismissed stale reviews from NereusWB922 and Arif-Khalid via f53a718 March 29, 2024 11:10

NereusWB922 approved these changes Mar 29, 2024

View reviewed changes

nknguyenhc merged commit c3418d8 into CATcher-org:main Mar 29, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preset views #320

Add preset views #320

nknguyenhc commented Mar 27, 2024

NereusWB922 commented Mar 28, 2024

nknguyenhc commented Mar 28, 2024

Arif-Khalid left a comment

Arif-Khalid Mar 28, 2024

Arif-Khalid Mar 28, 2024

nknguyenhc Mar 28, 2024

Arif-Khalid Mar 28, 2024

nknguyenhc Mar 28, 2024

Arif-Khalid Mar 28, 2024

nknguyenhc Mar 28, 2024

Arif-Khalid Mar 28, 2024

nknguyenhc Mar 29, 2024

Arif-Khalid Mar 28, 2024

Arif-Khalid Mar 28, 2024

Arif-Khalid Mar 28, 2024

nknguyenhc Mar 28, 2024

Arif-Khalid Mar 28, 2024

NereusWB922 left a comment •

edited

Loading

Arif-Khalid left a comment

NereusWB922 left a comment

Add preset views #320

Add preset views #320

Conversation

nknguyenhc commented Mar 27, 2024

Summary:

Type of change:

Changes Made:

Screenshots:

Proposed Commit Message:

Checklist:

NereusWB922 commented Mar 28, 2024

nknguyenhc commented Mar 28, 2024

Arif-Khalid 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

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

NereusWB922 left a comment • edited Loading

Choose a reason for hiding this comment

Arif-Khalid left a comment

Choose a reason for hiding this comment

NereusWB922 left a comment

Choose a reason for hiding this comment

NereusWB922 left a comment •

edited

Loading