[Lens] Do not reset formatting when switching between custom ranges and auto histogram

[denismaxim0v]

Fixes #81749

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.
I've noticed it has been resetting the format to undefined, so I just switched it to keep the format of the current column

Checklist

Delete any items that are not applicable to this PR.

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[kibanamachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[elasticmachine]

Pinging @elastic/kibana-app (Team:KibanaApp)

[flash1293]

Thanks for working on this, @denismaxim0v ! Would you mind adding a small unit test for the new behavior?

[denismaxim0v]

Thanks for working on this, @denismaxim0v ! Would you mind adding a small unit test for the new behavior?

Sure, will do

[denismaxim0v]

Alright, just added the unit test

[flash1293]

@elasticmachine merge upstream

[flash1293]

Jenkins, test this

[flash1293]

It seems like the only difference between the "default" format of MyField and the overwrite on the column are the params. Your test doesn't really validate these params are kept - I commented out lines 763 to 766 and the tests still pass. I suggest not setting the same formatter in the field format map of the index pattern (line 759)

[denismaxim0v]

Not sure if I understood that completely. Should I switch it to test for params on the same line(759)?

[flash1293]

No, I meant setting it to the number formatter there so you can actually check whether the format is persisted.

But I just realized your test is not testing the right thing anyway. lns-customBucketContainer-remove is removing one out of multiple ranges, but that's not what should be tested here - this is about switching between custom ranges in general and automatic ranges controlled by granularity.

A proper unit test should click the "Remove custom ranges" button, then check whether the state update issued by this still includes the custom format.

Let me know when you have problems adding such a test. I can do it and add it to your PR

[denismaxim0v]

No, I meant setting it to the number formatter there so you can actually check whether the format is persisted.

But I just realized your test is not testing the right thing anyway. lns-customBucketContainer-remove is removing one out of multiple ranges, but that's not what should be tested here - this is about switching between custom ranges in general and automatic ranges controlled by granularity.

A proper unit test should click the "Remove custom ranges" button, then check whether the state update issued by this still includes the custom format.

Let me know when you have problems adding such a test. I can do it and add it to your PR

So, I have been trying to

it('should not reset formatters when switching between custom ranges and auto histogram', () => {
        const setStateSpy = jest.fn();
        (state.layers.first.columns.col1 as RangeIndexPatternColumn).params.ranges.push({
          from: null,
          to: null,
          label: '',
        });
        // set a default formatter for the sourceField used
        state.indexPatterns['1'].fieldFormatMap = {
          MyField: { id: 'custom', params: {} },
        };
        // now set a format on the range operation
        (state.layers.first.columns.col1 as RangeIndexPatternColumn).params.format = {
          id: 'custom',
          params: { decimals: 3 },
        };

        const instance = mount(
          <InlineOptions
            {...defaultOptions}
            state={state}
            setState={setStateSpy}
            columnId="col1"
            currentColumn={state.layers.first.columns.col1 as RangeIndexPatternColumn}
            layerId="first"
          />
        );
        //expect(instance.find(RangePopover)).toHaveLength(2);
        // This series of act closures are made to make it work properly the update flush
        act(() => {
          instance
            .find(".euiLink")
            .first()
            .prop('onClick')!({} as ReactMouseEvent);

          expect(setStateSpy).toHaveBeenCalledWith({
            ...state,
            layers: {
              first: {
                ...state.layers.first,
                columns: {
                  ...state.layers.first.columns,
                  col1: {
                    ...state.layers.first.columns.col1,
                    params: {
                      ...state.layers.first.columns.col1.params,
                      format: {
                        id: "custom",
                        params: { decimals: 3 }
                      }
                    },
                  },
                },
              },
            },
          });
        });

But it seems that Number of calls: 2 and I'm not sure how to handle that

[flash1293]

Can you push your latest state? Then I can take a look

[denismaxim0v]

Can you push your latest state? Then I can take a look

any comments on the test issue?

[flash1293]

@denismaxim0v Sorry for the delay, I'm blocked this morning but I will take a look this afternoon. Expect an update today

[flash1293]

I made several changes to your unit test:

      it('should not reset formatters when switching between custom ranges and auto histogram', () => {
        const setStateSpy = jest.fn();
        // now set a format on the range operation
        (state.layers.first.columns.col1 as RangeIndexPatternColumn).params.format = {
          id: 'custom',
          params: { decimals: 3 },
        };

        const instance = mount(
          <InlineOptions
            {...defaultOptions}
            state={state}
            setState={setStateSpy}
            columnId="col1"
            currentColumn={state.layers.first.columns.col1 as RangeIndexPatternColumn}
            layerId="first"
          />
        );

        // This series of act closures are made to make it work properly the update flush
        act(() => {
          instance.find(EuiLink).first().prop('onClick')!({} as ReactMouseEvent);
        });

        expect(setStateSpy.mock.calls[1][0].layers.first.columns.col1.params.format).toEqual({
          id: 'custom',
          params: { decimals: 3 },
        });
      });

• Remove unnecessary mock setup
• Use EuiLink instead of class based selector (those shouldn't be used because they are internal to the EUI lib and can change anytime)
• Assert using .mock.calls instead to be able to just check the relevant bit

Please update the PR with latest state of upstream master and fix the test this way, then this LGTM.

[denismaxim0v]

Thanks, will do

[flash1293]

Jenkins, test this

[flash1293]

@denismaxim0v Seems like there is an unused import that has to be removed.

[denismaxim0v]

done

[flash1293]

Jenkins, test this

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: ba77781

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
lens	1.0MB	1.0MB	+18.0B

History

• 💔 Build #87014 failed 1954db8
• 💔 Build #86857 failed 11ce62b
• 💛 Build #86332 was flaky 1ab13e4

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[flash1293]

LGTM, thanks!