Fix incorrect assignee filtering #389

kokerinks · 2025-01-12T09:56:31Z

Summary:

Fixes bug #385

Type of change:

🐛 Bug Fix

Changes Made:

Assignee filter now correctly removes Assignee columns when it is not selected in the filter:

Proposed Commit Message:

Fix incorrect filtering of issues with multiple assignees

Previously, the application will sometimes display additional assignees
which were not selected in the assignee filter.

Let's fix this bug and ensure that the application only shows for 
assignees selected in the assignee filter.

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.

kokerinks · 2025-01-17T15:27:13Z

@Arif-Khalid @Eclipse-Dominator please help to review whenever convenient, thanks!

Arif-Khalid

A good effort at eradicating the bug that works effectively.
However, a number of responsibilities of the services are eroded by the implementation.
I would suggest a much simpler and structured fix by looking at ~/src/app/shared/issue-tables/dropdownfilter.ts. It already has reference to both services and has a consolidated section where filters are applied.
Please follow up with any questions or concerns you might have.

Arif-Khalid · 2025-01-19T15:25:50Z

src/app/core/services/grouping/assignee-grouping-strategy.service.ts

  isInHiddenList(group: GithubUser): boolean {
-    return group !== GithubUser.NO_ASSIGNEE;
+    return (
+      group !== GithubUser.NO_ASSIGNEE && // group is not "No Assignee"
+      !this.filtersService
+        .getAssignees()
+        .map((assignee) => assignee.login)
+        .includes(group.login)
+    ); // group is not selected


Should groups be forced into the hidden list?
The functionality of the hidden list is to display information about which groups happen to be hidden because of the filters selected.
The use of this function to specify unique behaviour for groups that shouldn't be shown in the hidden list even when they are empty, such as the NO_ASSIGNEE group.
Forcing groups into the hidden list goes against this single responsibility and erodes into the responsibility of the filters.

Arif-Khalid · 2025-01-19T15:28:09Z

src/app/issues-viewer/issues-viewer.component.html

+        [ngStyle]="{
+          display: card.isLoading || (card.issueLength > 0 && !groupService.hiddenGroups.includes(card.group)) ? 'initial' : 'none'
+        }"


Should the group service be used as a condition to hide cards?
Hiding based on the group service erodes into the function of filtering where group service is only meant to group, not filter.

Arif-Khalid · 2025-01-19T15:34:35Z

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

+
+  getAssignees(): GithubUser[] {
+    return this.assigneeService.assignees.filter((assignee) => this.filter$.value.assignees.includes(assignee.login));
+  }


Should the filtersService contain a method to return a list of filtered assignees?
The filters service has the single responsibility of housing filters used throughout the application. Therefore the most updated filters should already be within the filter$ observable, handled by the component updating the filter. This method goes against this responsibility and its implementation even outside the filters service shouldn't happen since it implies an error in the use of the filters service.

I understand your following of the already implemented methods getAssigneesForCurrentlyActive and getMilestonesForCurrentlyActive. Those methods are exclusively used to populate the filters based on the preset view which is currently handled by the filters service.
As an aside, it might make more sense to move the preset view and the corresponding functions out of filters service in the future.

kokerinks · 2025-01-26T13:42:17Z

@Arif-Khalid Thank you for your review.

Before this PR, I did attempt a solution through the aforementioned ~/src/app/shared/issue-tables/dropdownfilter.ts. However, I noticed it may not be possible due to the following reasons:

Unlike Milestones, each PR/issue can belong to more than one Assignee
Also note that the existing applyDropdownFilter implementation filters by issue/PR, not by issue-card**
Hence, what happens is:
- Assume an issue X has two assignees A and B, and assignee A is selected on the filter while B is not
- applyDropdownFilter filters by issue/PR, and accepts issue X as A is selected within the assignee filter
- However, when displaying the issue-cards, the system detects that issue X has two assignees A and B, and displays issue-cards under the assignee columns A and B separately (even though it should only display an issue-card for A, and not B)

However, I do agree that this PR's solution is indeed a bit hard-coded and rather unstructured. I will try to look deeper into it, if you have any other suggestions on how to proceed as well please do let me know, or feel free to contact me on the slack channel. Thanks!

** For clarity's sake, this is what i am referring to as an issue-card:

Eclipse-Dominator

Good attempt, however, the filtered items breaks when grouping by milestone.

This can be solved by including back the issueLength == 0 check but will
in turn break grouping by user check. Therefore, this might not be a good solution to solve this bug.

After some exploring, grouping service updates their hidden and displayed groups from result of IssueDataTable filters. Since this issue is present only for the case of multiple assignees., It is probably best to approach from the filters.

Some initial ideas that come to mind are:
let IssueDataTable obtain a list of hidden assignees (from some service probably)

   this.issueSubscription = merge(...displayDataChanges)
      .pipe(
        // maps each change in display value to new issue ordering or filtering
        map(() => {
          /* if this.group.equals matches any value in hidden assignees
                  => immediately return [] as this datatable is guarenteed to be hidden
          */
          // ....
          let data = <Issue[]>Object.values(this.issueService.issues$.getValue()).reverse();
          if (this.defaultFilter) {
            data = data.filter(this.defaultFilter);
          }

          // Filter by assignee of issue
          data = this.groupingContextService.getDataForGroup(data, this.group);

We can guarentee its correctness as group.equals will only return true when group is of type GithubUser type implying that it is grouping via assignees. Therefore, doing so will cause IssueDataTable to emit 0 length which means the given group will be hidden by updateHiddenGroups from group.service.ts.

This shouldnt break other parts of the program in the future since updateHiddenGroup takes in number of items in a group which this doesnt violate i think.

You can apply some typescript syntax to work around group being undefined like
e.g. if (this.group?.equals(...)) <- conditional would be false if group is undefined, which works fine in this case since we are testing for positives.

Eclipse-Dominator · 2025-02-03T09:59:50Z

src/app/core/services/grouping/group.service.ts

@@ -30,7 +30,7 @@ export class GroupService {
  updateHiddenGroups(issueLength: number, target: Group) {
    // If the group is in the hidden list, add it if it has no issues.
    // Also add it if it is unchecked in the filter.
-    if (issueLength === 0 && this.groupingContextService.isInHiddenList(target)) {
+    if (this.groupingContextService.isInHiddenList(target)) {


This change will break when grouping by milestone.
When IssueDataTable displays milestone, all milestones with issues/pr assigned will call isInHiddenList under the milestone context and all issues that originally belong to each milestone will be move to hidden.

kokerinks added 3 commits January 12, 2025 17:09

Replace missing assignee filter

84e0355

Fix assignee filter showing additional columns

b0bc7de

Merge branch 'main' into fix-assignee-filtering

4811b2c

kokerinks marked this pull request as ready for review January 16, 2025 07:54

kokerinks marked this pull request as draft January 16, 2025 07:55

kokerinks marked this pull request as ready for review January 17, 2025 15:25

Arif-Khalid requested changes Jan 19, 2025

View reviewed changes

Eclipse-Dominator requested changes Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect assignee filtering #389

Fix incorrect assignee filtering #389

kokerinks commented Jan 12, 2025 •

edited

Loading

kokerinks commented Jan 17, 2025

Arif-Khalid left a comment •

edited

Loading

Arif-Khalid Jan 19, 2025

Arif-Khalid Jan 19, 2025

Arif-Khalid Jan 19, 2025

Arif-Khalid Jan 19, 2025

kokerinks commented Jan 26, 2025

Eclipse-Dominator left a comment •

edited

Loading

Eclipse-Dominator Feb 3, 2025 •

edited

Loading

Fix incorrect assignee filtering #389

Are you sure you want to change the base?

Fix incorrect assignee filtering #389

Conversation

kokerinks commented Jan 12, 2025 • edited Loading

Summary:

Type of change:

Changes Made:

Proposed Commit Message:

Checklist:

kokerinks commented Jan 17, 2025

Arif-Khalid left a comment • edited Loading

Choose a reason for hiding this comment

Arif-Khalid Jan 19, 2025

Choose a reason for hiding this comment

Arif-Khalid Jan 19, 2025

Choose a reason for hiding this comment

Arif-Khalid Jan 19, 2025

Choose a reason for hiding this comment

Arif-Khalid Jan 19, 2025

Choose a reason for hiding this comment

kokerinks commented Jan 26, 2025

Eclipse-Dominator left a comment • edited Loading

Choose a reason for hiding this comment

Eclipse-Dominator Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

kokerinks commented Jan 12, 2025 •

edited

Loading

Arif-Khalid left a comment •

edited

Loading

Eclipse-Dominator left a comment •

edited

Loading

Eclipse-Dominator Feb 3, 2025 •

edited

Loading