Refactor: unify delete and restore issue actions and move conditional logic into typescript file by TobyCyan · Pull Request #1360 · CATcher-org/CATcher

TobyCyan · 2025-06-30T08:08:33Z

Summary:

Changes Made:

Moved long boolean statements from the html to the typescript file for better readability.
Combined the delete and restore issue actions into a single method, alongside with one handler helper method that is toggled by passing in a boolean argument.
Combined the 2 separate lists issuesPendingDeletion and issuesPendingRestore into a single issuesPendingAction.

Proposed Commit Message:

Refactor issue actions and conditions for clarity and maintainability

The deleteIssue and undeleteIssue methods share highly similar logic
that performs the same actions in opposite directions.

This causes code duplication and makes future maintenance more 
error-prone.

Let's,
* extract the common logic into a reusable function to eliminate
duplication and improve clarity.
* merge the two separate state-tracking lists — 
issuesPendingDeletion and issuesPendingRestore into a 
single list issuesPendingAction.
* move complex conditional logic in issue-tables.component.html 
into issue-tables.component.ts.

This makes the template easier to read, improves maintainability,
and reduce duplicated code for the same actions in reverse.

damithc · 2025-07-01T11:57:03Z

Tagging @NorbertLoh @HollaG @wx-03 @IzN432 in case any of you can help with the PR ...

Copilot

Pull Request Overview

This PR refactors the issue actions by moving conditional logic out of the template and unifying delete/restore flows into a single handler, while consolidating pending-action tracking into one structure.

Moved complex boolean expressions from the HTML template into dedicated TypeScript methods.
Combined delete and restore logic into deleteOrRestoreIssue with a boolean toggle.
Replaced two pending-action maps with a single issuesPendingAction map.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
src/app/shared/issue-tables/issue-tables.component.ts	Unified delete/restore methods, moved template logic, consolidated pending flags
src/app/shared/issue-tables/issue-tables.component.html	Updated action buttons to use new helper methods

Comments suppressed due to low confidence (1)

src/app/shared/issue-tables/issue-tables.component.ts:160

[nitpick] Add JSDoc comments for this method (and other new helper methods) to describe parameters, side effects, and intended behavior, improving maintainability.

  deleteOrRestoreIssue(isDeleteAction: boolean, id: number, event: Event, actionUndoable: boolean = true) {

Copilot · 2025-07-07T04:43:04Z

src/app/shared/issue-tables/issue-tables.component.html

-        *ngIf="permissions.isIssueDeletable() && !issuesPendingDeletion[issue.id] && this.isActionVisible(action_buttons.DELETE_ISSUE)"
-        (click)="deleteIssue(issue.id, $event); $event.stopPropagation()"
+        *ngIf="isActionPerformAllowed(true, issue.id)"
+        (click)="deleteOrRestoreIssue(true, issue.id, $event); $event.stopPropagation()"


There’s a duplicate stopPropagation() call here and in the component method. Remove the inline $event.stopPropagation() in the template since deleteOrRestoreIssue already handles it.

Suggested change

(click)="deleteOrRestoreIssue(true, issue.id, $event); $event.stopPropagation()"

(click)="deleteOrRestoreIssue(true, issue.id, $event)"

There’s a duplicate stopPropagation() call here and in the component method. Remove the inline $event.stopPropagation() in the template since deleteOrRestoreIssue already handles it.

Agree with this

Copilot · 2025-07-07T04:43:04Z

src/app/shared/issue-tables/issue-tables.component.ts

+        (actionedIssue) => this.handleIssueActionPerformedSuccess(isDeleteAction, id, event, actionUndoable),
        (error) => this.errorHandlingService.handleError(error)
      );
    event.stopPropagation();


[nitpick] Consider handling event propagation in one place only—either in the template or inside this method—to avoid redundant calls and improve clarity.

Suggested change

event.stopPropagation();

// Removed event.stopPropagation() to handle event propagation in the template.

[nitpick] Consider handling event propagation in one place only—either in the template or inside this method—to avoid redundant calls and improve clarity.

Maybe the event.stopPropagation() can be removed from the template and kept here instead.

Copilot · 2025-07-07T04:43:04Z

src/app/shared/issue-tables/issue-tables.component.ts

      )
      .subscribe(
-        (removedIssue) => this.handleIssueDeletionSuccess(id, event, actionUndoable),
+        (actionedIssue) => this.handleIssueActionPerformedSuccess(isDeleteAction, id, event, actionUndoable),


The actionedIssue callback parameter is never used. You can simplify this to () => ... to remove the unused parameter and make the code clearer.

Suggested change

(actionedIssue) => this.handleIssueActionPerformedSuccess(isDeleteAction, id, event, actionUndoable),

() => this.handleIssueActionPerformedSuccess(isDeleteAction, id, event, actionUndoable),

The actionedIssue callback parameter is never used. You can simplify this to () => ... to remove the unused parameter and make the code clearer.

Agree with this too

Copilot · 2025-07-07T04:43:05Z

src/app/shared/issue-tables/issue-tables.component.ts

+  deleteOrRestoreIssue(isDeleteAction: boolean, id: number, event: Event, actionUndoable: boolean = true) {
+    const deletingKeyword = 'Deleting';
+    const undeletingKeyword = 'Undeleting';
+    this.logger.info(`IssueTablesComponent: ${isDeleteAction ? deletingKeyword : undeletingKeyword} Issue ${id}`);

-  deleteIssue(id: number, event: Event, actionUndoable: boolean = true) {
-    this.logger.info(`IssueTablesComponent: Deleting Issue ${id}`);
+    this.issuesPendingAction = { ...this.issuesPendingAction, [id]: true };

-    this.issuesPendingDeletion = { ...this.issuesPendingDeletion, [id]: true };
-    this.issueService
-      .deleteIssue(id)
+    let observableActionedIssue: Observable<Issue>;
+    if (isDeleteAction) {


[nitpick] Using a boolean flag to toggle delete vs. restore can be error-prone and less readable. Consider using an enum or separate named methods to make the intent explicit.

[nitpick] Using a boolean flag to toggle delete vs. restore can be error-prone and less readable. Consider using an enum or separate named methods to make the intent explicit.

While an enum could make the intent more explicit, I think a boolean works well here given that the function is only used in the HTML for delete/restore actions. Using a boolean also makes toggling for undo more straightforward.

wx-03

Good job on refactoring all the boolean statements and combining the deletion and restoration of issues! Overall lgtm, left some comments on which of copilot's suggestions are worth keeping

wx-03 · 2025-07-07T10:36:35Z

src/app/shared/issue-tables/issue-tables.component.ts

+  deleteOrRestoreIssue(isDeleteAction: boolean, id: number, event: Event, actionUndoable: boolean = true) {
+    const deletingKeyword = 'Deleting';
+    const undeletingKeyword = 'Undeleting';
+    this.logger.info(`IssueTablesComponent: ${isDeleteAction ? deletingKeyword : undeletingKeyword} Issue ${id}`);

-  deleteIssue(id: number, event: Event, actionUndoable: boolean = true) {
-    this.logger.info(`IssueTablesComponent: Deleting Issue ${id}`);
+    this.issuesPendingAction = { ...this.issuesPendingAction, [id]: true };

-    this.issuesPendingDeletion = { ...this.issuesPendingDeletion, [id]: true };
-    this.issueService
-      .deleteIssue(id)
+    let observableActionedIssue: Observable<Issue>;
+    if (isDeleteAction) {


[nitpick] Using a boolean flag to toggle delete vs. restore can be error-prone and less readable. Consider using an enum or separate named methods to make the intent explicit.

While an enum could make the intent more explicit, I think a boolean works well here given that the function is only used in the HTML for delete/restore actions. Using a boolean also makes toggling for undo more straightforward.

wx-03 · 2025-07-07T10:38:14Z

src/app/shared/issue-tables/issue-tables.component.html

-        *ngIf="permissions.isIssueDeletable() && !issuesPendingDeletion[issue.id] && this.isActionVisible(action_buttons.DELETE_ISSUE)"
-        (click)="deleteIssue(issue.id, $event); $event.stopPropagation()"
+        *ngIf="isActionPerformAllowed(true, issue.id)"
+        (click)="deleteOrRestoreIssue(true, issue.id, $event); $event.stopPropagation()"


There’s a duplicate stopPropagation() call here and in the component method. Remove the inline $event.stopPropagation() in the template since deleteOrRestoreIssue already handles it.

Agree with this

wx-03 · 2025-07-07T12:28:46Z

src/app/shared/issue-tables/issue-tables.component.ts

      )
      .subscribe(
-        (removedIssue) => this.handleIssueDeletionSuccess(id, event, actionUndoable),
+        (actionedIssue) => this.handleIssueActionPerformedSuccess(isDeleteAction, id, event, actionUndoable),


The actionedIssue callback parameter is never used. You can simplify this to () => ... to remove the unused parameter and make the code clearer.

Agree with this too

wx-03 · 2025-07-07T12:37:25Z

src/app/shared/issue-tables/issue-tables.component.ts

+        (actionedIssue) => this.handleIssueActionPerformedSuccess(isDeleteAction, id, event, actionUndoable),
        (error) => this.errorHandlingService.handleError(error)
      );
    event.stopPropagation();


[nitpick] Consider handling event propagation in one place only—either in the template or inside this method—to avoid redundant calls and improve clarity.

Maybe the event.stopPropagation() can be removed from the template and kept here instead.

wx-03

Sorry I missed this in the previous review, but could you update the commit message body to follow the Git conventions mentioned here? Everything else looks good. I'll merge once the commit message has been edited!

TobyCyan · 2025-07-09T14:24:57Z

Sorry I missed this in the previous review, but could you update the commit message body to follow the Git conventions mentioned here? Everything else looks good. I'll merge once the commit message has been edited!

hi, apologies for the poorly-craft commit message body. I've updated it to follow the convention as much as I could, could you please review it? Thanks!

wx-03 · 2025-07-09T18:05:05Z

Sorry I missed this in the previous review, but could you update the commit message body to follow the Git conventions mentioned here? Everything else looks good. I'll merge once the commit message has been edited!

hi, apologies for the poorly-craft commit message body. I've updated it to follow the convention as much as I could, could you please review it? Thanks!

LGTM!

TobyCyan added 5 commits June 30, 2025 11:35

Create a new list and methods that combine delete/restore functions

771ebef

Apply combined methods to html and delete previous methods

15f1b00

Remove unused lists and combine boolean statements into a method

a19f99f

Move more long condition statements into the typescript file

770b72a

Move more long conditions into the typescript file

a0bdd30

damithc requested review from Copilot and wx-03 July 7, 2025 04:41

Copilot AI reviewed Jul 7, 2025

View reviewed changes

wx-03 requested changes Jul 7, 2025

View reviewed changes

Remove unnecessary method call in template

ad8877d

TobyCyan requested a review from wx-03 July 8, 2025 02:12

wx-03 approved these changes Jul 8, 2025

View reviewed changes

wx-03 merged commit 7ced8ae into CATcher-org:master Jul 9, 2025
5 checks passed

	(click)="deleteOrRestoreIssue(true, issue.id, $event); $event.stopPropagation()"
	(click)="deleteOrRestoreIssue(true, issue.id, $event)"

	event.stopPropagation();
	// Removed event.stopPropagation() to handle event propagation in the template.

	(actionedIssue) => this.handleIssueActionPerformedSuccess(isDeleteAction, id, event, actionUndoable),
	() => this.handleIssueActionPerformedSuccess(isDeleteAction, id, event, actionUndoable),

Conversation

TobyCyan commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Changes Made:

Proposed Commit Message:

Uh oh!

damithc commented Jul 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

wx-03 Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

wx-03 Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

wx-03 Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

wx-03 Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wx-03 left a comment

Choose a reason for hiding this comment

Uh oh!

wx-03 Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wx-03 Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wx-03 Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wx-03 Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wx-03 left a comment

Choose a reason for hiding this comment

Uh oh!

TobyCyan commented Jul 9, 2025

Uh oh!

wx-03 commented Jul 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TobyCyan commented Jun 30, 2025 •

edited

Loading

wx-03 Jul 7, 2025 •

edited

Loading

wx-03 Jul 7, 2025 •

edited

Loading

wx-03 Jul 7, 2025 •

edited

Loading

wx-03 Jul 7, 2025 •

edited

Loading

wx-03 Jul 7, 2025 •

edited

Loading

wx-03 Jul 7, 2025 •

edited

Loading

wx-03 Jul 7, 2025 •

edited

Loading

wx-03 Jul 7, 2025 •

edited

Loading