-
-
Notifications
You must be signed in to change notification settings - Fork 5
Integrate onboarding form #3612
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
...entApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts
Fixed
Show fixed
Hide fixed
...entApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts
Fixed
Show fixed
Hide fixed
644e5f9 to
6aceae5
Compare
6aceae5 to
b64c163
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR integrates an in-app onboarding form for draft generation signup, replacing the previous external link approach. The implementation spans backend (C#) and frontend (Angular) with comprehensive CRUD operations, admin management interface, and E2E testing.
Key Changes
- Added backend RPC controller and database models for onboarding requests with admin management capabilities
- Created frontend form component with validation, conditional fields, and project/resource selection
- Implemented Serval admin interface for viewing, managing, and processing onboarding requests
- Refactored
ProjectSelectComponentvalidation to support custom validators and error message mapping
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
src/SIL.XForge/Controllers/UrlConstants.cs |
Added URL constant for onboarding requests endpoint |
src/SIL.XForge.Scripture/Services/SFJsonRpcApplicationBuilderExtensions.cs |
Registered onboarding RPC controller |
src/SIL.XForge.Scripture/Models/OnboardingRequest.cs |
Backend data models for onboarding requests, submissions, and comments |
src/SIL.XForge.Scripture/DataAccess/SFDataAccessServiceCollectionExtensions.cs |
MongoDB repository configuration for onboarding requests |
src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs |
RPC endpoints for submission, retrieval, and admin management |
src/SIL.XForge.Scripture/ClientApp/tsconfig.json |
Disabled noUnusedLocals compiler option |
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/url-constants.ts |
Added frontend URL constant |
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/notice.service.ts |
Changed loading state implementation from setTimeout to queueMicrotask |
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/feature-flags/feature-flag.service.ts |
Added feature flag for in-app draft signup form |
src/SIL.XForge.Scripture/ClientApp/src/material-styles.scss |
Imported theme files for new components |
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json |
Added localization strings for signup form and related UI |
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/drafting-signup.service.ts |
Service for onboarding request CRUD operations |
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.* |
Main signup form component with reactive forms and validation |
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.* |
Updated to show signup button and existing request status |
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/* |
Refactored validation logic from custom validators to inline functions |
src/SIL.XForge.Scripture/ClientApp/src/app/shared/sfvalidators.ts |
Removed CustomValidatorState enum and customValidator method |
src/SIL.XForge.Scripture/ClientApp/src/app/shared/dev-only/* |
New component for dev-only content visibility |
src/SIL.XForge.Scripture/ClientApp/src/app/settings/settings.component.ts |
Changed to use MatButtonModule and MatIconModule |
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-administration.service.ts |
Added getByParatextId method |
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-administration.component.* |
Added draft requests tab |
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-request-detail.component.* |
Detail view for individual onboarding requests |
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-request-constants.ts |
Constants for status and resolution labels |
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-onboarding-requests/* |
Admin list view for managing onboarding requests |
src/SIL.XForge.Scripture/ClientApp/src/app/project-select/project-select.component.* |
Refactored to support external validators and custom error messages |
src/SIL.XForge.Scripture/ClientApp/src/app/app.routes.ts |
Added routes for signup form and request detail |
src/SIL.XForge.Scripture/ClientApp/e2e/workflows/submit-draft-signup.ts |
E2E test for form submission |
.github/copilot-instructions.md |
Added guideline about using CSS variables/themes instead of hard-coded colors |
| }); | ||
| } | ||
|
|
||
| private nextLoadingValue = this.isAppLoading; |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nextLoadingValue field is not initialized with a type annotation. According to the coding guidelines, specify types when declaring variables. This should be private nextLoadingValue: boolean = this.isAppLoading;
| async approveRequest(options: { requestId: string; sfProjectId: string }): Promise<OnboardingRequest> { | ||
| const requestUpdateResult = await this.onlineInvoke<OnboardingRequest | undefined>('setResolution', { | ||
| requestId: options.requestId, | ||
| resolution: 'approved' | ||
| }); | ||
| await this.projectService.onlineSetPreTranslate(options.sfProjectId, true); | ||
| return requestUpdateResult!; | ||
| } | ||
|
|
||
| /** Submits a new signup request. */ | ||
| async submitOnboardingRequest(projectId: string, formData: DraftingSignupFormData): Promise<string> { | ||
| return (await this.onlineInvoke<string>('submitOnboardingRequest', { projectId, formData }))!; | ||
| } | ||
|
|
||
| /** Gets the existing signup request for the specified project, if any. */ | ||
| async getOpenOnboardingRequest(projectId: string): Promise<OnboardingRequest | null> { | ||
| return (await this.onlineInvoke<OnboardingRequest | null>('getOpenOnboardingRequest', { projectId }))!; | ||
| } | ||
|
|
||
| /** Gets all onboarding requests (Serval admin only). */ | ||
| async getAllRequests(): Promise<OnboardingRequest[]> { | ||
| return (await this.onlineInvoke<OnboardingRequest[]>('getAllRequests'))!; | ||
| } | ||
|
|
||
| /** Sets the assignee for an onboarding request (Serval admin only). */ | ||
| async setAssignee(requestId: string, assigneeId: string): Promise<OnboardingRequest> { | ||
| return (await this.onlineInvoke<OnboardingRequest | undefined>('setAssignee', { requestId, assigneeId }))!; | ||
| } | ||
|
|
||
| /** Sets the resolution of an onboarding request (Serval admin only). */ | ||
| async setResolution(requestId: string, resolution: string | null): Promise<OnboardingRequest> { | ||
| return (await this.onlineInvoke<OnboardingRequest | undefined>('setResolution', { requestId, resolution }))!; |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onlineInvoke method is called with non-null assertion operators (!) on lines 67, 72, 77, 82, 92, and 97. According to the coding guidelines, prefer explicit null/undefined checks rather than relying on truthy/falsy or assertions. These should return the type Promise<T | undefined> without the non-null assertion, or should explicitly check for undefined before returning.
...c/app/serval-administration/draft-onboarding-requests/draft-onboarding-requests.component.ts
Outdated
Show resolved
Hide resolved
...c/app/serval-administration/draft-onboarding-requests/draft-onboarding-requests.component.ts
Show resolved
Hide resolved
...SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/drafting-signup.service.ts
Outdated
Show resolved
Hide resolved
...SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/drafting-signup.service.ts
Show resolved
Hide resolved
...entApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts
Show resolved
Hide resolved
...entApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts
Show resolved
Hide resolved
| catch (Exception exception) | ||
| { | ||
| _exceptionHandler.RecordEndpointInfoForException( | ||
| new Dictionary<string, string> | ||
| { | ||
| { "method", "SubmitOnboardingRequest" }, | ||
| { "projectId", projectId }, | ||
| { "userId", UserId }, | ||
| } | ||
| ); | ||
| // report the exception without failing the whole request | ||
| _exceptionHandler.ReportException(exception); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 days ago
In general, this problem is fixed by replacing a broad catch (Exception) with more specific exception types that correspond to the failures we explicitly want to handle, or by using an exception filter to exclude cancellation/critical exceptions while still logging them. We should still preserve the current behavior: failures in sending admin notification emails must not fail the whole onboarding request, but we can improve the specificity of what is treated as an “expected” operational failure.
The email-sending code is the only region we need to change. Since we don’t see the implementation of IEmailService, we cannot know its exact exception types, and we must avoid introducing project-specific dependencies. A safe and common pattern in .NET is to use an exception filter on Exception to exclude OperationCanceledException (and derived types), which are better allowed to propagate or be handled elsewhere, while still catching and logging any other exception. To keep behavior consistent (the whole onboarding request should remain successful even if email fails), we will (1) introduce a catch (OperationCanceledException) that rethrows, so cancellations are not swallowed, and (2) modify the generic catch to use a filter when (exception is not OperationCanceledException) to handle and log non-cancellation failures. This narrows the catch clause according to intent and improves diagnosability, while preserving the existing success path for the caller.
Concretely, in src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs, within SubmitOnboardingRequest, we will replace the existing catch (Exception exception) block at lines 103–115 with two catch blocks: one specific catch (OperationCanceledException) that rethrows, and one catch (Exception exception) when (exception is not OperationCanceledException) that logs via _exceptionHandler as before. No new imports or helper methods are required, as OperationCanceledException is already in System.
-
Copy modified line R103 -
Copy modified lines R105-R109 -
Copy modified line R117
| @@ -100,8 +100,13 @@ | ||
| await emailService.SendEmailAsync(email, subject, body); | ||
| } | ||
| } | ||
| catch (Exception exception) | ||
| catch (OperationCanceledException) | ||
| { | ||
| // Propagate cancellations so they can be handled appropriately upstream. | ||
| throw; | ||
| } | ||
| catch (Exception exception) when (exception is not OperationCanceledException) | ||
| { | ||
| _exceptionHandler.RecordEndpointInfoForException( | ||
| new Dictionary<string, string> | ||
| { | ||
| @@ -110,7 +114,7 @@ | ||
| { "userId", UserId }, | ||
| } | ||
| ); | ||
| // report the exception without failing the whole request | ||
| // Report the exception without failing the whole request | ||
| _exceptionHandler.ReportException(exception); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional.
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
b64c163 to
e612e74
Compare
RaymondLuong3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this branch out and it is working well. I deleted a request and that deleted the record in mongo. I think this is probably OK. It allows the project to submit a new request, which is what I expected would happen.
@RaymondLuong3 partially reviewed 47 files and all commit messages, and made 14 comments.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @Nateowami).
src/SIL.XForge.Scripture/ClientApp/e2e/workflows/submit-draft-signup.ts line 131 at r3 (raw file):
// Back translation project (required when stage is written) await selectProjectByFieldName(page, user, 'Select your back translation', 'DHH94');
Not really a back translation but I'll let that slide.
Code quote:
await selectProjectByFieldName(page, user, 'Select your back translation', 'DHH94');src/SIL.XForge.Scripture/ClientApp/e2e/test_characterization.json line 24 at r3 (raw file):
"submit_draft_signup": { "success": 0, "failure": 0
This looks suspicious that both are assigned the value 0.
Code quote:
"submit_draft_signup": {
"success": 0,
"failure": 0src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 52 at r3 (raw file):
// selected) even after the user has made the selection (though in the ProjectSelectComponent itself the selection // exists and is known to be valid). changeDetection: ChangeDetectionStrategy.OnPush,
Thanks for the explanation.
Code quote:
changeDetection: ChangeDetectionStrategy.OnPush,src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 222 at r3 (raw file):
} private async loadProjectsAndResources(): Promise<void> {
These private functions should be moved to the end of the file.
Code quote:
private async loadProjectsAndResources(): Promise<void> {src/SIL.XForge.Scripture/Models/OnboardingRequest.cs line 107 at r3 (raw file):
public string TranslationLanguageIsoCode { get; set; } = string.Empty; public int[]? CompletedBooks { get; set; } public int[]? NextBooksToDraft { get; set; }
This can probably be initialized to an empty array and not be nullable.
Code quote:
public int[]? CompletedBooks { get; set; }
public int[]? NextBooksToDraft { get; set; }src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 36 at r3 (raw file):
private readonly ISFProjectService _projectService = projectService; public async Task<IRpcMethodResult> SubmitOnboardingRequest(string projectId, OnboardingRequestFormData formData)
Add a summary statement here.
Code quote:
public async Task<IRpcMethodResult> SubmitOnboardingRequest(string projectId, OnboardingRequestFormData formData)src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 135 at r3 (raw file):
{ // Create the resource/source project and add the user to it string sourceProjectId = await _projectService.CreateResourceProjectAsync(
What is the reason we need to create the project on SF at this time? Is it so that the serval admin has the ability to download the PT project?
Code quote:
string sourceProjectId = await _projectService.CreateResourceProjectAsync(src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.html line 46 at r3 (raw file):
<mat-option value="Bolshoi Group">Bolshoi Group</mat-option> <mat-option value="Seed Company">Seed Company</mat-option> <mat-option value="none">{{ t("partner_none") }}</mat-option>
We should probably read these companies from the component and iterate on the names rather than coding it directly into the template.
Code quote:
<mat-option value="Bolshoi Group">Bolshoi Group</mat-option>
<mat-option value="Seed Company">Seed Company</mat-option>
<mat-option value="none">{{ t("partner_none") }}</mat-option>src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.html line 65 at r3 (raw file):
<mat-card-content> <div> <div class="form-field-label">{{ t("translation_language_name_label") }}</div>
This can probably be removed since it is in the textbox label.
Code quote:
<div class="form-field-label">{{ t("translation_language_name_label") }}</div>src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.html line 77 at r3 (raw file):
<div> <!-- <div class="form-field-label">{{ t("translation_language_iso_label") }}</div> -->
This line can be removed since the label is already in the textbox.
Code quote:
<!-- <div class="form-field-label">{{ t("translation_language_iso_label") }}</div> -->src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-request-detail.component.html line 76 at r3 (raw file):
<mat-icon>check_circle</mat-icon> Approve & Enable </button> <button mat-button color="warn" (click)="deleteRequest()"><mat-icon>delete</mat-icon> Delete Request</button>
This doesn't look like it has an effect.
Code quote:
color="warn" src/SIL.XForge.Scripture/ClientApp/src/xforge-common/feature-flags/feature-flag.service.ts line 366 at r3 (raw file):
); readonly inAppDraftSignupForm: ObservableFeatureFlag = new FeatureFlagFromStorage(
When adding a new feature flag you should ad it to parse-verse.ts also.
Code quote:
readonly inAppDraftSignupForm: ObservableFeatureFlag = new FeatureFlagFromStorage(src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 134 at r3 (raw file):
* Called after loading all requests or after updating individual requests. */ private initializeRequestData(): void {
I normally expect private functions to come at the end of the file. Can you move these private functions to the end?
Code quote:
private initializeRequestData(): void {2050891 to
785b846
Compare
Nateowami
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, being able to delete the requests is intended. The normal flow is that it is submitted and processed and marked complete without being deleted. Deletion provides additional flexibility (and makes more scenarios possible to test).
@Nateowami made 6 comments.
Reviewable status: 37 of 47 files reviewed, 17 unresolved discussions (waiting on @RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/e2e/test_characterization.json line 24 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
This looks suspicious that both are assigned the value 0.
I have now run the test characterization.
src/SIL.XForge.Scripture/ClientApp/e2e/workflows/submit-draft-signup.ts line 131 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Not really a back translation but I'll let that slide.
Yeah, it really doesn't make any difference. A lot of projects have back translations that are not marked as back translations in Paratext.
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/draft-request-detail.component.html line 76 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
This doesn't look like it has an effect.
I've opened a PR that addresses this: #3621
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.ts line 222 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
These private functions should be moved to the end of the file.
Done.
src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 135 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
What is the reason we need to create the project on SF at this time? Is it so that the serval admin has the ability to download the PT project?
Yes. This is actually one of the most important improvements this form provides.
RaymondLuong3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RaymondLuong3 reviewed 10 files and all commit messages, made 1 comment, and resolved 5 discussions.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Nateowami).
src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 135 at r3 (raw file):
Previously, Nateowami wrote…
Yes. This is actually one of the most important improvements this form provides.
Ok I see. It might be a surprise when a user discovers that their source project is already connected to Scripture Forge without them explicitly doing so, but it is conceivable that choosing a source at this step would result in that source being connected.
785b846 to
761d907
Compare
Nateowami
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nateowami made 7 comments and resolved 4 discussions.
Reviewable status: 44 of 47 files reviewed, 8 unresolved discussions (waiting on @RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.html line 46 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
We should probably read these companies from the component and iterate on the names rather than coding it directly into the template.
If we needed to reference these elsewhere, I'd agree. But for static values that are just saved as a string in the model, I don't see any value to that.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.html line 65 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
This can probably be removed since it is in the textbox label.
Good catch.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-signup-form/draft-onboarding-form.component.html line 77 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
This line can be removed since the label is already in the textbox.
Good catch.
src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs line 36 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Add a summary statement here.
Done.
src/SIL.XForge.Scripture/Models/OnboardingRequest.cs line 107 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
This can probably be initialized to an empty array and not be nullable.
Done.
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 134 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
I normally expect private functions to come at the end of the file. Can you move these private functions to the end?
For some other files I've moved private methods to the bottom. However, I find it to be an odd rule that I think harms readability.
I was about to move these private methods to the bottom as I've done with some others, but immediately noticed that it would make it hard to see how the component initializes. Right now you can read the constructor and see that it only inits its parent, the notice service. Then you can read ngOnInit and see that it sets the current user ID and kicks off an async method called loadRequests. loadRequests is private because we'd never expect anything outside the component to call it. But it's also the very next method in the file because it's the next logical thing the component does. If you had to scroll hundreds of lines it would be much harder to understand how the component works. You can then read loadRequests and see that it calls initializeRequestData, which is also the very next method. That method then calls loadProjectNames.
| catch (Exception exception) | ||
| { | ||
| _exceptionHandler.RecordEndpointInfoForException( | ||
| new Dictionary<string, string> | ||
| { | ||
| { "method", "SubmitOnboardingRequest" }, | ||
| { "projectId", projectId }, | ||
| { "userId", UserId }, | ||
| } | ||
| ); | ||
| // report the exception without failing the whole request | ||
| _exceptionHandler.ReportException(exception); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional.
761d907 to
c164e3f
Compare
Nateowami
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nateowami made 1 comment.
Reviewable status: 44 of 48 files reviewed, 8 unresolved discussions (waiting on @RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/src/xforge-common/feature-flags/feature-flag.service.ts line 366 at r3 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
When adding a new feature flag you should ad it to parse-verse.ts also.
Done
RaymondLuong3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think other than the failing tests it would be good to pass this along to testers to see what you will find. It is possible to exclude tests and come back to them later as long as the code is functional.
@RaymondLuong3 reviewed 4 files and all commit messages, made 3 comments, and resolved 6 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.html line 46 at r3 (raw file):
Previously, Nateowami wrote…
If we needed to reference these elsewhere, I'd agree. But for static values that are just saved as a string in the model, I don't see any value to that.
That is reasonable to me.
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 134 at r3 (raw file):
Previously, Nateowami wrote…
For some other files I've moved private methods to the bottom. However, I find it to be an odd rule that I think harms readability.
I was about to move these private methods to the bottom as I've done with some others, but immediately noticed that it would make it hard to see how the component initializes. Right now you can read the constructor and see that it only inits its parent, the notice service. Then you can read
ngOnInitand see that it sets the current user ID and kicks off an async method calledloadRequests.loadRequestsis private because we'd never expect anything outside the component to call it. But it's also the very next method in the file because it's the next logical thing the component does. If you had to scroll hundreds of lines it would be much harder to understand how the component works. You can then readloadRequestsand see that it callsinitializeRequestData, which is also the very next method. That method then callsloadProjectNames.
Ok, I see what you mean. The natural flow of the processes is more important than grouping public and private methods. I have appreciated having the conceptual grouping of public methods together to help me understand how a component is exposed to other components, but I do agree that in some situations it makes sense to layout the code in a linear way.
Two tests are still failing, and I haven't had time to fix them yet.
Here's a quick walkthrough.
Enable the feature flag
Fill out the form
View the list of submissions
View/manage an individual submission
This change is