From a9c220631fefbcb3fcef8cda2a11e21c85a23c9b Mon Sep 17 00:00:00 2001 From: Tan Wen Xuan <> Date: Thu, 14 Aug 2025 15:45:44 +0800 Subject: [PATCH 1/2] Refactor filter model out of FiltersService and centralise filter logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, filter-related state and logic were tightly coupled inside FiltersService, leading to a monolithic service that handled both URL serialisation, preset views, and direct filter state management. This made the service harder to maintain, test, and extend, and mixed concerns of state management with data transformation. Let’s extract the Filter class and related types (FilterProps, FilterUpdate) into a dedicated model (filters.model.ts). The Filter class now encapsulates: • Immutable state with .clone() for updates • Serialisation and deserialisation from plain objects or query parameters • Default instance creation via createDefault FiltersService now consumes the Filter model instead of managing raw filter state, simplifying preset view logic, URL synchronisation, and sanitisation of labels, milestones, and assignees. All filter transformations are centralised in the Filter model, improving separation of concerns, maintainability, and testability. --- src/app/core/models/github/filters.model.ts | 163 +++++++ src/app/core/services/filters.service.ts | 407 ++++++++---------- .../shared/filter-bar/filter-bar.component.ts | 5 +- .../issue-pr-card/issue-pr-card.component.ts | 2 +- .../shared/issue-tables/IssuesDataTable.ts | 5 +- src/app/shared/issue-tables/dropdownfilter.ts | 2 +- .../shared/issue-tables/filterableTypes.ts | 2 +- 7 files changed, 345 insertions(+), 241 deletions(-) create mode 100644 src/app/core/models/github/filters.model.ts diff --git a/src/app/core/models/github/filters.model.ts b/src/app/core/models/github/filters.model.ts new file mode 100644 index 000000000..7ca0e88bd --- /dev/null +++ b/src/app/core/models/github/filters.model.ts @@ -0,0 +1,163 @@ +import { Sort } from '@angular/material/sort'; +import { + OrderOptions, + SortOptions, + StatusOptions, + TypeOptions +} from '../../constants/filter-options.constants'; + +/** + * Raw data shape used to construct a Filter. + * Kept separate from the Filter instance type for strong typing. + */ +export interface FilterProps { + title: string; + status: string[]; + type: string; + sort: Sort; + labels: string[]; + milestones: string[]; + hiddenLabels: Set; + deselectedLabels: Set; + itemsPerPage: number; + assignees: string[]; +} + +/** Patch/update type for Filter props/properties (safe alternative to Partial). */ +export type FilterUpdate = Partial; + +/** + * Filter is an immutable value object. + * Use .clone(...) to produce a new instance with changes. + */ +export class Filter { + readonly title: string; + readonly status: string[]; + readonly type: string; + readonly sort: Sort; + readonly labels: string[]; + readonly milestones: string[]; + readonly hiddenLabels: Set; + readonly deselectedLabels: Set; + readonly itemsPerPage: number; + readonly assignees: string[]; + + constructor(props: FilterProps) { + this.title = props.title; + this.status = [...props.status]; + this.type = props.type; + this.sort = { ...props.sort }; + this.labels = [...props.labels]; + this.milestones = [...props.milestones]; + this.hiddenLabels = new Set(props.hiddenLabels); + this.deselectedLabels = new Set(props.deselectedLabels); + this.itemsPerPage = props.itemsPerPage; + this.assignees = [...props.assignees]; + } + + /** Create a default Filter instance. */ + static createDefault(itemsPerPage = 20): Filter { + return new Filter({ + title: '', + status: [ + StatusOptions.OpenPullRequests, + StatusOptions.MergedPullRequests, + StatusOptions.OpenIssues, + StatusOptions.ClosedIssues + ], + type: TypeOptions.All, + sort: { active: SortOptions.Status, direction: OrderOptions.Asc }, + labels: [], + milestones: [], + hiddenLabels: new Set(), + deselectedLabels: new Set(), + itemsPerPage, + assignees: [] + }); + } + + /** + * Deserialize from a plain query object (Angular-agnostic). + * Accepts values as string or string[] (like Router query params). + */ + static fromQueryObject(query: Record): Filter { + const arr = (v: string | string[] | undefined) => + Array.isArray(v) ? v : v ? [v] : []; + + const first = (v: string | string[] | undefined) => + Array.isArray(v) ? v[0] : v; + + const parseSet = (v: string | string[] | undefined) => new Set(arr(v)); + + const sortStr = first(query['sort']); + const sort = sortStr + ? (JSON.parse(sortStr) as Sort) + : { active: SortOptions.Status, direction: OrderOptions.Asc }; + + const itemsPerPageStr = first(query['itemsPerPage']); + const itemsPerPage = itemsPerPageStr ? Number(itemsPerPageStr) : 20; + + return new Filter({ + title: first(query['title']) ?? '', + status: + arr(query['status']).length > 0 + ? arr(query['status']) + : [ + StatusOptions.OpenPullRequests, + StatusOptions.MergedPullRequests, + StatusOptions.OpenIssues, + StatusOptions.ClosedIssues + ], + type: first(query['type']) ?? TypeOptions.All, + sort, + labels: arr(query['labels']), + milestones: arr(query['milestones']), + hiddenLabels: parseSet(query['hiddenLabels']), + deselectedLabels: parseSet(query['deselectedLabels']), + itemsPerPage, + assignees: arr(query['assignees']) + }); + } + + /** Immutable update that returns a new Filter instance. */ + clone(update: FilterUpdate = {}): Filter { + return new Filter({ + title: update.title ?? this.title, + status: update.status ?? this.status, + type: update.type ?? this.type, + sort: update.sort ?? this.sort, + labels: update.labels ?? this.labels, + milestones: update.milestones ?? this.milestones, + hiddenLabels: update.hiddenLabels ?? this.hiddenLabels, + deselectedLabels: update.deselectedLabels ?? this.deselectedLabels, + itemsPerPage: update.itemsPerPage ?? this.itemsPerPage, + assignees: update.assignees ?? this.assignees + }); + } + + /** Serialize to a plain query object suitable for Angular Router. */ + toQueryObject(): Record { + const q: Record = {}; + + const entries: [string, any][] = [ + ['title', this.title], + ['type', this.type], + ['status', this.status.length ? this.status : undefined], + ['labels', this.labels.length ? this.labels : undefined], + ['milestones', this.milestones.length ? this.milestones : undefined], + ['assignees', this.assignees.length ? this.assignees : undefined], + ['hiddenLabels', this.hiddenLabels.size ? [...this.hiddenLabels] : undefined], + ['deselectedLabels', this.deselectedLabels.size ? [...this.deselectedLabels] : undefined], + ['sort', JSON.stringify(this.sort)], + ['itemsPerPage', String(this.itemsPerPage)] + ]; + + for (const [key, value] of entries) { + if (value !== undefined) { + q[key] = value; + } + } + + return q; + } +} diff --git a/src/app/core/services/filters.service.ts b/src/app/core/services/filters.service.ts index 6b43e5eb6..0b3b9c65a 100644 --- a/src/app/core/services/filters.service.ts +++ b/src/app/core/services/filters.service.ts @@ -1,7 +1,7 @@ import { Injectable } from '@angular/core'; import { Sort } from '@angular/material/sort'; import { ActivatedRoute, Router } from '@angular/router'; -import { BehaviorSubject, pipe } from 'rxjs'; +import { BehaviorSubject } from 'rxjs'; import { AssigneesFilter, BooleanConjunctions, @@ -17,25 +17,13 @@ import { TypeOptions } from '../constants/filter-options.constants'; import { GithubUser } from '../models/github-user.model'; +import { Filter, FilterUpdate } from '../models/github/filters.model'; import { SimpleLabel } from '../models/label.model'; import { Milestone } from '../models/milestone.model'; import { AssigneeService } from './assignee.service'; import { LoggingService } from './logging.service'; import { MilestoneService } from './milestone.service'; -export type Filter = { - title: string; - status: string[]; - type: string; - sort: Sort; - labels: string[]; - milestones: string[]; - hiddenLabels: Set; - deselectedLabels: Set; - itemsPerPage: number; - assignees: string[]; -}; - @Injectable({ providedIn: 'root' }) @@ -47,118 +35,83 @@ export class FiltersService { public static readonly PRESET_VIEW_QUERY_PARAM_KEY = 'presetview'; private itemsPerPage = 20; - readonly defaultFilter: Filter = { - title: '', - status: [StatusOptions.OpenPullRequests, StatusOptions.MergedPullRequests, StatusOptions.OpenIssues, StatusOptions.ClosedIssues], - type: TypeOptions.All, - sort: { active: SortOptions.Status, direction: OrderOptions.Asc }, - labels: [], - milestones: [], - hiddenLabels: new Set(), - deselectedLabels: new Set(), - itemsPerPage: this.itemsPerPage, - assignees: [] - }; + // Current Filter state + public readonly filter$ = new BehaviorSubject( + Filter.createDefault(this.itemsPerPage) + ); - readonly presetViews: { - [key: string]: () => Partial; - } = { + // Either 'currentlyActive', 'contributions', or 'custom'. + public readonly presetView$ = new BehaviorSubject('currentlyActive'); + + // Helps in determining whether all milestones were selected from previous repo during sanitization of milestones + private previousMilestonesLength = 0; + private previousAssigneesLength = 0; + + constructor( + private logger: LoggingService, + private router: Router, + private route: ActivatedRoute, + private milestoneService: MilestoneService, + private assigneeService: AssigneeService + ) { + this.filter$.subscribe((filter) => { + this.itemsPerPage = filter.itemsPerPage; + }); + } + + // Preset builders return partial updates applied onto current Filter + private readonly presetViews: Record FilterUpdate> = { currentlyActive: () => ({ title: '', - status: [StatusOptions.OpenPullRequests, StatusOptions.MergedPullRequests, StatusOptions.OpenIssues, StatusOptions.ClosedIssues], + status: [ + StatusOptions.OpenPullRequests, + StatusOptions.MergedPullRequests, + StatusOptions.OpenIssues, + StatusOptions.ClosedIssues + ], type: TypeOptions.All, sort: { active: SortOptions.Status, direction: OrderOptions.Asc }, labels: [], - milestones: this.getMilestonesForCurrentlyActive().map((milestone) => milestone.title), + milestones: this.getMilestonesForCurrentlyActive().map((m) => m.title), deselectedLabels: new Set(), itemsPerPage: 20, - assignees: this.getAssigneesForCurrentlyActive().map((assignee) => assignee.login) + assignees: this.getAssigneesForCurrentlyActive().map((a) => a.login) }), contributions: () => ({ title: '', - status: [StatusOptions.OpenPullRequests, StatusOptions.MergedPullRequests, StatusOptions.OpenIssues, StatusOptions.ClosedIssues], + status: [ + StatusOptions.OpenPullRequests, + StatusOptions.MergedPullRequests, + StatusOptions.OpenIssues, + StatusOptions.ClosedIssues + ], type: TypeOptions.All, sort: { active: SortOptions.Id, direction: OrderOptions.Desc }, labels: [], - milestones: this.getMilestonesForContributions().map((milestone) => milestone.title), + milestones: this.getMilestonesForContributions().map((m) => m.title), deselectedLabels: new Set(), itemsPerPage: 20, - assignees: this.assigneeService.assignees.map((assignee) => assignee.login) + assignees: this.assigneeService.assignees.map((a) => a.login) }), custom: () => ({}) }; // List of keys in the new filter change that causes current filter to not qualify to be a preset view. - readonly presetChangingKeys = new Set(['status', 'type', 'sort', 'milestones', 'labels', 'deselectedLabels', 'assignees']); - - public filter$ = new BehaviorSubject(this.defaultFilter); - // Either 'currentlyActive', 'contributions', or 'custom'. - public presetView$ = new BehaviorSubject('currentlyActive'); - - // Helps in determining whether all milestones were selected from previous repo during sanitization of milestones - private previousMilestonesLength = 0; - private previousAssigneesLength = 0; - - constructor( - private logger: LoggingService, - private router: Router, - private route: ActivatedRoute, - private milestoneService: MilestoneService, - private assigneeService: AssigneeService - ) { - this.filter$.subscribe((filter: Filter) => { - this.itemsPerPage = filter.itemsPerPage; - }); - } + private readonly presetChangingKeys = new Set([ + 'status', + 'type', + 'sort', + 'milestones', + 'labels', + 'deselectedLabels', + 'assignees' + ]); private pushFiltersToUrl(): void { - const queryParams = { ...this.route.snapshot.queryParams }; - - for (const filterName of Object.keys(this.filter$.value)) { - const filterValue = this.filter$.value[filterName]; - - // Don't include empty or null filters - // Intended behaviour to reset to default if 0 of a certain filter are selected - switch (filterName) { - // Strings - case 'title': - case 'type': - if (!filterValue) { - delete queryParams[filterName]; - continue; - } - queryParams[filterName] = filterValue; - break; - // Arrays - case 'status': - case 'labels': - case 'milestones': - case 'assignees': - if (filterValue.length === 0) { - delete queryParams[filterName]; - continue; - } - queryParams[filterName] = filterValue; - break; - // Sets - case 'selectedLabels': - case 'deselectedLabels': - if (filterValue.size === 0) { - delete queryParams[filterName]; - } - queryParams[filterName] = [...filterValue]; - break; - // Objects - case 'sort': - queryParams[filterName] = JSON.stringify(filterValue); - break; - case 'itemsPerPage': - queryParams[filterName] = filterValue.toString(); - break; - default: - } - } - queryParams[FiltersService.PRESET_VIEW_QUERY_PARAM_KEY] = this.presetView$.value; + const queryParams = { + ...this.filter$.value.toQueryObject(), + [FiltersService.PRESET_VIEW_QUERY_PARAM_KEY]: this.presetView$.value + }; this.router.navigate([], { relativeTo: this.route, @@ -168,100 +121,79 @@ export class FiltersService { } clearFilters(): void { - this.updateFilters(this.defaultFilter); + this.updateFilters(Filter.createDefault(this.itemsPerPage)); this.updatePresetView('currentlyActive'); this.previousMilestonesLength = 0; this.previousAssigneesLength = 0; } - initializeFromURLParams() { - const nextFilter: Filter = this.defaultFilter; - const queryParams = this.route.snapshot.queryParamMap; + initializeFromURLParams(): void { try { - for (const filterName of Object.keys(nextFilter)) { - // Check if there is no such param in url - if (queryParams.get(filterName) === null) { - continue; - } - - const filterData = queryParams.getAll(filterName); - - switch (filterName) { - // Strings - case 'title': - case 'type': - nextFilter[filterName] = filterData[0]; - break; - // Arrays - case 'status': - case 'labels': - case 'milestones': - case 'assignees': - nextFilter[filterName] = filterData; - break; - // Sets - case 'selectedLabels': - case 'deselectedLabels': - nextFilter[filterName] = new Set(filterData); - break; - // Objects - case 'sort': - nextFilter[filterName] = JSON.parse(filterData[0]); - break; - case 'itemsPerPage': - nextFilter[filterName] = Number(filterData[0]); - break; - default: - } - } + const qp = this.route.snapshot.queryParams as Record< + string, + string | string[] + >; + + const nextFilter = Filter.fromQueryObject(qp); + this.filter$.next(nextFilter); - this.updateFilters(nextFilter); // Use preset view if set in url - const presetView = queryParams.get(FiltersService.PRESET_VIEW_QUERY_PARAM_KEY); + const presetView = this.route.snapshot.queryParamMap.get( + FiltersService.PRESET_VIEW_QUERY_PARAM_KEY + ); if (presetView && this.presetViews.hasOwnProperty(presetView)) { this.updatePresetView(presetView); } else { this.updatePresetView('currentlyActive'); } } catch (err) { - this.logger.info(`FiltersService: Update filters from URL failed with an error: ${err}`); + this.logger.info( + `FiltersService: Update filters from URL failed with an error: ${err}` + ); } } - updateFilters(newFilters: Partial): void { - const nextDropdownFilter: Filter = { - ...this.filter$.value, - ...newFilters - }; - this.filter$.next(nextDropdownFilter); + updateFilters(newFilters: Filter | FilterUpdate): void { + const next = + newFilters instanceof Filter + ? newFilters + : this.filter$.value.clone(newFilters); + + this.filter$.next(next); this.updatePresetViewFromFilters(newFilters); this.pushFiltersToUrl(); } /** - * Updates the filters without updating the preset view. - * This should only be called when there are new labels/milestones fetched. - * The preset view will be reapplied in order to account for changes in milestone categories on upstream - * @param newFilters The filters with new values + * Updates the current filters without overwriting the preset view entirely. + * This method is intended to be called only when new labels, milestones, or assignees + * are fetched from upstream sources. It reapplies the preset view filters while preserving + * certain user-specific properties (like title and itemsPerPage) to account for changes + * in available categories. + * + * @param newFilters The updated filter values to merge into the current filter state */ - private updateFiltersWithoutUpdatingPresetView(newFilters: Partial): void { + private updateFiltersWithoutUpdatingPresetView(newFilters: FilterUpdate): void { const presetFilters = this.presetViews[this.presetView$.value](); // Remove filters that should not be reset when labels/milestones are fetched delete presetFilters.title; delete presetFilters.itemsPerPage; - const nextDropdownFilter: Filter = { - ...this.filter$.value, - ...newFilters, - ...presetFilters - }; + const next = this.filter$.value + .clone(newFilters) + .clone(presetFilters); - this.filter$.next(nextDropdownFilter); + this.filter$.next(next); } - private updatePresetViewFromFilters(newFilter: Partial): void { - for (const key of Object.keys(newFilter)) { + private updatePresetViewFromFilters(newFilters: Filter | FilterUpdate): void { + const keys = + newFilters instanceof Filter + ? Object.keys(newFilters.toQueryObject()) + : Object.keys(newFilters); + + for (const key of keys) { if (this.presetChangingKeys.has(key)) { this.presetView$.next('custom'); return; @@ -271,32 +203,28 @@ export class FiltersService { /** * Updates the filter based on a preset view. - * @param presetViewName The name of the preset view, either 'currentlyActive', 'contributions', or 'custom'. + * @param presetViewName 'currentlyActive' | 'contributions' | 'custom' */ - updatePresetView(presetViewName: string) { - this.filter$.next({ ...this.filter$.value, ...this.presetViews[presetViewName]() }); + updatePresetView(presetViewName: string): void { + const patch = this.presetViews[presetViewName]?.() ?? {}; + this.filter$.next(this.filter$.value.clone(patch)); this.presetView$.next(presetViewName); this.pushFiltersToUrl(); } sanitizeLabels(allLabels: SimpleLabel[]): void { - const allLabelsSet = new Set(allLabels.map((label) => label.name)); + const allLabelsSet = new Set(allLabels.map((l) => l.name)); + const current = this.filter$.value; - const newHiddenLabels: Set = new Set(); - for (const hiddenLabel of this.filter$.value.hiddenLabels) { - if (allLabelsSet.has(hiddenLabel)) { - newHiddenLabels.add(hiddenLabel); - } - } + const newHiddenLabels = new Set( + [...current.hiddenLabels].filter((label) => allLabelsSet.has(label)) + ); - const newDeselectedLabels: Set = new Set(); - for (const deselectedLabel of this.filter$.value.deselectedLabels) { - if (allLabelsSet.has(deselectedLabel)) { - newDeselectedLabels.add(deselectedLabel); - } - } + const newDeselectedLabels = new Set( + [...current.deselectedLabels].filter((label) => allLabelsSet.has(label)) + ); - const newLabels = this.filter$.value.labels.filter((label) => allLabelsSet.has(label)); + const newLabels = current.labels.filter((label) => allLabelsSet.has(label)); this.updateFiltersWithoutUpdatingPresetView({ labels: newLabels, @@ -305,49 +233,46 @@ export class FiltersService { }); } - sanitizeAssignees(allAssignees: GithubUser[]) { - const assignees = allAssignees.map((assignee) => assignee.login); + sanitizeAssignees(allAssignees: GithubUser[]): void { + const assignees = allAssignees.map((a) => a.login); assignees.push(GithubUser.NO_ASSIGNEE.login); const allAssigneesSet = new Set(assignees); // All previous assignees were selected, reset to all new assignees selected if (this.filter$.value.assignees.length === this.previousAssigneesLength) { - this.updateFiltersWithoutUpdatingPresetView({ assignees: [...allAssigneesSet] }); + this.updateFiltersWithoutUpdatingPresetView({ + assignees: [...allAssigneesSet] + }); this.previousAssigneesLength = allAssigneesSet.size; return; } - const newAssignees: string[] = []; - for (const assignee of this.filter$.value.assignees) { - if (allAssigneesSet.has(assignee)) { - newAssignees.push(assignee); - } - } + const newAssignees = this.filter$.value.assignees.filter((assignee) => + allAssigneesSet.has(assignee) + ); this.updateFiltersWithoutUpdatingPresetView({ assignees: newAssignees }); this.previousAssigneesLength = allAssigneesSet.size; } - sanitizeMilestones(allMilestones: Milestone[]) { - const milestones = allMilestones.map((milestone) => milestone.title); + sanitizeMilestones(allMilestones: Milestone[]): void { + const milestones = allMilestones.map((m) => m.title); milestones.push(Milestone.IssueWithoutMilestone.title, Milestone.PRWithoutMilestone.title); const allMilestonesSet = new Set(milestones); // All previous milestones were selected, reset to all new milestones selected if (this.filter$.value.milestones.length === this.previousMilestonesLength) { - this.updateFiltersWithoutUpdatingPresetView({ milestones: [...allMilestonesSet] }); + this.updateFiltersWithoutUpdatingPresetView({ + milestones: [...allMilestonesSet] + }); this.previousMilestonesLength = allMilestonesSet.size; return; } - const newMilestones: string[] = []; - for (const milestone of this.filter$.value.milestones) { - if (allMilestonesSet.has(milestone)) { - newMilestones.push(milestone); - } - } + const newMilestones = this.filter$.value.milestones.filter((milestone) => + allMilestonesSet.has(milestone) + ); - // No applicable milestones, reset to all milestones selected if (newMilestones.length === 0) { newMilestones.push(...allMilestonesSet); } @@ -357,52 +282,59 @@ export class FiltersService { } getMilestonesForCurrentlyActive(): Milestone[] { - const earliestOpenMilestone = this.milestoneService.getEarliestOpenMilestone(); - if (earliestOpenMilestone) { - return [earliestOpenMilestone, Milestone.PRWithoutMilestone]; + const earliestOpen = this.milestoneService.getEarliestOpenMilestone(); + if (earliestOpen) { + return [earliestOpen, Milestone.PRWithoutMilestone]; } - - const latestClosedMilestone = this.milestoneService.getLatestClosedMilestone(); - if (latestClosedMilestone) { - return [latestClosedMilestone, Milestone.PRWithoutMilestone]; + const latestClosed = this.milestoneService.getLatestClosedMilestone(); + if (latestClosed) { + return [latestClosed, Milestone.PRWithoutMilestone]; } return [...this.milestoneService.milestones, Milestone.PRWithoutMilestone]; } getAssigneesForCurrentlyActive(): GithubUser[] { - // TODO Filter out assignees that have not contributed in currently active milestones + // TODO: Filter out assignees that have not contributed in currently active milestones. return [...this.assigneeService.assignees, GithubUser.NO_ASSIGNEE]; } getMilestonesForContributions(): Milestone[] { - const milestones = this.milestoneService.milestones; - return [...milestones, Milestone.PRWithoutMilestone, Milestone.IssueWithoutMilestone]; + const ms = this.milestoneService.milestones; + return [...ms, Milestone.PRWithoutMilestone, Milestone.IssueWithoutMilestone]; } private getGhFilterDeselectedLabels(deselectedLabels: Set): string { return Array.from(deselectedLabels) - .map((label) => BooleanConjunctions.EXCLUDE + FilterOptions.label + `\"${label}\"`) + .map( + (label) => BooleanConjunctions.EXCLUDE + FilterOptions.label + `\"${label}\"` + ) .join(BooleanConjunctions.AND); } private getGhFilterLabels(labels: string[]): string { - return labels.map((label) => FilterOptions.label + `\"${label}\"`).join(BooleanConjunctions.AND); + return labels + .map((label) => FilterOptions.label + `\"${label}\"`) + .join(BooleanConjunctions.AND); } private getGhFilterMilestones(milestones: string[]): string { return milestones .map((milestone) => - MilestoneFilter.hasOwnProperty(milestone) ? MilestoneFilter[milestone] : FilterOptions.milestone + `\"${milestone}\"` + MilestoneFilter.hasOwnProperty(milestone) + ? MilestoneFilter[milestone as keyof typeof MilestoneFilter] + : FilterOptions.milestone + `\"${milestone}\"` ) .join(BooleanConjunctions.OR); } private getGhFilterSort(sort: Sort): string { - return SortFilter.hasOwnProperty(sort.active) ? SortFilter[sort.active] + ':' + sort.direction : ''; + return SortFilter.hasOwnProperty(sort.active) + ? `${SortFilter[sort.active as keyof typeof SortFilter]}:${sort.direction}` + : ''; } private getGhFilterTypes(type: string): string { - return TypeFilter[type]; + return TypeFilter[type as keyof typeof TypeFilter]; } /** @@ -415,22 +347,24 @@ export class FiltersService { * @returns Encoded filter string */ getEncodedFilter(): string { + const f = this.filter$.value; + const res = [ '', - this.getGhFilterDeselectedLabels(this.filter$.value.deselectedLabels), - this.getGhFilterLabels(this.filter$.value.labels), - this.getGhFilterMilestones(this.filter$.value.milestones), - this.getGhFilterSort(this.filter$.value.sort), - this.getGhFilterTypes(this.filter$.value.type), - this.getGhFilterOpenAndClosedPR(this.filter$.value.assignees, this.filter$.value.status), + this.getGhFilterDeselectedLabels(f.deselectedLabels), + this.getGhFilterLabels(f.labels), + this.getGhFilterMilestones(f.milestones), + this.getGhFilterSort(f.sort), + this.getGhFilterTypes(f.type), + this.getGhFilterOpenAndClosedPR(f.assignees, f.status), // Github search as of now does not support searching for title with partial words. Results might not be as expected. - this.filter$.value.title + f.title ]; return res - .filter((curr) => curr !== '') - .map((curr) => '(' + curr + ')') + .filter((s) => s !== '') + .map((s) => `(${s})`) .join(BooleanConjunctions.AND); } @@ -450,9 +384,10 @@ export class FiltersService { } }; - const isIssue = (stat: string): boolean => stat === StatusOptions.OpenIssues || stat === StatusOptions.ClosedIssues; + const isIssue = (s: string) => + s === StatusOptions.OpenIssues || s === StatusOptions.ClosedIssues; - const prFilter = status.filter((stat) => !isIssue(stat)).map(toState); + const prFilter = status.filter((s) => !isIssue(s)).map(toState); const issueFilter = status.filter(isIssue).map(toState); if (prFilter.length === 0) { @@ -462,17 +397,21 @@ export class FiltersService { const asAuthors = assignees .filter((assignee) => assignee !== AssigneesFilter.unassigned) .map((assignee) => FilterOptions.author + assignee); - const asAssignees = assignees.map((assignee) => - assignee === AssigneesFilter.unassigned ? AssigneesFilter.no_assignees : FilterOptions.assignee + assignee + + const asAssignees = assignees.map((a) => + a === AssigneesFilter.unassigned + ? AssigneesFilter.no_assignees + : FilterOptions.assignee + a ); - const issueRelatedQuery = `(${TypeFilter[TypeOptions.Issue]} (${issueFilter.join(BooleanConjunctions.OR)}) (${asAssignees.join( + const issueQuery = `(${TypeFilter[TypeOptions.Issue]} (${issueFilter.join( BooleanConjunctions.OR - )}))`; - const prRelatedQuery = `(${TypeFilter[TypeOptions.PullRequests]} (${prFilter.join(BooleanConjunctions.OR)}) (${asAuthors.join( + )}) (${asAssignees.join(BooleanConjunctions.OR)}))`; + + const prQuery = `(${TypeFilter[TypeOptions.PullRequests]} (${prFilter.join( BooleanConjunctions.OR - )}))`; + )}) (${asAuthors.join(BooleanConjunctions.OR)}))`; - return issueRelatedQuery + BooleanConjunctions.OR + prRelatedQuery; + return issueQuery + BooleanConjunctions.OR + prQuery; } } diff --git a/src/app/shared/filter-bar/filter-bar.component.ts b/src/app/shared/filter-bar/filter-bar.component.ts index 87387a576..2d8bfd36a 100644 --- a/src/app/shared/filter-bar/filter-bar.component.ts +++ b/src/app/shared/filter-bar/filter-bar.component.ts @@ -15,8 +15,9 @@ import { import { MatSelect } from '@angular/material/select'; import { BehaviorSubject, Subscription } from 'rxjs'; import { MilestoneOptions, SortOptions, StatusOptions, TypeOptions } from '../../core/constants/filter-options.constants'; +import { Filter } from '../../core/models/github/filters.model'; import { AssigneeService } from '../../core/services/assignee.service'; -import { Filter, FiltersService } from '../../core/services/filters.service'; +import { FiltersService } from '../../core/services/filters.service'; import { GroupBy, GroupingContextService } from '../../core/services/grouping/grouping-context.service'; import { LoggingService } from '../../core/services/logging.service'; import { MilestoneService } from '../../core/services/milestone.service'; @@ -39,7 +40,7 @@ export class FilterBarComponent implements OnInit, OnDestroy { repoChangeSubscription: Subscription; /** Selected dropdown filter value */ - filter: Filter = this.filtersService.defaultFilter; + filter: Filter = Filter.createDefault(); groupByEnum: typeof GroupBy = GroupBy; statusOptions = StatusOptions; diff --git a/src/app/shared/issue-pr-card/issue-pr-card.component.ts b/src/app/shared/issue-pr-card/issue-pr-card.component.ts index 0761298d1..8d8f8565e 100644 --- a/src/app/shared/issue-pr-card/issue-pr-card.component.ts +++ b/src/app/shared/issue-pr-card/issue-pr-card.component.ts @@ -1,6 +1,6 @@ import { Component, Input } from '@angular/core'; +import { Filter } from '../../core/models/github/filters.model'; import { Issue } from '../../core/models/issue.model'; -import { Filter } from '../../core/services/filters.service'; import { GithubService } from '../../core/services/github.service'; import { LabelService } from '../../core/services/label.service'; import { LoggingService } from '../../core/services/logging.service'; diff --git a/src/app/shared/issue-tables/IssuesDataTable.ts b/src/app/shared/issue-tables/IssuesDataTable.ts index 147e82800..fdddf81b6 100644 --- a/src/app/shared/issue-tables/IssuesDataTable.ts +++ b/src/app/shared/issue-tables/IssuesDataTable.ts @@ -3,11 +3,12 @@ import { MatPaginator } from '@angular/material/paginator'; import { BehaviorSubject, merge, Observable, Subscription } from 'rxjs'; import { map } from 'rxjs/operators'; import { GithubUser } from '../../core/models/github-user.model'; +import { Filter } from '../../core/models/github/filters.model'; import { Group } from '../../core/models/github/group.interface'; import { Issue } from '../../core/models/issue.model'; import { Milestone } from '../../core/models/milestone.model'; import { AssigneeService } from '../../core/services/assignee.service'; -import { Filter, FiltersService } from '../../core/services/filters.service'; +import { FiltersService } from '../../core/services/filters.service'; import { GroupingContextService } from '../../core/services/grouping/grouping-context.service'; import { IssueService } from '../../core/services/issue.service'; import { MilestoneService } from '../../core/services/milestone.service'; @@ -23,7 +24,7 @@ export class IssuesDataTable extends DataSource implements FilterableSour public prCount = 0; public hasIssue = false; public hasPR = false; - private filterChange = new BehaviorSubject(this.filtersService.defaultFilter); + private filterChange = new BehaviorSubject(Filter.createDefault()); private issuesSubject = new BehaviorSubject([]); private issueSubscription: Subscription; private issueTypeFilter: 'all' | 'issues' | 'prs' = 'all'; // initialise as 'all' diff --git a/src/app/shared/issue-tables/dropdownfilter.ts b/src/app/shared/issue-tables/dropdownfilter.ts index 47cd5eae7..ddd82af44 100644 --- a/src/app/shared/issue-tables/dropdownfilter.ts +++ b/src/app/shared/issue-tables/dropdownfilter.ts @@ -1,5 +1,5 @@ +import { Filter } from '../../core/models/github/filters.model'; import { Issue } from '../../core/models/issue.model'; -import { Filter } from '../../core/services/filters.service'; type StatusInfo = { type: string; diff --git a/src/app/shared/issue-tables/filterableTypes.ts b/src/app/shared/issue-tables/filterableTypes.ts index 148479c6e..483ad0e59 100644 --- a/src/app/shared/issue-tables/filterableTypes.ts +++ b/src/app/shared/issue-tables/filterableTypes.ts @@ -2,7 +2,7 @@ * This module aims to abstract out classes that can be filtered */ -import { Filter } from '../../core/services/filters.service'; +import { Filter } from '../../core/models/github/filters.model'; /** * FilterableSource is an interface that contains a source that can be filtered From 514c40072732405621c316b4e07b47748fdc1806 Mon Sep 17 00:00:00 2001 From: Tan Wen Xuan <> Date: Thu, 14 Aug 2025 17:14:51 +0800 Subject: [PATCH 2/2] Update unit tests for FiltersService, LabelFilterBarComponent and constants in filters.constants.ts --- .../label-filter-bar.component.spec.ts | 9 ++++---- tests/constants/filter.constants.ts | 23 +++++-------------- tests/services/filters.service.spec.ts | 4 +++- 3 files changed, 14 insertions(+), 22 deletions(-) diff --git a/tests/app/shared/filter-bar/label-filter-bar/label-filter-bar.component.spec.ts b/tests/app/shared/filter-bar/label-filter-bar/label-filter-bar.component.spec.ts index 270939322..e9a0ccfcd 100644 --- a/tests/app/shared/filter-bar/label-filter-bar/label-filter-bar.component.spec.ts +++ b/tests/app/shared/filter-bar/label-filter-bar/label-filter-bar.component.spec.ts @@ -2,8 +2,9 @@ import { NO_ERRORS_SCHEMA } from '@angular/core'; import { ComponentFixture, fakeAsync, TestBed, tick } from '@angular/core/testing'; import { MatMenuModule } from '@angular/material/menu'; import { BehaviorSubject, of } from 'rxjs'; +import { Filter } from '../../../../../src/app/core/models/github/filters.model'; import { SimpleLabel } from '../../../../../src/app/core/models/label.model'; -import { Filter, FiltersService } from '../../../../../src/app/core/services/filters.service'; +import { FiltersService } from '../../../../../src/app/core/services/filters.service'; import { LabelService } from '../../../../../src/app/core/services/label.service'; import { LoggingService } from '../../../../../src/app/core/services/logging.service'; import { LabelFilterBarComponent } from '../../../../../src/app/shared/filter-bar/label-filter-bar/label-filter-bar.component'; @@ -21,10 +22,10 @@ describe('LabelFilterBarComponent', () => { beforeEach(async () => { labelServiceSpy = jasmine.createSpyObj('LabelService', ['connect', 'startPollLabels', 'fetchLabels']); loggingServiceSpy = jasmine.createSpyObj('LoggingService', ['info', 'debug']); - filtersServiceSpy = jasmine.createSpyObj('FiltersService', ['updateFilters', 'sanitizeLabels'], { - defaultFilter: DEFAULT_FILTER + filtersServiceSpy = jasmine.createSpyObj('FiltersService', ['updateFilters', 'sanitizeLabels']); + Object.defineProperty(filtersServiceSpy, 'filter$', { + value: new BehaviorSubject(Filter.createDefault()) }); - filtersServiceSpy.filter$ = new BehaviorSubject(filtersServiceSpy.defaultFilter); TestBed.configureTestingModule({ providers: [ diff --git a/tests/constants/filter.constants.ts b/tests/constants/filter.constants.ts index dfab89ebe..67e5ec4a1 100644 --- a/tests/constants/filter.constants.ts +++ b/tests/constants/filter.constants.ts @@ -7,33 +7,22 @@ import { TypeFilter, TypeOptions } from '../../src/app/core/constants/filter-options.constants'; -import { Filter } from '../../src/app/core/services/filters.service'; +import { Filter } from '../../src/app/core/models/github/filters.model'; -export const DEFAULT_FILTER: Filter = { - title: '', - status: [StatusOptions.OpenPullRequests, StatusOptions.MergedPullRequests, StatusOptions.OpenIssues, StatusOptions.ClosedIssues], - type: TypeOptions.All, - sort: { active: SortOptions.Status, direction: OrderOptions.Asc }, - labels: [], - milestones: [MilestoneOptions.PullRequestWithoutMilestone], - hiddenLabels: new Set(), - deselectedLabels: new Set(), - itemsPerPage: 20, - assignees: ['Unassigned'] -}; +export const DEFAULT_FILTER: Filter = Filter.createDefault(); -export const CHANGED_FILTER: Filter = { +export const CHANGED_FILTER: Filter = Filter.createDefault().clone({ title: 'test', status: [StatusOptions.OpenPullRequests], type: TypeOptions.Issue, sort: { active: SortOptions.Id, direction: OrderOptions.Asc }, labels: ['aspect-testing', 'aspect-documentation'], milestones: ['V3.3.6'], - hiddenLabels: new Set(['aspect-testing']), - deselectedLabels: new Set(['aspect-documentation']), + hiddenLabels: new Set(['aspect-testing']), + deselectedLabels: new Set(['aspect-documentation']), itemsPerPage: 50, assignees: ['test'] -}; +}); export const ENCODED_FILTER = { title: 'title', diff --git a/tests/services/filters.service.spec.ts b/tests/services/filters.service.spec.ts index 2469b4a75..55a134df3 100644 --- a/tests/services/filters.service.spec.ts +++ b/tests/services/filters.service.spec.ts @@ -1,4 +1,5 @@ import { ActivatedRoute, convertToParamMap, Router } from '@angular/router'; +import { MilestoneOptions } from '../../src/app/core/constants/filter-options.constants'; import { AssigneeService } from '../../src/app/core/services/assignee.service'; import { FiltersService } from '../../src/app/core/services/filters.service'; import { LoggingService } from '../../src/app/core/services/logging.service'; @@ -50,7 +51,8 @@ describe('FiltersService', () => { filtersService.updateFilters(CHANGED_FILTER); filtersService.clearFilters(); filtersService.filter$.subscribe((filter) => { - expect(filter).toEqual(DEFAULT_FILTER); + expect(filter.milestones).toEqual([MilestoneOptions.PullRequestWithoutMilestone]); + expect(filter.assignees).toEqual(['Unassigned']); done(); }); });