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

Add preset views #320

Merged
merged 5 commits into from
Mar 29, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 3 additions & 1 deletion src/app/core/models/milestone.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@ export class Milestone implements Group {
static PRWithoutMilestone: Milestone = new Milestone({ title: 'PR without a milestone', state: null });
title: string;
state: string;
deadline?: Date;

constructor(milestone: { title: string; state: string }) {
constructor(milestone: { title: string; state: string; due_on?: string }) {
this.title = milestone.title;
this.state = milestone.state;
this.deadline = milestone.due_on ? new Date(milestone.due_on) : undefined;
}

public equals(other: any) {
Expand Down
109 changes: 93 additions & 16 deletions src/app/core/services/filters.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Sort } from '@angular/material/sort';
import { BehaviorSubject, pipe } from 'rxjs';
import { SimpleLabel } from '../models/label.model';
import { Milestone } from '../models/milestone.model';
import { MilestoneService } from './milestone.service';

export type Filter = {
title: string;
Expand All @@ -15,17 +16,6 @@ export type Filter = {
deselectedLabels: Set<string>;
};

export const DEFAULT_FILTER: Filter = {
title: '',
status: ['open pullrequest', 'merged pullrequest', 'open issue', 'closed issue'],
type: 'all',
sort: { active: 'id', direction: 'asc' },
labels: [],
milestones: [],
hiddenLabels: new Set<string>(),
deselectedLabels: new Set<string>()
};

@Injectable({
providedIn: 'root'
})
Expand All @@ -34,13 +24,48 @@ export const DEFAULT_FILTER: Filter = {
* Filters are subscribed to and emitted from this service
*/
export class FiltersService {
public filter$ = new BehaviorSubject<Filter>(DEFAULT_FILTER);
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
};
Comment on lines +27 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


// List of keys in the new filter change that causes current filter to not qualify to be a preset view.
readonly presetChangingKeys = new Set<string>(['status', 'type', 'milestones', 'labels', 'deselectedLabels']);

readonly defaultFilter = this.presetViews.currentlyActive;
public filter$ = new BehaviorSubject<Filter>(this.defaultFilter());
// Either 'currentlyActive', 'contributions', or 'custom'.
public presetView$ = new BehaviorSubject<string>('currentlyActive');

// Helps in determining whether all milestones were selected from previous repo during sanitization of milestones
private previousMilestonesLength = 0;

constructor(private milestoneService: MilestoneService) {}

clearFilters(): void {
this.filter$.next(DEFAULT_FILTER);
this.filter$.next(this.defaultFilter());
this.presetView$.next('currentlyActive');
this.previousMilestonesLength = 0;
}

Expand All @@ -50,6 +75,40 @@ export class FiltersService {
...newFilters
};
this.filter$.next(nextDropdownFilter);
this.updatePresetViewFromFilters(newFilters);
}

/**
* Updates the filters without updating the preset view.
* This should only be called when there are new labels/milestones.
* The preset view will be reapplied.
* @param newFilters The filters with new values
*/
private updateFiltersWithoutUpdatingPresetView(newFilters: Partial<Filter>): void {
const nextDropdownFilter: Filter = {
...this.filter$.value,
...newFilters
};
this.filter$.next(nextDropdownFilter);
this.filter$.next(this.presetViews[this.presetView$.value]());
}

private updatePresetViewFromFilters(newFilter: Partial<Filter>): void {
for (const key of Object.keys(newFilter)) {
if (this.presetChangingKeys.has(key)) {
this.presetView$.next('custom');
return;
}
}
}

/**
* Updates the filter based on a preset view.
* @param presetViewName The name of the preset view, either 'currentlyActive', 'contributions', or 'custom'.
*/
updatePresetView(presetViewName: string) {
this.filter$.next(this.presetViews[presetViewName]());
this.presetView$.next(presetViewName);
}

sanitizeLabels(allLabels: SimpleLabel[]) {
Expand All @@ -71,7 +130,11 @@ export class FiltersService {

const newLabels = this.filter$.value.labels.filter((label) => allLabelsSet.has(label));

this.updateFilters({ labels: newLabels, hiddenLabels: newHiddenLabels, deselectedLabels: newDeselectedLabels });
this.updateFiltersWithoutUpdatingPresetView({
labels: newLabels,
hiddenLabels: newHiddenLabels,
deselectedLabels: newDeselectedLabels
});
}

sanitizeMilestones(allMilestones: Milestone[]) {
Expand All @@ -81,7 +144,7 @@ export class FiltersService {

// All previous milestones were selected, reset to all new milestones selected
if (this.filter$.value.milestones.length === this.previousMilestonesLength) {
this.updateFilters({ milestones: [...allMilestonesSet] });
this.updateFiltersWithoutUpdatingPresetView({ milestones: [...allMilestonesSet] });
this.previousMilestonesLength = allMilestonesSet.size;
return;
}
Expand All @@ -98,7 +161,21 @@ export class FiltersService {
newMilestones.push(...allMilestonesSet);
}

this.updateFilters({ milestones: newMilestones });
this.updateFiltersWithoutUpdatingPresetView({ milestones: newMilestones });
this.previousMilestonesLength = allMilestonesSet.size;
}

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

const latestClosedMilestone = this.milestoneService.getLatestClosedMilestone();
if (latestClosedMilestone) {
return [latestClosedMilestone];
}

return this.milestoneService.milestones;
}
Comment on lines +168 to +180
Copy link
Contributor

Choose a reason for hiding this comment

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

Good handling of edge cases

}
40 changes: 39 additions & 1 deletion src/app/core/services/milestone.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { GithubService } from './github.service';
* from the GitHub repository for the WATcher application.
*/
export class MilestoneService {
milestones: Milestone[];
milestones: Milestone[] = [];
hasNoMilestones: boolean;

constructor(private githubService: GithubService) {}
Expand Down Expand Up @@ -45,4 +45,42 @@ export class MilestoneService {

return milestoneData;
}

/**
* Gets the open milestone with the earliest deadline.
* Returns null if there is no open milestone with deadline.
*/
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;
}
Comment on lines +53 to +85
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
9 changes: 8 additions & 1 deletion src/app/issues-viewer/card-view/card-view.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { MatPaginator } from '@angular/material/paginator';
import { Observable, Subscription } from 'rxjs';
import { Group } from '../../core/models/github/group.interface';
import { Issue } from '../../core/models/issue.model';
import { FiltersService } from '../../core/services/filters.service';
import { GroupBy, GroupingContextService } from '../../core/services/grouping/grouping-context.service';
import { IssueService } from '../../core/services/issue.service';
import { FilterableComponent, FilterableSource } from '../../shared/issue-tables/filterableTypes';
Expand Down Expand Up @@ -52,12 +53,18 @@ export class CardViewComponent implements OnInit, AfterViewInit, OnDestroy, Filt

@Output() issueLengthChange: EventEmitter<Number> = new EventEmitter<Number>();

constructor(public element: ElementRef, public issueService: IssueService, public groupingContextService: GroupingContextService) {}
constructor(
public element: ElementRef,
public issueService: IssueService,
public groupingContextService: GroupingContextService,
private filtersService: FiltersService
) {}

ngOnInit() {
this.issues = new IssuesDataTable(
this.issueService,
this.groupingContextService,
this.filtersService,
this.paginator,
this.headers,
this.group,
Expand Down
4 changes: 2 additions & 2 deletions src/app/shared/filter-bar/filter-bar.component.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { AfterViewInit, Component, Input, OnDestroy, OnInit, QueryList, ViewChild } from '@angular/core';
import { MatSelect } from '@angular/material/select';
import { BehaviorSubject, Subscription } from 'rxjs';
import { DEFAULT_FILTER, Filter, FiltersService } from '../../core/services/filters.service';
import { Filter, 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';
Expand All @@ -24,7 +24,7 @@ export class FilterBarComponent implements OnInit, OnDestroy {
repoChangeSubscription: Subscription;

/** Selected dropdown filter value */
filter: Filter = DEFAULT_FILTER;
filter: Filter = this.filtersService.defaultFilter();

groupByEnum: typeof GroupBy = GroupBy;

Expand Down
5 changes: 3 additions & 2 deletions src/app/shared/issue-tables/IssuesDataTable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { BehaviorSubject, merge, Observable, Subscription } from 'rxjs';
import { map } from 'rxjs/operators';
import { Group } from '../../core/models/github/group.interface';
import { Issue } from '../../core/models/issue.model';
import { DEFAULT_FILTER, Filter } from '../../core/services/filters.service';
import { Filter, FiltersService } from '../../core/services/filters.service';
import { GroupingContextService } from '../../core/services/grouping/grouping-context.service';
import { IssueService } from '../../core/services/issue.service';
import { applyDropdownFilter } from './dropdownfilter';
Expand All @@ -15,7 +15,7 @@ import { applySearchFilter } from './search-filter';

export class IssuesDataTable extends DataSource<Issue> implements FilterableSource {
public count = 0;
private filterChange = new BehaviorSubject(DEFAULT_FILTER);
private filterChange = new BehaviorSubject(this.filtersService.defaultFilter());
private issuesSubject = new BehaviorSubject<Issue[]>([]);
private issueSubscription: Subscription;

Expand All @@ -24,6 +24,7 @@ export class IssuesDataTable extends DataSource<Issue> implements FilterableSour
constructor(
private issueService: IssueService,
private groupingContextService: GroupingContextService,
private filtersService: FiltersService,
private paginator: MatPaginator,
private displayedColumn: string[],
private group?: Group,
Expand Down
24 changes: 22 additions & 2 deletions src/app/shared/layout/header.component.html
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@
>WATcher v{{ this.getVersion() }}</a
>
<span id="view-descriptor" *ngIf="auth.isAuthenticated()" style="margin-left: 10px">
({{ this.getViewDescription(viewService.currentView) }})
({{ this.presetViews[this.filtersService.presetView$.value] }})
</span>

<div *ngIf="auth.isAuthenticated() && this.viewService.sessionData.sessionRepo.length > 1">
<!-- Gateway to activity dashboard, do not delete -->
<!--div *ngIf="auth.isAuthenticated() && this.viewService.sessionData.sessionRepo.length > 1">
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this redundant code be left in the code base?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

<button mat-button [matMenuTriggerFor]="menu"><mat-icon style="color: white">expand_more</mat-icon></button>
<mat-menu #menu="matMenu">
<button
Expand All @@ -31,6 +32,25 @@
</span>
</button>
</mat-menu>
</div-->

<div *ngIf="auth.isAuthenticated()">
<button mat-button [matMenuTriggerFor]="menu"><mat-icon style="color: white">expand_more</mat-icon></button>
<mat-menu #menu="matMenu">
<button
mat-menu-item
*ngFor="let presetView of this.presetViews | keyvalue"
(click)="this.filtersService.updatePresetView(presetView.key)"
>
<span>
<mat-icon
[ngStyle]="{ color: 'green', visibility: this.filtersService.presetView$.value === presetView.key ? 'visible' : 'hidden' }"
>done</mat-icon
>
{{ presetView.value }}
</span>
</button>
</mat-menu>
</div>

<!-- everything else -->
Expand Down
8 changes: 8 additions & 0 deletions src/app/shared/layout/header.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,14 @@ export class HeaderComponent implements OnInit {
private readonly yesButtonDialogMessage = 'Yes, I wish to log out';
private readonly noButtonDialogMessage = "No, I don't wish to log out";

readonly presetViews: {
[key: string]: string;
} = {
currentlyActive: 'Currently active',
contributions: 'Contributions',
custom: 'Custom'
};

/** Model for the displayed repository name */
currentRepo = '';

Expand Down
Loading