SF-3577 Fix Stay on Page option in Draft Formatting

[josephmyers]

Apparently, the Location object is not aware of navigation guards, where Router is. The Location object was popping off the top item on history immediately after clicking Cancel, even when the confirm prompt returned "stay on page." Changing to use Router fixes the issue.

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.30%. Comparing base (eea0a15) to head (74b6bf2).

Files with missing lines	Patch %	Lines
...n/draft-usfm-format/draft-usfm-format.component.ts	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3467      +/-   ##
==========================================
- Coverage   82.30%   82.30%   -0.01%     
==========================================
  Files         613      613              
  Lines       36877    36879       +2     
  Branches     6046     6047       +1     
==========================================
+ Hits        30352    30353       +1     
- Misses       5629     5643      +14     
+ Partials      896      883      -13     

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

[Nateowami]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 201 at r1 (raw file):

  async close(): Promise<void> {
    await this.router.navigate(['..'], { relativeTo: this.activatedRoute });

This works? In any context I've seen before, .. in a URL does not mean backward in a history, but goes up one level in a path hierarchy. So .. relative to /projects/68c0da61d10a6e37a0c5b7d4/draft-generation/format would be equivalent to removing /format from the end of the path. If this somehow works as a way to go back, I can't see how.

[josephmyers]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 201 at r1 (raw file):

Previously, Nateowami wrote…

This works? In any context I've seen before, .. in a URL does not mean backward in a history, but goes up one level in a path hierarchy. So .. relative to /projects/68c0da61d10a6e37a0c5b7d4/draft-generation/format would be equivalent to removing /format from the end of the path. If this somehow works as a way to go back, I can't see how.

Big oops. Thanks for catching me on this. I had to solve it a different way. I'm wondering if that config change would impact anything else in the app?

[Nateowami]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @josephmyers)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 201 at r1 (raw file):

Previously, josephmyers wrote…

Big oops. Thanks for catching me on this. I had to solve it a different way. I'm wondering if that config change would impact anything else in the app?

In my opinion, the simplest solution is this:

cancelClicked(): void {
  if (this.changesHaveBeenMade && !(await this.confirmLeaveWithUnsavedChanges())) return;
  exitComponent();
}

exitComponent(): void {
  this.location.back();
}

The fact that the logic is explicit makes it so much easier to understand what is happening. There's also no concerns about how it might impact other components, or whether automated tests properly set up the routing environment to have a correct test. When it's in some other file it's extremely difficult for a developer who's unaware it exists (it confused me when I was trying to figure out what was going on). Or at the very least the DraftNavigationAuthGuard could be moved to this file.

[josephmyers]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 201 at r1 (raw file):

Previously, Nateowami wrote…

In my opinion, the simplest solution is this:

cancelClicked(): void {
  if (this.changesHaveBeenMade && !(await this.confirmLeaveWithUnsavedChanges())) return;
  exitComponent();
}

exitComponent(): void {
  this.location.back();
}

The fact that the logic is explicit makes it so much easier to understand what is happening. There's also no concerns about how it might impact other components, or whether automated tests properly set up the routing environment to have a correct test. When it's in some other file it's extremely difficult for a developer who's unaware it exists (it confused me when I was trying to figure out what was going on). Or at the very least the DraftNavigationAuthGuard could be moved to this file.

I may be misunderstanding your full solution, but this only partially works, since it just fixes the bug when using Cancel. If you use the Back button, you can still break things following the test steps. I'm not sure how to get your solution to work with Back.

[Nateowami]

@josephmyers I wasn't considering the back button, but even considering it, I don't think we really need to care if the back button lets a user leave with unsaved changes.

[josephmyers]

Okay, so just confirming you're good with having the Back button bugged? I've pushed your solution

[Nateowami]

What do you mean by "bugged"? Do you mean simply that it won't open a confirmation dialog? I wouldn't consider that a bug.

[josephmyers]

It's bugged in the same way that Cancel is bugged (on master). On master, Cancel and Back behave the same way, and this bugfix fixes the issue for Cancel only. (The real bug is with how the browser's Back functionality works with the navigation guard and the browser history.)

[Nateowami]

The changes look a lot better now, but the confirmation dialog now appears twice when the cancel button is used.

As an aside, I don't think that my proposed solution would leave the back button with the behavior on master. My suggestion is/was:

• Remove all deactivate guards
• When user clicks back, allow the browser to send the user back
• When user clicks cancel, if changes have been made, open confirmation dialog, and depending on selection, go back

To me that seemed the simplest. I think you may be close to a simple-ish solution that confirms upon use of the back button as well, so don't feel like you have to go down the route I was proposing, but I'm leaving the explanation here for the sake of clarifying what I was suggesting.

[josephmyers]

Ok, I see now that you had later suggested changing the Back button behavior altogether. I think I was interpreting your suggestion differently because I think that kind of change will cause problems, especially for desktop users with short screens and mobile users. But maybe you're seeing something I'm not? I will make the change.

[Nateowami]

@josephmyers My opinion is based on the following:

• I don't see a clear path to showing a confirmation dialog for the back button without spending a lot of time on it.
• I don't see a lot of value.

So it seems like something that could take a lot of time for very little benefit.

[josephmyers]

That's totally fair. (My second solution, the first one that actually worked, fixed both Cancel and Back, since it fixes the browser behavior, not just a button)

[Nateowami]

My second solution, the first one that actually worked, fixed both Cancel and Back, since it fixes the browser behavior, not just a button

I never tested a change that actually fixed both scenarios. Can you please provide the commit hash that you think was working for both scenarios?

@Nateowami reviewed 1 of 1 files at r3, 1 of 2 files at r4, 1 of 3 files at r6.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.spec.ts line 133 at r6 (raw file):

    expect(env.component.paragraphFormat.value).toBe(ParagraphBreakFormat.BestGuess);
    expect(env.component.quoteFormat.value).toBe(QuoteFormat.Denormalized);
    expect(await env.component['confirmLeave']()).toBe(true);

Marking a member private and then using string keys makes it harder to maintain without providing an appreciable benefit.

[josephmyers]

7dc9ea8

Edit: after rebase, bd187a1

[Nateowami]

@josephmyers I think that change is probably fine. Can you reset this PR to that commit and move it to ready to test?