[#13594] Question incorrectly displayed as saved when there are submission errors#13595
[#13594] Question incorrectly displayed as saved when there are submission errors#13595samuelfangjw wants to merge 5 commits intoTEAMMATES:masterfrom
Conversation
| this.isSaved = true; | ||
| this.hasResponseChanged = false; | ||
| this.model.hasResponseChangedForRecipients.forEach( | ||
| (_hasResponseChangedForRecipient: boolean, recipientId: string) => { | ||
| this.model.hasResponseChangedForRecipients.set(recipientId, false); | ||
| }); |
There was a problem hiding this comment.
These should only be set after saving is completed so they have been removed.
There was a problem hiding this comment.
Pull request overview
Fixes an issue in the feedback session submission UI where a question tab could be shown as “saved” even when submissions fail, by tightening the conditions under which “saved/changed” state is updated.
Changes:
- Removes
isSubmitAllClickedstate plumbing and centralizes “changed” tracking onhasResponseChangedForRecipients. - Updates save flow to reset “changed” flags only on successful save, and renames
getformattedSessionClosingTimetogetFormattedSessionClosingTime. - Updates unit tests and snapshots to reflect the new state tracking and template bindings.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/web/app/pages-session/session-submission-page/session-submission-page.component.ts | Adjusts save flow and “changed” reset logic; renames session closing-time formatter; changes recipient-save error handling. |
| src/web/app/pages-session/session-submission-page/session-submission-page.component.spec.ts | Updates save-flow tests to reflect new hasResponseChangedForRecipients behavior and updated method signature. |
| src/web/app/pages-session/session-submission-page/session-submission-page.component.html | Removes isSubmitAllClicked bindings and updates save handler calls to match the new signature. |
| src/web/app/pages-session/session-submission-page/snapshots/session-submission-page.component.spec.ts.snap | Snapshot updates removing the isSubmitAllClicked input. |
| src/web/app/components/question-submission-form/question-submission-form.component.ts | Replaces mutable hasResponseChanged with a getter; simplifies saved-state calculation; removes isSubmitAllClicked input/output. |
| src/web/app/components/question-submission-form/question-submission-form.component.spec.ts | Updates/extends tests for the new derived hasResponseChanged and revised saved-state behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/web/app/components/question-submission-form/question-submission-form.component.ts
Outdated
Show resolved
Hide resolved
src/web/app/pages-session/session-submission-page/session-submission-page.component.ts
Show resolved
Hide resolved
src/web/app/pages-session/session-submission-page/session-submission-page.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| triggerRecipientSubmissionFormChange(index: number, field: string, data: any): void { | ||
| if (!this.isFormsDisabled) { | ||
| this.hasResponseChanged = true; | ||
| this.isSubmitAllClickedChange.emit(false); | ||
| this.model.hasResponseChangedForRecipients.set(this.model.recipientList[index].recipientIdentifier, true); | ||
| if (this.isFormsDisabled) { | ||
| return; | ||
| } | ||
|
|
||
| this.model.recipientSubmissionForms[index] = | ||
| { | ||
| ...this.model.recipientSubmissionForms[index], | ||
| [field]: data, | ||
| }; | ||
| const recipientIdentifier: string = this.model.recipientSubmissionForms[index].recipientIdentifier; | ||
| this.model.hasResponseChangedForRecipients.set(recipientIdentifier, true); | ||
|
|
||
| this.updateIsValidByQuestionConstraint(); | ||
| this.formModelChange.emit(this.model); | ||
| } | ||
| this.model.recipientSubmissionForms[index] = | ||
| { | ||
| ...this.model.recipientSubmissionForms[index], | ||
| [field]: data, | ||
| }; | ||
|
|
||
| this.updateIsValidByQuestionConstraint(); | ||
| this.formModelChange.emit(this.model); |
There was a problem hiding this comment.
In triggerRecipientSubmissionFormChange, when the changed field is recipientIdentifier (flexible-recipient dropdown), the code currently marks hasResponseChangedForRecipients using the old recipientIdentifier from the model before it gets updated. This can leave a stale key (e.g., '' or a previously selected recipient) stuck at true, causing hasResponseChanged / the saved-state to remain incorrect even after the user picks a different recipient. Consider using the new identifier (data) when field === 'recipientIdentifier' (and potentially clearing the old key) so the change-tracking maps to the currently selected recipient.
src/web/app/components/question-submission-form/question-submission-form.component.spec.ts
Show resolved
Hide resolved
samuelfangjw
force-pushed
the
save-state-failure
branch
from
473e9fe to
71eb7ab
Compare
Fixes #13594
Outline of Solution
Root cause was that
hasResponseChangedForRecipientsandisSavedis unconditionally set even when there are submission failures.Solution is to only set these when it has been successfully saved. In addition,
hasResponseChangedis changed to be dependent onhasResponseChangedForRecipients. Previously these two variables were set independently leading to inconsistencies.