Allow plugins to turn off the “link to last URL” navigation helper

[tsullivan]

This re-implements #9011 for 6.0

This PR fixes the navLink generated from a plugin spec object to include the linkToLastSubUrl setting. Without this, the linkToLastSubUrl is always undefined, which by default is treated as if the setting was true

[cjcenizal]

Sweet, sweet tests. 🤤 Had a few comments and would like to hear your thoughts!

[cjcenizal]

Such a nit-pick but the term "built-in" isn't necessary here right? 🤓

[cjcenizal]

Or in the next two tests.

[tsullivan]

If it's in question, then probably not :)

[cjcenizal]

Do we need all of these assertions here? If one of them fails and the message is "expected true to be false", it will be tricky to figure out what went wrong. If we need all of these assertions, maybe we can split them into two tests, one for "has listed set to true if it's passed as true" and another for "has listed set to false if it's passed as false"?

[tsullivan]

I disagree that it is tricky to figure out what went wrong if there is a failure because the failure will have a stack trace that points to the failing assertion. In the past I have went through some hoops to avoid that kind of failure messaging, but eventually just simplified because my team member and myself felt it wasn't worth the extra code.

[pickypg]

Doesn't the stack trace solve this issue though? Just trace to the line of the failure.

[tsullivan]

FYI, I'm not exactly sure why Github says this is an outdated change. I didn't explicitly make any change here.

[cjcenizal]

That's so funny, I didn't even know about the stack trace. 😄 Yes that addresses my concern.

[cjcenizal]

When I was reading these tests, I was having a hard time figuring out how these two properties interacted. After I read the source, I saw that listed was dependent on hidden, and that hidden was cast to a boolean values.

I think this structure makes that relationship a little clearer. What do you think?

    describe('hidden', () => {
      describe('is cast to boolean value', () => {
        it('when undefined', () => {
          const spec = {
            id: 'uiapp-test',
          };
          const newApp = new UiApp(uiExports, spec);
          expect(newApp.hidden).to.be(false);
        });

        it('when null', () => {
          const spec = {
            id: 'uiapp-test',
            hidden: null,
          };
          const newApp = new UiApp(uiExports, spec);
          expect(newApp.hidden).to.be(false);
        });
      });
    });

    describe('listed', () => {
      describe('defaults to the opposite value of hidden', () => {
        it(`when it's null and hidden is true`, () => {
          const spec = {
            id: 'uiapp-test',
            hidden: true,
            listed: null,
          };
          const newApp = new UiApp(uiExports, spec);
          expect(newApp.listed).to.be(false);
        });

        it(`dhen it's null and hidden is false`, () => {
          const spec = {
            id: 'uiapp-test',
            hidden: false,
            listed: null,
          };
          const newApp = new UiApp(uiExports, spec);
          expect(newApp.listed).to.be(true);
        });

        it(`when it's undefined and hidden is false`, () => {
          const spec = {
            id: 'uiapp-test',
            hidden: false,
          };
          const newApp = new UiApp(uiExports, spec);
          expect(newApp.listed).to.be(true);
        });

        it(`when it's undefined and hidden is true`, () => {
          const spec = {
            id: 'uiapp-test',
            hidden: true,
          };
          const newApp = new UiApp(uiExports, spec);
          expect(newApp.listed).to.be(false);
        });
      });

      it(`is set to true when it's passed as true`, () => {
        const spec = {
          id: 'uiapp-test',
          listed: true,
        };
        const newApp = new UiApp(uiExports, spec);
        expect(newApp.listed).to.be(true);
      });

      it(`is set to false when it's passed as false`, () => {
        const spec = {
          id: 'uiapp-test',
          listed: false,
        };
        const newApp = new UiApp(uiExports, spec);
        expect(newApp.listed).to.be(false);
      });
    });

[tsullivan]

I was also a little surprised that these properties have a relationship and the logic previously had no test coverage. But I think if it is hard to figure that out just by looking at the test, would it suffice to just add a comment in the code that all these tests are here to check the logic for the relationship?

Your proposal looks good to me though, and I appreciate you taking the time to present it like that. I have no problem with integrating it. I'll add a comment as well.

[cjcenizal]

Really weird test failure!

[pickypg]

LGTM once tests pass!

[cjcenizal]

LGTM!