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

Status filter checkboxes #310

Merged
merged 9 commits into from
Mar 25, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
44 changes: 16 additions & 28 deletions src/app/core/services/filters.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,30 @@ import { BehaviorSubject, pipe } from 'rxjs';

export type Filter = {
title: string;
status: string;
status: string[];
type: string;
sort: Sort;
labels: string[];
milestones: string[];
hiddenLabels?: Set<string>;
};

type StatusInfo = {
type: string;
status: string;
};

/**
* Converts a status string into an info object
*/
export const statusToType = (statusString: string): StatusInfo => {
const [status, type] = statusString.split(' ');
return { status, type };
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to include a statusToType sort of function in the filters service?
The filters service should be exclusively kept for storing and updating filters in line with the SRP.
I think this characterization of information from certain filters should be done where the filters are applied i.e., in dropdownfilter.ts in this case.
I would suggest simply using the status string in dropdownfilter.ts and parsing it appropriately as part of filtering

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!


export const DEFAULT_FILTER: Filter = {
title: '',
status: 'all',
status: ['open pullrequest', 'merged pullrequest', 'closed pullrequest', 'open issue', 'closed issue'],
Copy link
Contributor

Choose a reason for hiding this comment

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

As per the description of #296, closed pullrequest should not be selected by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

type: 'all',
sort: { active: 'id', direction: 'asc' },
labels: [],
Expand All @@ -31,40 +44,15 @@ export const DEFAULT_FILTER: Filter = {
export class FiltersService {
public filter$ = new BehaviorSubject<Filter>(DEFAULT_FILTER);

private _validateFilter = pipe(this.updateStatusPairing, this.updateTypePairing);

clearFilters(): void {
this.filter$.next(DEFAULT_FILTER);
}

updateFilters(newFilters: Partial<Filter>): void {
let nextDropdownFilter: Filter = {
const nextDropdownFilter: Filter = {
...this.filter$.value,
...newFilters
};

nextDropdownFilter = this._validateFilter(nextDropdownFilter);

this.filter$.next(nextDropdownFilter);
}

/**
* Changes type to a valid, default value when an incompatible combination of type and status is encountered.
*/
updateTypePairing(dropdownFilter: Filter): Filter {
if (dropdownFilter.status === 'merged') {
dropdownFilter.type = 'pullrequest';
}
return dropdownFilter;
}

/**
* Changes status to a valid, default value when an incompatible combination of type and status is encountered.
*/
updateStatusPairing(dropdownFilter: Filter): Filter {
if (dropdownFilter.status === 'merged' && dropdownFilter.type === 'issue') {
dropdownFilter.status = 'all';
}
return dropdownFilter;
}
}
11 changes: 6 additions & 5 deletions src/app/shared/filter-bar/filter-bar.component.html
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@
<div class="dropdown-filters">
<mat-form-field appearance="standard">
<mat-label>Status</mat-label>
<mat-select [value]="this.filter.status" (selectionChange)="this.filtersService.updateFilters({ status: $event.value })">
<mat-option value="all">All</mat-option>
<mat-option value="open">Open</mat-option>
<mat-option value="closed">Closed</mat-option>
<mat-option value="merged" *ngIf="isNotFilterIssue()">Merged</mat-option>
<mat-select [value]="this.filter.status" (selectionChange)="this.filtersService.updateFilters({ status: $event.value })" multiple>
<mat-option *ngIf="isFilterPullRequest()" value="open pullrequest">Open Pull Requests</mat-option>
<mat-option *ngIf="isFilterPullRequest()" value="merged pullrequest">Merged Pull Requests</mat-option>
<mat-option *ngIf="isFilterPullRequest()" value="closed pullrequest">Closed Pull Request</mat-option>
<mat-option *ngIf="isFilterIssue()" value="open issue">Open Issues</mat-option>
<mat-option *ngIf="isFilterIssue()" value="closed issue">Closed Issues</mat-option>
</mat-select>
</mat-form-field>
<mat-form-field appearance="standard">
Expand Down
8 changes: 6 additions & 2 deletions src/app/shared/filter-bar/filter-bar.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,12 @@ export class FilterBarComponent implements OnInit, AfterViewInit, OnDestroy {
/**
* Checks if program is filtering by type issue.
*/
isNotFilterIssue() {
return this.filter.type !== 'issue';
isFilterIssue() {
return this.filter.type === 'issue' || this.filter.type === 'all';
}

isFilterPullRequest() {
return this.filter.type === 'pullrequest' || this.filter.type === 'all';
}

/**
Expand Down
17 changes: 8 additions & 9 deletions src/app/shared/issue-tables/dropdownfilter.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Issue } from '../../core/models/issue.model';
import { Filter } from '../../core/services/filters.service';
import { Filter, statusToType } from '../../core/services/filters.service';

/**
* This module serves to improve separation of concerns in IssuesDataTable.ts and IssueList.ts module by containing the logic for
Expand All @@ -11,14 +11,13 @@ export function applyDropdownFilter(filter: Filter, data: Issue[]): Issue[] {
const filteredData: Issue[] = data.filter((issue) => {
let ret = true;

if (filter.status === 'open') {
ret = ret && issue.state === 'OPEN';
} else if (filter.status === 'closed') {
// there is apparently also a status called 'all' based on github api
ret = ret && issue.state === 'CLOSED';
} else if (filter.status === 'merged') {
ret = ret && issue.state === 'MERGED';
}
// status can either be 'open', 'closed', or 'merged'
ret =
ret &&
filter.status.some((item) => {
const statusInfo = statusToType(item);
return statusInfo.status === issue.state.toLowerCase() && statusInfo.type === issue.issueOrPr.toLowerCase();
});

if (filter.type === 'issue') {
ret = ret && issue.issueOrPr === 'Issue';
Expand Down
Loading