SF-3768 Add assignee and resolution to draft request detail page by Nateowami · Pull Request #3780 · sillsdev/web-xforge

Nateowami · 2026-04-06T23:39:37Z

This change takes this capability:

...and copies it here, while trying to avoid duplicating logic too much.

This change is

codecov · 2026-04-06T23:47:17Z

Codecov Report

❌ Patch coverage is 28.78788% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.22%. Comparing base (9010370) to head (2a777ef).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ture/Controllers/OnboardingRequestRpcController.cs	0.00%	29 Missing ⚠️
...boarding-requests/onboarding-requests.component.ts	0.00%	13 Missing ⚠️
...uest-detail/onboarding-request-detail.component.ts	81.81%	2 Missing ⚠️
...ate/draft-generation/onboarding-request.service.ts	0.00%	2 Missing ⚠️
...ct/onboarding-request-assignee-select.component.ts	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3780      +/-   ##
==========================================
- Coverage   81.43%   81.22%   -0.22%     
==========================================
  Files         623      625       +2     
  Lines       39451    39656     +205     
  Branches     6398     6465      +67     
==========================================
+ Hits        32128    32210      +82     
- Misses       6335     6437     +102     
- Partials      988     1009      +21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

devin-ai-integration

Devin Review found 3 new potential issues.

View 6 additional findings in Devin Review.

devin-ai-integration · 2026-04-08T23:51:04Z

+  async onAssigneeChange(newAssigneeId: string): Promise<void> {
+    if (this.request == null) return;
+    this.request = await this.onboardingRequestService.setAssignee(this.request.id, newAssigneeId);
+  }
+
+  async onResolutionChange(newResolution: DraftRequestResolutionKey | null): Promise<void> {
+    if (this.request == null) return;
+    this.request = await this.onboardingRequestService.setResolution(this.request.id, newResolution);
+  }


🔴 Missing error handling in detail component's onAssigneeChange and onResolutionChange

The onAssigneeChange and onResolutionChange methods in the detail component have no try/catch error handling, unlike their counterparts in the list component (onboarding-requests.component.ts:189-207 and onboarding-requests.component.ts:214-235). If the API call (setAssignee or setResolution) fails, the dropdown will already display the new value (because mat-select updates its view immediately on selection), but this.request won't be updated with the server response. This results in an unhandled promise rejection, no user-visible error feedback, and the UI becoming desynchronized from the server state. This also violates AGENTS.md: "It is better to explicitly check for and handle problems, or prevent problems from happening, than to assume problems will not happen."

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-04-08T23:51:06Z

+function createTestRequest(overrides: Partial<OnboardingRequest> = {}): OnboardingRequest {
+  return {
+    id: REQUEST_ID,
+    submittedAt: '2024-01-01T00:00:00Z',
+    submittedBy: { name: 'Test User', email: 'test@example.com' },
+    submission: {
+      projectId: 'project01',
+      userId: 'user03',
+      timestamp: '2024-01-01T00:00:00Z',
+      formData: {
+        name: 'Test User',
+        email: 'test@example.com',
+        organization: 'Test Org',
+        partnerOrganization: 'Partner Org',
+        translationLanguageName: 'English',
+        translationLanguageIsoCode: 'en',
+        completedBooks: [40, 41, 42, 43],
+        nextBooksToDraft: [44],
+        sourceProjectA: 'ptproject01',
+        draftingSourceProject: 'ptproject02',
+        backTranslationStage: 'None',
+        backTranslationProject: null
+      }
+    },
+    assigneeId: '',
+    status: 'new',
+    resolution: 'unresolved',
+    comments: [],
+    ...overrides
+  };
+}


🔴 Helper function createTestRequest is defined outside the TestEnvironment class

The createTestRequest function is defined at module level outside both the describe block and the TestEnvironment class. AGENTS.md mandates: "Do not put helper functions outside of TestEnvironment classes; helper functions or setup functions should be in the TestEnvironment class." This function should be a static method on the TestEnvironment class.

Prompt for agents

Move the createTestRequest function into the TestEnvironment class (defined at draft-request-detail.component.spec.ts:149) as a static method. It is currently at module level (line 34-64) but AGENTS.md requires helper functions to be inside the TestEnvironment class. Since it is used both in the TestEnvironment constructor and in individual test cases, make it static createTestRequest(...) and call it as TestEnvironment.createTestRequest(...).

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-04-08T23:51:07Z

      const requests = await this.onboardingRequestService.getAllRequests();
      if (requests != null) {
        this.requests = requests;
-        this.initializeRequestData();
+        this.filterRequests();
+        void this.loadProjectNames();
      }
-      this.loadingFinished();
-    } catch (error) {
-      console.error('Error loading draft requests:', error);
-      this.noticeService.showError('Failed to load draft requests');
+    } finally {
      this.loadingFinished();
    }
  }


🚩 loadRequests error handling changed from catch-and-notify to finally-only

In onboarding-requests.component.ts:118-130, the old code had a catch block that logged the error and called noticeService.showError('Failed to load draft requests'). The new code uses only try/finally, so errors will propagate as unhandled rejections without a user-facing message. Since this is called via void this.loadRequests() from ngOnInit(), the rejection won't be caught. This is a pre-existing pattern shift—the detail component's loadRequest also uses try/finally without catch. Both are consistent with each other now, but the user loses the error notification.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration

Devin Review found 2 new potential issues.

View 5 additional findings in Devin Review.

devin-ai-integration · 2026-04-13T16:50:20Z

@@ -101,14 +109,30 @@ export class DraftRequestDetailComponent extends DataLoadingComponent implements
  private async loadRequest(requestId: string): Promise<void> {
    this.loadingStarted();
    try {
+      void this.loadExistingAssignees();


🚩 existingAssigneeIds not refreshed after assignee/resolution changes

In both onboarding-requests.component.ts and draft-request-detail.component.ts, existingAssigneeIds is loaded once via a fire-and-forget call to GetCurrentlyAssignedUserIds() during initial load (loadRequests/loadRequest). When an assignee is changed or removed, existingAssigneeIds is NOT refreshed. The old code in the list component called initializeRequestData() which recalculated assigned user IDs from the local this.requests array after every change. Now, the dropdown options can become stale — e.g., if a user is unassigned from all requests, they'll still appear in other rows' dropdowns. This is a minor UX regression; the dropdown won't be incorrect per se (the previously-assigned user is still a valid selection), but it won't reflect the latest assignment state until page reload.

Was this helpful? React with 👍 or 👎 to provide feedback.

marksvc

Good. Thank you for the screenshots.

@marksvc reviewed 13 files and all commit messages, and made 11 comments.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on Nateowami).

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-request-detail.component.ts line 124 at r7 (raw file):

  }

  getStatus = this.onboardingRequestService.getStatus;

(Hmm, I see that this line was already present but in another place. Notwithstanding,)
What's the reason for this way to get to the getStatus method?

I understand that using this isn't a problem now, because onboarding-request.service.ts getStatus does not reference this there. But since this could become a problem if onboarding-request.service.ts getStatus changes to use this, which isn't unreasonable, and because it's not very complicated to implement another way to do this, what do you think about replacing this line with one of the following solutions?

We could use getStatus directly from onboardingRequestService:

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-request-detail.component.html
           <span class="label">Status:</span>
-          <span class="value">{{ getStatus(request.status).label }}</span>
+          <span class="value">{{ onboardingRequestService.getStatus(request.status).label }}</span>

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-request-detail.component.ts
-  getStatus = this.onboardingRequestService.getStatus;

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.html
           <th mat-header-cell *matHeaderCellDef>Status</th>
-          <td mat-cell *matCellDef="let request">{{ getStatus(request.status).label }}</td>
+          <td mat-cell *matCellDef="let request">{{ onboardingRequestService.getStatus(request.status).label }}</td>

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts
-  getStatus = this.onboardingRequestService.getStatus;

We could wrap:

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-request-detail.component.ts
-  getStatus = this.onboardingRequestService.getStatus;
+  getStatus(status: DraftRequestStatusOption): DraftRequestStatusMetadata {
+    return this.onboardingRequestService.getStatus(status);
+  }

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts
-  getStatus = this.onboardingRequestService.getStatus;
+  getStatus(status: DraftRequestStatusOption): DraftRequestStatusMetadata {
+    return this.onboardingRequestService.getStatus(status);
+  }

Please excuse if any of these don't actually work; my local SF is a bit more cumbersome to use until .net 10 is merged.

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-request-detail.component.ts line 131 at r7 (raw file):

  }

  async onResolutionChange(newResolution: DraftRequestResolutionKey | null): Promise<void> {

Devin says that since these are not wrapped in a try/catch with error feedback to the user, like onboarding-requests.component.ts onAssigneeChange is, that their failure will happen without user-feedback or handling. Now, we may have universal error handling, but in so far as the feedback and recovery in onboarding-requests.component.ts onAssigneeChange / onResolutionChange is helpful, it may likewise be helpful here.

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-request-detail.component.ts line 132 at r7 (raw file):

  async onResolutionChange(newResolution: DraftRequestResolutionKey | null): Promise<void> {
    if (this.request == null) return;

We might consider updating REVIEW.md to explain to Devin about the global error handling system, and what our expectations are regarding when, where, and how exceptions should be processed, to help Devin flag the right things.

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 145 at r7 (raw file):

    // Collect all assigned user IDs for the dropdown options (excluding empty string)
    this.assignedUserIds = new Set(
      this.requests.map(r => r.assigneeId).filter((id): id is string => id != null && id !== '')

I know this code went away, but for next time, note that type-utils.ts isPopulatedString() could potentially be useful.

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 161 at r7 (raw file):

  }

  readonly getResolution = this.onboardingRequestService.getResolution;

This field/method appears to be unused.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/onboarding-request.service.ts line 65 at r7 (raw file):

/** Status options for draft requests. Some are user-selectable, others are system-managed. */
export const DRAFT_REQUEST_STATUS_OPTIONS = [
  { value: 'new', label: 'New' },

(Just saying.) Hmm. Non-localized strings that are outside of the serval-administration directory.

src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 595 at r7 (raw file):

    }

    public async Task<IRpcMethodResult> GetCurrentlyAssignedUserIds()

(Just saying.) I think we're going to regret letting this RpcController class be written contrary to the structure of our other RpcController files.

src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 595 at r7 (raw file):

    }

    public async Task<IRpcMethodResult> GetCurrentlyAssignedUserIds()

I think this endpoint name could be improved. It's naempsaced in the onboarding-requests area, so that's good, but I think an important and unstated aspect to what the endpoint is giving is that the IDs are Serval administrator staff user IDs and on the intake part of the process.

With the current endpoint name, this endpoint might instead provide information about the user IDs of translators who have their onboarding request assigned.

A clearer endpoint name might be something like

getHandlingStaffIds
getAssignedStaffIds
getAssignedServalAdminIds

This is not a blocking comment, but you might consider a different endpoint name.

src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 605 at r7 (raw file):

            }

            var adminIds = await InternalGetCurrentlyAssignedUserIds();

We have sought to avoid using var except when constructing or for difficult anonymous types. I apologize for all of the examples of it being used. You'll instead want to use

string[] adminIds = ...

src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 621 at r7 (raw file):

    }

    private async Task<string[]> InternalGetCurrentlyAssignedUserIds()

This looks like a good motivation to start to conform the structure of this Controller file to be like its peers.

In other words, putting this method into a new SIL.XForge.Scripture/Services/OnboardingRequestService.cs file, declaring the method as public async Task<string[]> GetCurrentlyAssignedUserIdsAsync(, creating a new file SIL.XForge/Services/IOnboardingRequestService.cs, and declaring in the interface a Task<string[]> GetCurrentlyAssignedUserIdsAsync(.

That's probably not welcome code review feedback :). But I think it's the right step.

(BTW take note that by convention the method name will end in Async since it returns Task<>. (But the endpoint method will not.))

Nateowami added the will require testing PR should not be merged until testers confirm testing is complete label Apr 6, 2026