Csfd 15 ungrey back button #1706
Conversation
We're in the process of debugging an issue where the user hits the review page & then hits change, the previous button is not always accessible.
…d when triggering a change
…he case flag journey
CSFD-18 Event flow cancel navigation
|
| @@ -434,6 +493,20 @@ export class CaseEditPageComponent implements OnInit, AfterViewChecked, OnDestro | |||
| } | |||
|
|
|||
| public cancel(): void { | |||
| if (this.isLinkedCasesJourney()){ | |||
There was a problem hiding this comment.
Minor comment - add cleanUpData method so that this can be used elsewhere if needed
There was a problem hiding this comment.
Or at least think about it
There was a problem hiding this comment.
I have moved it to its own reset function
| tab?.fields?.some((field) => field.id === 'caseLinks') | ||
| )?.fields?.[0] ?? null; | ||
| const initalLinks = this.linkedCasesService.initialCaseLinkRefs; | ||
| if (linkedCasesTab && linkedCasesTab?.value.length !== initalLinks.length){ |
There was a problem hiding this comment.
Another minor comment - a few assumptions made here, i.e. that there is this.caseEdit.caseDetails.tabs and that linkedCasesTab.value exists, just check that they definitely will
There was a problem hiding this comment.
Added additional checks for properties
| @@ -36,6 +38,9 @@ import { CaseEditSubmitTitles } from './case-edit-submit-titles.enum'; | |||
| import { CaseEditSubmitComponent } from './case-edit-submit.component'; | |||
|
|
|||
| import createSpyObj = jasmine.createSpyObj; | |||
| import { CaseFlagStateService } from '../services/case-flag-state.service'; | |||
| import { LinkedCasesService } from '../../palette/linked-cases/services/linked-cases.service'; | |||
| import exp from 'constants'; | |||
There was a problem hiding this comment.
Minor comment - should constants not be a relative import i.e. ./constants?
There was a problem hiding this comment.
Moved imports and removed unused imports
| it('should display the "Previous" button if submission is not for a Case Flag', () => { | ||
| comp.eventTrigger.case_fields = []; | ||
| fixture.detectChanges(); | ||
| expect(comp.caseEdit.isCaseFlagSubmission).toBe(false); | ||
| const previousButton = de.query(By.css('.button-secondary')); | ||
| expect(previousButton.nativeElement.textContent).toContain('Previous'); | ||
| }); | ||
|
|
||
| it('should remove any unsibmitted data on cancel', () => { |
| @@ -167,6 +177,20 @@ export class CaseEditSubmitComponent implements OnInit, OnDestroy { | |||
| } | |||
|
|
|||
| public cancel(): void { | |||
| if (this.caseEdit.isLinkedCasesSubmission){ | |||
| // if they cancel on the last CYA page we should ensure there is no unsubmitted data in the caseDetails | |||
There was a problem hiding this comment.
Minor comment - Again could create other reusable method for this (removeUnsubmittedData)
There was a problem hiding this comment.
Split to child functions
| this.linkedCasesStateEmitter.emit({ | ||
| currentLinkedCasesPage: LinkedCasesPages.BEFORE_YOU_START, | ||
| errorMessages: this.errorMessages, | ||
| navigateToNextPage: true | ||
| }); | ||
| } | ||
|
|
||
| public next() { |
There was a problem hiding this comment.
public next(): void {
Also onNext and next as two methods placed together is confusing, can they be renamed and/or commented?
There was a problem hiding this comment.
Not sure why it was done like this, have simplified to one next function
| import { CaseLink, LinkedCasesState } from './domain'; | ||
| import { LinkedCasesEventTriggers, LinkedCasesPages } from './enums'; | ||
| import { LinkedCasesService } from './services'; | ||
| import { Subscription } from 'rxjs'; | ||
| import { MultipageComponentStateService } from '../../../services'; | ||
| import { Router } from '@angular/router'; |
There was a problem hiding this comment.
External imports before internal imports
| private readonly caseEditDataService: CaseEditDataService, | ||
| private router: Router, | ||
| multipageComponentStateService: MultipageComponentStateService) { | ||
| super(multipageComponentStateService); |
There was a problem hiding this comment.
Indentation confusing here
There was a problem hiding this comment.
bumped ) { to next line
| } | ||
|
|
||
| public ngOnInit(): void { | ||
| const trigUrl = location.href; |
There was a problem hiding this comment.
Minor pedantic comment (sorry there's quite a few of these now) - trigUrl could be more descriptive e.g. triggeredUrl or toTriggerUrl depending on its usage
There was a problem hiding this comment.
Valid point, renamed to trigger
|
|
||
| public handleBackButton(event) { | ||
| event.preventDefault(); | ||
| if (this.linkedCasesPage === 0){ |
There was a problem hiding this comment.
Another minor pedantic comment - see quite a few instances of ){ at the end of ifs and methods. Just did a find for '){' and there are 31 instances of it on the full change summary. Linting is obviously not picking it up but as it is not consistent with the application I would recommend changing them, especially for the end of methods/functions
There was a problem hiding this comment.
Done a search through the changes and updated all instances of ){
| @@ -58,6 +62,9 @@ import { CaseEditPageText } from './case-edit-page-text.enum'; | |||
| import { CaseEditPageComponent } from './case-edit-page.component'; | |||
| import { ShowCondition } from '../../../directives'; | |||
| import createSpyObj = jasmine.createSpyObj; | |||
| import { LinkedCasesService } from '../../palette/linked-cases/services/linked-cases.service'; | |||
| import { CaseFlagStateService } from '../services/case-flag-state.service'; | |||
| import { CaseTab } from 'ccd-case-ui-toolkit'; | |||
There was a problem hiding this comment.
Minor comment - External imports before internal imports
There was a problem hiding this comment.
Think as it was a large diff I missed it before
| }) => | ||
| new CaseEditPageComponent( | ||
| new CaseEditPageComponent( |
There was a problem hiding this comment.
Very minor comment - Indentation not needed here (can ignore)
| expect(caseFlagStateService.lastPageFieldState).toEqual(3); | ||
| }); | ||
|
|
||
| it('should remove any unsibmitted data on cancel', () => { |
| const initialCaseLinks = this.linkedCasesService.initialCaseLinkRefs || []; | ||
| console.log(initialCaseLinks); | ||
| console.log(proposedCaseLink); | ||
| return initialCaseLinks.includes(proposedCaseLink); |
There was a problem hiding this comment.
@Josh-HMCTS can you remove these console.log
| // the journey can become broken in some situations as the journeyPageNumber is not correct, account for this and set to the correct value | ||
| const journeyPageNumber = this.getJourneyCollection()['journeyPageNumber']; | ||
| const linkedCasesPageNumber = this.getJourneyCollection()['linkedCasesPage']; | ||
| console.log(journeyPageNumber, linkedCasesPageNumber) |
There was a problem hiding this comment.
@Josh-HMCTS same here if we don't need it then we can remove it.
| public validateTranslationNeeded(): void { | ||
| // it is possible that the user can select to have translation and then navigate back to remove the required translation in the same journey | ||
| // this function will check the user does not have any of the translation fields applied and remove if they do | ||
| console.log('validateTranslateNeeded'); |
There was a problem hiding this comment.
@Josh-HMCTS if we don't need this console.log, then remove it.
|
JIRA link (if applicable)
Change Description
I've included the epic above to show how there are other items of work associated to this. The purpose of this pull request is to ensure that the existing changes get pushed into master, while they're not 100% complete, they're an improvement on what is currently there in production as of the time of writing this. There are still some issues with the implementation, it's not perfect, but it is an improvement.
Does this PR introduce a breaking change? (check one with "x")