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

[tsullivan]

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.

Scenario:

1. Create a plugin with a boolean set to false for uiExports.app.linkToLastSubUrl:

   export default function myPluginIndex(kibana) {
     return new kibana.Plugin({
       require: ['kibana', 'elasticsearch'],
       id: 'myPlugin',
       publicDir: resolve(__dirname, 'public'),
       uiExports: {
         app: {
           title: 'My Plugin',
           order: 200,
           description: 'Test Plugin Setup',
           icon: 'plugins/myPlugin/myPlugin.svg',
           main: 'plugins/myPlugin/myPlugin',
           linkToLastSubUrl: false // this one is the relevant field for this bug
         }
       },
       config: function (Joi) {
         // ...
       }
     });
   }

2. Expect that the linkToLastSubUrl setting will be set to false for this app plugin's navLink
3. Find that the linkToLastSubUrl field of the navLink is enabled as if the setting was true.

[tsullivan]

Note that the toJSON method doesn't appear to be used anywhere, not even in unit tests: https://github.com/elastic/kibana/pull/9011/files#diff-d79d2eb3a74f8afdf08d74679f0094eeR67

[cjcenizal]

Just needs some tests, and then I can give a 👍 !

[tsullivan]

@cjcenizal added some tests for src/ui/ui_app

[tsullivan]

@cjcenizal I made some updates to the test to analyze each field in the output objects in a better way, as you suggested.

[cjcenizal]

Awesome work! I have some suggestions, but will leave it up to you to decide whether or not to implement them.

[cjcenizal]

Can we make these assert against undefined instead?

expect(newApp.linkToLastSubUrl).to.be(undefined);

I think this will improve the messages we see in the terminal. As the tests currently are, they will say something like "expected true but got false", which won't give you any insight into what the actual value of the property was. If we change it, the message will be something like "expected undefined but got 'value'".

[tsullivan]

I deliberately didn't do that because it wouldn't actually assure anything. In JS, a field that was never set on an object evaluates to undefined

> const fooObj = {};
undefined
> fooObj.bar === undefined
true

The in operator was the only way I know of to test that a field has actually been set:

> 'bar' in fooObj
false
> fooObj.bar = 1;
1
> 'bar' in fooObj
true

[cjcenizal]

In the docs they give an example making an assertion that something is undefined:

expect(window.r).to.be(undefined);

I think all that matters in this case is that the default values of various properties is undefined when they're undefined by the spec argument. It doesn't matter whether the internal implementation is something like

this.main = this.spec.main;

or

if (this.spec.main) {
  this.main = this.spec.main;
}

[cjcenizal]

Small suggestion, but we can make these tests a bit less brittle by asserting against a reference to the spec here, e.g.

expect(newApp.main).to.be(spec.main);

[cjcenizal]

Another small suggestion, but I like to make sure the message in the terminal forms a grammatically-correct sentence, e.g. by changing this to 'is noop function by default', we will get the message:

UiApp constructor getInjectedVars is noop function by default

vs

UiApp constructor getInjectedVars noop function by default

[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', () => {
      it('when null defaults to the opposite value of hidden, when 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('when null defaults to the opposite value of hidden, when 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 undefined defaults to the opposite value of hidden, when hidden is false', () => {
        const spec = {
          id: 'uiapp-test',
          hidden: false,
        };
        const newApp = new UiApp(uiExports, spec);
        expect(newApp.listed).to.be(true);
      });

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

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

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

[tsullivan]

I don't need this feature in the foreseeable future, so I'm going to close this.