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.

Sign up for GitHub

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 incorrect assignee filtering #389

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix incorrect assignee filtering #389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
84e0355
Replace missing assignee filter
kokerinks Jan 12, 2025
b0bc7de
Fix assignee filter showing additional columns
kokerinks Jan 12, 2025
4811b2c
Merge branch 'main' into fix-assignee-filtering
kokerinks Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/app/core/services/filters.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,4 +359,8 @@ export class FiltersService {
// TODO Filter out assignees that have not contributed in currently active milestones
return [...this.assigneeService.assignees, GithubUser.NO_ASSIGNEE];
}

getAssignees(): GithubUser[] {
return this.assigneeService.assignees.filter((assignee) => this.filter$.value.assignees.includes(assignee.login));
}
Comment on lines +362 to +365
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}
11 changes: 9 additions & 2 deletions src/app/core/services/grouping/assignee-grouping-strategy.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Observable } from 'rxjs';
import { map } from 'rxjs/operators';
import { GithubUser } from '../../models/github-user.model';
import { Issue } from '../../models/issue.model';
import { FiltersService } from '../filters.service';
import { GithubService } from '../github.service';
import { GroupingStrategy } from './grouping-strategy.interface';

Expand All @@ -13,7 +14,7 @@ import { GroupingStrategy } from './grouping-strategy.interface';
providedIn: 'root'
})
export class AssigneeGroupingStrategy implements GroupingStrategy {
constructor(private githubService: GithubService) {}
constructor(private githubService: GithubService, private filtersService: FiltersService) {}

/**
* Retrieves data for a specific assignee.
Expand Down Expand Up @@ -45,7 +46,13 @@ export class AssigneeGroupingStrategy implements GroupingStrategy {
* hidden group list if empty.
*/
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
Comment on lines 48 to +55
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

private getDataAssignedToUser(issues: Issue[], user: GithubUser): Issue[] {
Expand Down
2 changes: 1 addition & 1 deletion src/app/core/services/grouping/group.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Contributor

@Eclipse-Dominator Eclipse-Dominator Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

this.addToHiddenGroups(target);
} else {
this.removeFromHiddenGroups(target);
Expand Down
4 changes: 3 additions & 1 deletion src/app/issues-viewer/issues-viewer.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@
*ngFor="let group of this.groupService.groups"
class="issue-table"
#card
[ngStyle]="{ display: card.isLoading || card.issueLength > 0 ? 'initial' : 'none' }"
[ngStyle]="{
display: card.isLoading || (card.issueLength > 0 && !groupService.hiddenGroups.includes(card.group)) ? 'initial' : 'none'
}"
Comment on lines +14 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

[group]="group"
[headers]="this.displayedColumns"
(issueLengthChange)="this.groupService.updateHiddenGroups($event, group)"
Expand Down
Loading