Update labels without refresh #165

chia-yh · 2023-07-07T07:42:10Z

Summary:

Attempt to resolve #13

Labels are only fetched on initialization, which requires users to refresh the page in order for labels to update. Having labels update without requiring users to refresh would be ideal. The changes in this PR make it so that labels are polled and updated periodically, and also causes the Sync button to also update labels.

Currently polls label data at intervals of 5 seconds (the same POLL_INTERVAL as issues), however, as updates to labels are unlikely to occur as frequently as updates to issues, increasing the interval is probably a good idea to avoid excessive requests, as users can still use the Sync button if necessary.

Type of change:

✨ New Feature/ Enhancement

Changes Made:

-label.service.ts: added periodic polling and updating of label data
-label-filter-bar.component.ts: use updated label.service.ts to support updating of label data, moved simplifiedLabel into label.service.ts
-header-component.ts: changed reload() to also fetch label data
-Handle setup/cleanup of LabelService where appropriate

Screenshots:

Periodic polling:

Sync button (polling interval adjusted to 30 seconds for demonstration purposes):

Proposed Commit Message:

Update labels without needing to refresh

Labels are only fetched on initialization, requiring users to refresh
in order for labels to update.

Let's change it so labels are updated periodically, and when the Sync
button is clicked.

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.

gycgabriel

LGTM

gycgabriel · 2023-07-11T10:31:48Z

@chia-yh Thanks for your PR.

@CATcher-org/2223s2 @damithc Any thoughts on what is a good poll interval for fetching labels, or should we restrict it to a manual Sync button to minimize the number of github API requests?

gycgabriel · 2023-07-11T10:34:14Z

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

+export type simplifiedLabel = {
+  name: string;
+  color: string;
+};
+


For another issue/PR: This would be good to resolve why we need Label vs SimplifiedLabel, it seems redundant to have both. #82

Eclipse-Dominator

Some initial review

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

Eclipse-Dominator · 2023-07-13T16:11:56Z

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

+        this.simplifiedLabels = this.labels.map((label) => {
+          return {
+            name: label.getFormattedName(),
+            color: label.color
+          };
+        });
+        this.labelsSubject.next(this.simplifiedLabels);


I think at this point it is probably more efficient to just store getFormattedName()'s result as a property of Label instead of cloning the entire label.

We can still keep simplifiedLabel but make Label implement the type simplified label so this conversion will no longer be needed.

Not sure if I implemented the change requested here in the manner that was intended, but I made an attempt to do so, please let me know if I should make further changes

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

Eclipse-Dominator

See above (sorry accidentally commented instead of request change)

Eclipse-Dominator · 2023-07-14T13:06:09Z

src/app/core/models/label.model.ts

@@ -4,26 +4,27 @@
 export class Label {


I meant by

Suggested change

export class Label {

export class Label implements SimplifiedLabel {

This way Label is also a simplified label. So in service there is no need to convert the label array into simplified label array as label array is already a simplified label array.

Although we do need to rename the references to SimplifiedLabel from name to formattedName.

Let's rename SimplifiedLabel -> SimpleLabel

So it makes sense for Label to implement SimpleLabel as a more complex label

I see, that's a much better way of implementing SimplifiedLabel. I've made the changes as suggested in the most recent commits, and also renamed SimplifiedLabel -> SimpleLabel as mentioned.

I've also made some changes to accommodate #183 , as this PR modifies parts of the original label-filter-bar.component.ts used in that PR.

chia-yh · 2023-07-25T05:52:55Z

@chia-yh Thanks for your PR.

@CATcher-org/2223s2 @damithc Any thoughts on what is a good poll interval for fetching labels, or should we restrict it to a manual Sync button to minimize the number of github API requests?

I was looking through some of the other issues, and came across #117 , which brings up a good point, that WATcher users can afford to view non-updated data. As this PR already has the sync button also update the labels, which aligns with the suggestion in #117 (comment), should I change it so that the periodic polling of labels is removed, and the fetching of labels is restricted to just the sync button as mentioned in an earlier comment?

While I think having periodic polling would be good to have, in my opinion, setting it to a long enough interval to prevent too many calls to fetch data would cause it to provide little benefit to the user, as they are unlikely to run into a situation where the refresh would be timely enough to update the data as they need it, and they could just use the sync button to refresh data.

Also, since the issue concerning periodic label polling is similar to the issue brought up in #117 , should the same changes decided on here be applied to the issue polling? I think the sync button also fetches issues at the moment, so it would just be a matter of removing the polling at 5 second intervals, though the change should definitely be done in a separate PR addressing #117 directly.

Eclipse-Dominator · 2023-07-29T14:14:19Z

Perhaps we can make the polling an optional toggle somewhere on the page.
This way users (who somehow want to keep the page open for a long time) can still have access to updated data. Handling it should be done in a seperate PR.

Eclipse-Dominator

LGTM!

gycgabriel

LGTM

Update labels without needing to refresh Labels are only fetched on initialization, requiring users to refresh in order for labels to update. Let's change it so labels are updated periodically, and when the Sync button is clicked.

chia-yh added 8 commits July 6, 2023 16:04

Add polling, reset methods to label.service.ts

6cbf1f2

Update labels periodically and on sync button click

d5acfce

Adjust polling interval

ec2f24a

Cleanup LabelService on changing repo, logging out

3df12dd

Remove duplicate load

19aa1f5

Fix labels breaking on change repo

52f5db8

Fix lint errors

45c8ca4

Merge branch 'main' into update-labels-without-refresh

f97520f

gycgabriel added the s.toReview label Jul 11, 2023

gycgabriel previously approved these changes Jul 11, 2023

View reviewed changes

gycgabriel reviewed Jul 11, 2023

View reviewed changes

Eclipse-Dominator reviewed Jul 13, 2023

View reviewed changes

Eclipse-Dominator requested changes Jul 13, 2023

View reviewed changes

chia-yh added 4 commits July 14, 2023 11:38

Move SimplifiedLabel into label.model

bf72ff0

Convert check to a guard clause

d44adce

Store formattedName as property of Label

4f38d69

Merge branch 'main' into update-labels-without-refresh

abbe68c

Eclipse-Dominator reviewed Jul 14, 2023

View reviewed changes

chia-yh dismissed gycgabriel’s stale review via abbe68c July 18, 2023 01:54

chia-yh added 3 commits July 24, 2023 09:48

Resolve conflicts, fix no labels, add SimpleLabel

3c3966b

Format html

e4a22a2

Rename references from name to formattedName

3c73261

Eclipse-Dominator approved these changes Jul 29, 2023

View reviewed changes

gycgabriel approved these changes Aug 15, 2023

View reviewed changes

gycgabriel merged commit d2d5370 into CATcher-org:main Aug 15, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update labels without refresh #165

Update labels without refresh #165

chia-yh commented Jul 7, 2023 •

edited

Loading

gycgabriel left a comment

gycgabriel commented Jul 11, 2023

gycgabriel Jul 11, 2023 •

edited

Loading

Eclipse-Dominator left a comment

Eclipse-Dominator Jul 13, 2023

chia-yh Jul 14, 2023

Eclipse-Dominator left a comment •

edited

Loading

Eclipse-Dominator Jul 14, 2023

gycgabriel Jul 17, 2023

chia-yh Jul 24, 2023 •

edited

Loading

chia-yh commented Jul 25, 2023

Eclipse-Dominator commented Jul 29, 2023

Eclipse-Dominator left a comment

gycgabriel left a comment

	export class Label {
	export class Label implements SimplifiedLabel {

Update labels without refresh #165

Update labels without refresh #165

Conversation

chia-yh commented Jul 7, 2023 • edited Loading

Summary:

Type of change:

Changes Made:

Screenshots:

Proposed Commit Message:

Checklist:

gycgabriel left a comment

Choose a reason for hiding this comment

gycgabriel commented Jul 11, 2023

gycgabriel Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Eclipse-Dominator left a comment

Choose a reason for hiding this comment

Eclipse-Dominator Jul 13, 2023

Choose a reason for hiding this comment

chia-yh Jul 14, 2023

Choose a reason for hiding this comment

Eclipse-Dominator left a comment • edited Loading

Choose a reason for hiding this comment

Eclipse-Dominator Jul 14, 2023

Choose a reason for hiding this comment

gycgabriel Jul 17, 2023

Choose a reason for hiding this comment

chia-yh Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

chia-yh commented Jul 25, 2023

Eclipse-Dominator commented Jul 29, 2023

Eclipse-Dominator left a comment

Choose a reason for hiding this comment

gycgabriel left a comment

Choose a reason for hiding this comment

chia-yh commented Jul 7, 2023 •

edited

Loading

gycgabriel Jul 11, 2023 •

edited

Loading

Eclipse-Dominator left a comment •

edited

Loading

chia-yh Jul 24, 2023 •

edited

Loading