SF-3772 Prevent duplicate onboarding requests#3784
Conversation
Nateowami
added
the
will require testing
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3784 +/- ##
==========================================
- Coverage 81.01% 80.99% -0.02%
==========================================
Files 630 630
Lines 40637 40645 +8
Branches 6602 6601 -1
==========================================
Hits 32922 32922
+ Misses 6691 6686 -5
- Partials 1024 1037 +13 ☔ View full report in Codecov by Sentry. |
Nateowami
force-pushed
the
feature/SF-3772-prevent-duplicate-onboarding-requests
branch
from
1c4ac76 to
dec8b56
Compare
marksvc
left a comment
marksvc
left a comment
There was a problem hiding this comment.
@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
force-pushed
the
feature/SF-3772-prevent-duplicate-onboarding-requests
branch
from
dec8b56 to
cdb6283
Compare
Nateowami
force-pushed
the
feature/SF-3772-prevent-duplicate-onboarding-requests
branch
3 times, most recently
from
4ebb88a to
a10bbcc
Compare
Nateowami
left a comment
Nateowami
left a comment
There was a problem hiding this comment.
@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.
Nateowami
force-pushed
the
feature/SF-3772-prevent-duplicate-onboarding-requests
branch
from
a10bbcc to
59ad7d9
Compare
| async onSubmit(): Promise<void> { | ||
| const projectId = this.activatedProject.projectId; | ||
| if (projectId == null || this.uiState === 'submitting') { | ||
| return; | ||
| } | ||
| if (projectId == null || this.uiState === 'submitting') return; | ||
|
|
||
| if (await this.checkAndWarnIfAlreadySubmitted()) return; |
There was a problem hiding this comment.
📝 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.
There was a problem hiding this comment.
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.
Nateowami
force-pushed
the
feature/SF-3772-prevent-duplicate-onboarding-requests
branch
2 times, most recently
from
2b005bf to
d0845ba
Compare
|
@marksvc This is waiting for re-review. |
marksvc
left a comment
marksvc
left a comment
There was a problem hiding this comment.
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.
| async onSubmit(): Promise<void> { | ||
| const projectId = this.activatedProject.projectId; | ||
| if (projectId == null || this.uiState === 'submitting') { | ||
| return; | ||
| } | ||
| if (projectId == null || this.uiState === 'submitting') return; | ||
|
|
||
| if (await this.checkAndWarnIfAlreadySubmitted()) return; |
| dateCreated: string; | ||
| } |
There was a problem hiding this comment.
📝 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.
Nateowami
force-pushed
the
feature/SF-3772-prevent-duplicate-onboarding-requests
branch
from
d0845ba to
c2048b1
Compare
Nateowami
left a comment
Nateowami
left a comment
There was a problem hiding this comment.
@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.
| dateCreated: string; | ||
| } |
|
Previously, Nateowami 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 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
left a comment
Nateowami
left a comment
There was a problem hiding this comment.
@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 explicitlyInstead 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
asneeded, 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.
|
Previously, Nateowami wrote…
Yeah, I don't recommend we do that, but I hope the methodology was communicated. |
|
Previously, marksvc wrote…
(I'm also don't think it's super sensible to have both an |
2 participants
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