SF-3772 Prevent duplicate onboarding requests

[Nateowami]

Before this change, the UI already informs the user when a request has already been submitted and doesn't allow submitting another. However, the check is only done when the drafting component loads, so by using multiple tabs, or possibly by using browser history, it's possible to submit the form twice. This change adds additional checks to prevent duplicate submissions.

EDIT: I think the actual reason we were getting duplicate requests was network errors when submitting the form:

The request did however get saved, but the client didn't know it, so the user was able to re-submit.

EDIT 2: I think the network errors are timeouts. I've created a PR to fix them: #3785. However, I think this PR is still a good change.

---

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 11.76471% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.60%. Comparing base (242bb96) to head (3f4ef6a).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...aft-signup-form/draft-onboarding-form.component.ts	0.00%	15 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3784      +/-   ##
==========================================
- Coverage   80.62%   80.60%   -0.02%     
==========================================
  Files         634      634              
  Lines       40988    40997       +9     
  Branches     6660     6664       +4     
==========================================
  Hits        33046    33046              
+ Misses       6904     6900       -4     
- Partials     1038     1051      +13     

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

[marksvc]

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

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 406 at r2 (raw file):
Hmm. If they hear back from the team, can they submit another request?

If not, perhaps this might say

A request to activate drafting on this project has already been submitted. Thank you for your patience in waiting for a response from the team.

Also, we might consider mentioning where they should be looking for a response. An email? A new ability/button present on the Draft Generation page?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 373 at r2 (raw file):

  private async hasRequestAlreadyBeenSubmitted(): Promise<boolean> {
    return (await this.onboardingRequestService.getOpenOnboardingRequest(this.activatedProject.projectId!)) != null;

I think we will do ourselves good to continue to let the type system help us. Rather than introduce the non null assertion operator here, what do you think about using

    if (this.activatedProject.projectId == null) return false;
    return (await this.onboardingRequestService.getOpenOnboardingRequest(this.activatedProject.projectId)) != null;

or

    const projectId: string | undefined = this.activatedProject.projectId;
    return (projectId == null || (await this.onboardingRequestService.getOpenOnboardingRequest(projectId))) != null;

?

[Nateowami]

@Nateowami made 2 comments.
Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 373 at r2 (raw file):

Previously, marksvc wrote…

I think we will do ourselves good to continue to let the type system help us. Rather than introduce the non null assertion operator here, what do you think about using

    if (this.activatedProject.projectId == null) return false;
    return (await this.onboardingRequestService.getOpenOnboardingRequest(this.activatedProject.projectId)) != null;

or

    const projectId: string | undefined = this.activatedProject.projectId;
    return (projectId == null || (await this.onboardingRequestService.getOpenOnboardingRequest(projectId))) != null;

?

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 406 at r2 (raw file):

Previously, marksvc wrote…

Hmm. If they hear back from the team, can they submit another request?

If not, perhaps this might say

A request to activate drafting on this project has already been submitted. Thank you for your patience in waiting for a response from the team.

Also, we might consider mentioning where they should be looking for a response. An email? A new ability/button present on the Draft Generation page?

Good point. Done. I didn't try to elaborate because it should only be possible to trigger this message by opening the drafting page in multiple tabs, submitting the form in one, and then trying to do so in the other.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 5 additional findings in Devin Review.

[devin-ai-integration]

📝 Info: Race condition double-click guard is effective but adds a redundant network call

The race condition guard at line 219 (if ((this.uiState as OnboardingFormUiState) === 'submitting') return;) correctly handles the double-click scenario. In JavaScript's single-threaded event loop: Click 1 awaits checkAndWarnIfAlreadySubmitted, then proceeds to set uiState = 'submitting'. Click 2, which also awaited checkAndWarnIfAlreadySubmitted concurrently, reaches line 219 and finds uiState is now 'submitting', so it exits. However, both clicks trigger a network call to getOpenOnboardingRequest, which is wasteful. A simpler approach would be to set a flag (or check uiState) before entering the async check, but the current approach is functionally correct.

---

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

[Nateowami]

Devin had a good point here, which I've addressed. However, note that the type system is overly optimistic and assumes the type of this.uiState can't be 'submitting' because we already added a guard for that. This is one of many instances where the type checker is overly permissive.

[marksvc]

Interesting.

[Nateowami]

@marksvc This is waiting for re-review.

[marksvc]

Approved, but not moving to testing myself because of a concern in a non-blocking comment (about removing void this.checkAndWarnIfAlreadySubmitted();)

@marksvc reviewed 6 files and all commit messages, made 4 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Nateowami).

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 152 at r1 (raw file):

    this.loadingStarted();

    void this.checkAndWarnIfAlreadySubmitted();

What was the reason to remove this line? I thought this was part of defending against particular scenarios.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 225 at r5 (raw file):

      this.cd.markForCheck();

      const formData = this.signupForm.getRawValue() as OnboardingRequestFormData;

Hmm. I understand that there are some reasonable ways to get the values and still use the type system. I suppose with the code at hand, we are getting something and passing it to the server and the server needs to validate it anyway.

[marksvc]

Interesting.

[devin-ai-integration]

📝 Info: Duplicate OnboardingRequestComment interface cleanup is correct

The old file had OnboardingRequestComment defined twice — once at line 6-11 (before DraftingSignupFormData) and once at line 55-60 (after OnboardingRequest). This PR removes the first duplicate definition, keeping only the one at line 47-53 which has the JSDoc comment. The remaining definition is the one actually referenced by OnboardingRequest.comments. This is a correct cleanup.

---

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

[marksvc]

^^^

[Nateowami]

Done.

[Nateowami]

@Nateowami made 3 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 152 at r1 (raw file):

Previously, marksvc wrote…

What was the reason to remove this line? I thought this was part of defending against particular scenarios.

Good catch. I think that was accidental, though I don't remember how it happened.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 225 at r5 (raw file):

Previously, marksvc wrote…

Hmm. I understand that there are some reasonable ways to get the values and still use the type system. I suppose with the code at hand, we are getting something and passing it to the server and the server needs to validate it anyway.

This is probably fixable. If I remove it there are some errors regarding which fields are nullable. The types could probably be made to match more closely, but there's a logical difference: the types that make sense for the form (regarding what fields are nullable) aren't necessarily the same as the types that make sense for the data saved to the DB (where required fields shouldn't ever be null).

If I make draftingSourceProject non-nullable, it doesn't have a way to represent "no project selected". However, we enforce a project to be selected when the form is actually submitted. In a sense, we're narrowing the type when we check if the form is valid.

[Nateowami]

Done.

[marksvc]

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 225 at r5 (raw file):

Previously, Nateowami wrote…

This is probably fixable. If I remove it there are some errors regarding which fields are nullable. The types could probably be made to match more closely, but there's a logical difference: the types that make sense for the form (regarding what fields are nullable) aren't necessarily the same as the types that make sense for the data saved to the DB (where required fields shouldn't ever be null).

If I make draftingSourceProject non-nullable, it doesn't have a way to represent "no project selected". However, we enforce a project to be selected when the form is actually submitted. In a sense, we're narrowing the type when we check if the form is valid.

I asked Devin about this the other day. The option that I think is best for our situation is

### 2. Build the payload explicitly

Instead of casting the whole form, construct a typed object field-by-field in onSubmit() (draft-onboarding-form.component.ts:225):

const v = this.signupForm.getRawValue();
const formData: OnboardingRequestFormData = {  
  name: v.name,  
  email: v.email,  
  ...  
  sourceProjectA: v.sourceProjectA ?? '',  
  sourceProjectB: v.sourceProjectB ?? undefined,  
  ...
};

No as needed, and TypeScript verifies the mapping. Verbose, but explicit.

Although as I look more closely, I see that the TS OnboardingRequestFormData is not the same shape as the C# OnboardingRequestFormData. So that could get some adjustment.

Here's a sketch of the idea. I didn't see a .spec.ts file to verify the changes against.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts
       this.uiState = 'submitting';
       this.cd.markForCheck();
 
-      const formData = this.signupForm.getRawValue() as OnboardingRequestFormData;
+      const value = this.signupForm.getRawValue();
+      const formData: OnboardingRequestFormDataDto = {
+        name: value.name,
+        email: value.email,
+        organization: value.organization,
+        partnerOrganization: value.partnerOrganization,
+        translationLanguageName: value.translationLanguageName,
+        translationLanguageIsoCode: value.translationLanguageIsoCode,
+        completedBooks: value.completedBooks,
+        nextBooksToDraft: value.nextBooksToDraft,
+        sourceProjectA: value.sourceProjectA ?? undefined,
+        sourceProjectB: value.sourceProjectB ?? undefined,
+        sourceProjectC: value.sourceProjectC ?? undefined,
+        draftingSourceProject: value.draftingSourceProject ?? undefined,
+        backTranslationStage: value.backTranslationStage,
+        backTranslationProject: value.backTranslationProject ?? undefined,
+        backTranslationLanguageName: value.backTranslationLanguageName,
+        backTranslationLanguageIsoCode: value.backTranslationLanguageIsoCode,
+        additionalComments: value.additionalComments
+      };
 
       try {
         const requestId = await this.onboardingRequestService.submitOnboardingRequest(projectId, formData);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/onboarding-request.service.ts
   additionalComments?: string;
 }
 
+/**
+ * Outgoing payload shape for submitting onboarding request form data.
+ * To be kept in sync with C# OnboardingRequestFormData.
+ */
+export interface OnboardingRequestFormDataDto {
+  name: string;
+  email: string;
+  organization: string;
+  partnerOrganization: string;
+
+  translationLanguageName: string;
+  translationLanguageIsoCode: string;
+
+  completedBooks: number[];
+  nextBooksToDraft: number[];
+
+  sourceProjectA?: string;
+  sourceProjectB?: string;
+  sourceProjectC?: string;
+  draftingSourceProject?: string;
+
+  backTranslationStage: string;
+  backTranslationProject?: string;
+  backTranslationLanguageName: string;
+  backTranslationLanguageIsoCode: string;
+
+  additionalComments: string;
+}
+
 export interface OnboardingRequest {
   id: string;
   submittedAt: string;
@@ -97,7 +126,7 @@ export class OnboardingRequestService {
   }
 
   /** Submits a new signup request. */
-  async submitOnboardingRequest(projectId: string, formData: OnboardingRequestFormData): Promise<string> {
+  async submitOnboardingRequest(projectId: string, formData: OnboardingRequestFormDataDto): Promise<string> {
     return (await this.onlineInvoke<string>('submitOnboardingRequest', { projectId, formData }))!;
   }

[Nateowami]

@Nateowami made 1 comment.
Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 225 at r5 (raw file):

Previously, marksvc wrote…

I asked Devin about this the other day. The option that I think is best for our situation is

### 2. Build the payload explicitly

Instead of casting the whole form, construct a typed object field-by-field in onSubmit() (draft-onboarding-form.component.ts:225):

const v = this.signupForm.getRawValue();
const formData: OnboardingRequestFormData = {  
  name: v.name,  
  email: v.email,  
  ...  
  sourceProjectA: v.sourceProjectA ?? '',  
  sourceProjectB: v.sourceProjectB ?? undefined,  
  ...
};

No as needed, and TypeScript verifies the mapping. Verbose, but explicit.

Although as I look more closely, I see that the TS OnboardingRequestFormData is not the same shape as the C# OnboardingRequestFormData. So that could get some adjustment.

Here's a sketch of the idea. I didn't see a .spec.ts file to verify the changes against.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts
       this.uiState = 'submitting';
       this.cd.markForCheck();
 
-      const formData = this.signupForm.getRawValue() as OnboardingRequestFormData;
+      const value = this.signupForm.getRawValue();
+      const formData: OnboardingRequestFormDataDto = {
+        name: value.name,
+        email: value.email,
+        organization: value.organization,
+        partnerOrganization: value.partnerOrganization,
+        translationLanguageName: value.translationLanguageName,
+        translationLanguageIsoCode: value.translationLanguageIsoCode,
+        completedBooks: value.completedBooks,
+        nextBooksToDraft: value.nextBooksToDraft,
+        sourceProjectA: value.sourceProjectA ?? undefined,
+        sourceProjectB: value.sourceProjectB ?? undefined,
+        sourceProjectC: value.sourceProjectC ?? undefined,
+        draftingSourceProject: value.draftingSourceProject ?? undefined,
+        backTranslationStage: value.backTranslationStage,
+        backTranslationProject: value.backTranslationProject ?? undefined,
+        backTranslationLanguageName: value.backTranslationLanguageName,
+        backTranslationLanguageIsoCode: value.backTranslationLanguageIsoCode,
+        additionalComments: value.additionalComments
+      };
 
       try {
         const requestId = await this.onboardingRequestService.submitOnboardingRequest(projectId, formData);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/onboarding-request.service.ts
   additionalComments?: string;
 }
 
+/**
+ * Outgoing payload shape for submitting onboarding request form data.
+ * To be kept in sync with C# OnboardingRequestFormData.
+ */
+export interface OnboardingRequestFormDataDto {
+  name: string;
+  email: string;
+  organization: string;
+  partnerOrganization: string;
+
+  translationLanguageName: string;
+  translationLanguageIsoCode: string;
+
+  completedBooks: number[];
+  nextBooksToDraft: number[];
+
+  sourceProjectA?: string;
+  sourceProjectB?: string;
+  sourceProjectC?: string;
+  draftingSourceProject?: string;
+
+  backTranslationStage: string;
+  backTranslationProject?: string;
+  backTranslationLanguageName: string;
+  backTranslationLanguageIsoCode: string;
+
+  additionalComments: string;
+}
+
 export interface OnboardingRequest {
   id: string;
   submittedAt: string;
@@ -97,7 +126,7 @@ export class OnboardingRequestService {
   }
 
   /** Submits a new signup request. */
-  async submitOnboardingRequest(projectId: string, formData: OnboardingRequestFormData): Promise<string> {
+  async submitOnboardingRequest(projectId: string, formData: OnboardingRequestFormDataDto): Promise<string> {
     return (await this.onlineInvoke<string>('submitOnboardingRequest', { projectId, formData }))!;
   }

From the suggestion:

sourceProjectA: v.sourceProjectA ?? ''

An empty string to represent null is worse than just null or undefined. This isn't solving a problem; it's hiding it so it doesn't show up until a project can't be found because the id for it is empty.

[marksvc]

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 225 at r5 (raw file):

Previously, Nateowami wrote…

From the suggestion:

sourceProjectA: v.sourceProjectA ?? ''

An empty string to represent null is worse than just null or undefined. This isn't solving a problem; it's hiding it so it doesn't show up until a project can't be found because the id for it is empty.

Yeah, I don't recommend we do that, but I hope the methodology was communicated.

[marksvc]

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 225 at r5 (raw file):

Previously, marksvc wrote…

Yeah, I don't recommend we do that, but I hope the methodology was communicated.

(I'm also don't think it's super sensible to have both an
OnboardingRequestFormData type and a
OnboardingRequestFormDataDto type in TS, but I didn't continue working with it to see what preferential situation could be done.)

[Nateowami]

@Nateowami resolved 2 discussions.
Reviewable status: 3 of 6 files reviewed, all discussions resolved (waiting on marksvc).

[marksvc]

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

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 152 at r1 (raw file):

Previously, Nateowami wrote…

Good catch. I think that was accidental, though I don't remember how it happened.

I had to try a few revisions to find it again :). Looks like the line that was lost was

void this.checkAndWarnIfAlreadySubmitted();

It sounds like you were last saying that this is important and accidental for it to have been removed.

---

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

import { SFProjectService } from '../../core/sf-project.service';

export interface OnboardingRequestComment {

Looks like this duplicate interface definition was re-introduced. I assume from rebase conflict resolution. Looks like we should just delete this interface definition again.

[marksvc]

Hope I'm not causing trouble. Because the missing void this.checkAndWarnIfAlreadySubmitted(); may be an accidental incorrect behaviour change, I set the issue and PR back to being in code review and not yet ready for testing, so we don't waste testing with void this.checkAndWarnIfAlreadySubmitted(); missing, if in fact it is important.

@marksvc made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Nateowami).

[Nateowami]

@Nateowami made 1 comment.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 152 at r1 (raw file):

Previously, marksvc wrote…

I had to try a few revisions to find it again :). Looks like the line that was lost was

void this.checkAndWarnIfAlreadySubmitted();

It sounds like you were last saying that this is important and accidental for it to have been removed.

Thank you for catching this. I don't know how I've botched this so many times. It's restored now.

[Nateowami]

@marksvc You're not causing trouble; you're saving me from a level of confusion/incompetence I can hardly grasp.

[marksvc]

@Nateowami We all get mixed up.

@marksvc reviewed 2 files and all commit messages, made 3 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Nateowami).

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 152 at r1 (raw file):

Previously, Nateowami wrote…

Thank you for catching this. I don't know how I've botched this so many times. It's restored now.

👍

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 225 at r5 (raw file):

Previously, marksvc wrote…

(I'm also don't think it's super sensible to have both an
OnboardingRequestFormData type and a
OnboardingRequestFormDataDto type in TS, but I didn't continue working with it to see what preferential situation could be done.)

This conversation might also be one that slipped thru the cracks, where we discussed using the type system with these values. In a comment above I have a diff example below a line saying "Here's a sketch of the idea". IIRC this essentially distinguishes between the form's types and the types of the data to transfer, and translates from one to the other.
The similarity between the two may be motivating us to bypass some of the type system, but I think we can still work with it.

I do think it would be better to adjust this and not to use as, but my position here is non-blocking. But I'm marking this comment blocking to help surface it in case your Satisfied Disposition is hiding the thread. If you want to keep the current implementation, we can but I want to check on that.

[Nateowami]

@Nateowami made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 225 at r5 (raw file):

Previously, marksvc wrote…

This conversation might also be one that slipped thru the cracks, where we discussed using the type system with these values. In a comment above I have a diff example below a line saying "Here's a sketch of the idea". IIRC this essentially distinguishes between the form's types and the types of the data to transfer, and translates from one to the other.
The similarity between the two may be motivating us to bypass some of the type system, but I think we can still work with it.

I do think it would be better to adjust this and not to use as, but my position here is non-blocking. But I'm marking this comment blocking to help surface it in case your Satisfied Disposition is hiding the thread. If you want to keep the current implementation, we can but I want to check on that.

I was thinking I don't think this PR makes it any better or worse and therefore wasn't planning to address it in this PR.

Thanks for making sure the comment wasn't lost though.

[marksvc]

@marksvc resolved 1 discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on Nateowami).