fix addd to case relative #117487

shahzad31 · 2021-11-04T10:57:59Z

Summary

Fixes #117008

While adding to case, convert relative dates to absolute while generating params for add to case call to action.

shahzad31 · 2021-11-09T12:13:47Z

@elasticmachine merge upstream

elasticmachine · 2021-11-09T12:15:15Z

Pinging @elastic/uptime (Team:uptime)

lucasfcosta

Thanks for the PR @shahzad31!

The code LGTM, my only concern is with regards to the tests. I will not mark this as "request changes" because I think it would be useful to consider that I'm not fully up-to-speed on the team's position with regards to testing, so I will not block because of that.

However, I think it would still be nice to have tests for these changes.

With regards to the smoke test, here is what I've done:

Open the exploratory view
Add a series with a small relative range (using a few seconds ago as the final range)
Apply the changes to the graph and save it to a case
Refresh the case multiple times to ensure that the added graph still maintains the absolute time

You can see this on the GIF below.

Please let me know if any further testing is needed.

I've also confirmed that the "Add To Case" button disappears when no time-series exist.

lucasfcosta · 2021-11-09T15:46:49Z

...servability/public/components/shared/exploratory_view/components/action_menu/action_menu.tsx

+        {timeRange && (
+          <EuiFlexItem grow={false}>
+            <AddToCaseAction lensAttributes={lensAttributes} timeRange={timeRange} />
+          </EuiFlexItem>
+        )}


I'd suggest adding a test to confirm that this action shows/hides depending on the value of timeRange.

well, i think testing a simple condition doesn't make sense, if this condition was based on complex logic, it makes sense. A simple boolean condition, which hides/shows a dom element. I don't think it adds any value in terms of testing.

lucasfcosta · 2021-11-09T16:01:54Z

...lugins/observability/public/components/shared/exploratory_view/header/add_to_case_action.tsx

  const { createCaseUrl, goToCreateCase, onCaseClicked, isCasesOpen, setIsCasesOpen, isSaving } =
    useAddToCase({
      lensAttributes,
      getToastText,
-      timeRange,
+      timeRange: { from: absoluteFromDate.toISOString(), to: absoluteToDate.toISOString() },


Similarly, here I think we could we could add a test to ensure that these are passed as absolute dates to addToCase. In the test we could either check for the correct absolute dates on the URL (which would, in my opinion, be the most straightforward and decoupled option), or we could check the parameters passed to the hook itself (which I don't really like as it tightly couples the test with the implementation).

@lucasfcosta added a test case 274ebac (#117487)

shahzad31 · 2021-11-10T18:05:38Z

@elasticmachine merge upstream

lucasfcosta

Thanks for adding that test! 🙌

I added a tip there in case you want to follow it, but please feel free to go ahead and merge as I understand there may be time constraints on this. I don't want to block because of the mock.

lucasfcosta · 2021-11-10T18:04:51Z

...s/observability/public/components/shared/exploratory_view/header/add_to_case_action.test.tsx

@@ -21,6 +24,31 @@ describe('AddToCaseAction', function () {
    expect(await findByText('Add to case')).toBeInTheDocument();
  });

+  it('should parse relative data to the useAddToCase hook', async function () {
+    const useAddToCaseHook = jest.spyOn(useCaseHook, 'useAddToCase');
+    jest.spyOn(datePicker, 'parseRelativeDate').mockReturnValue(moment('2021-11-10T10:52:06.091Z'));


Thanks for adding this test! It LGTM.

As a nit in case you want a tip: I think here we're kinda losing some value by mocking early on parseRelativeDate. Instead we could mock the global timers setting a fixed date and then the parsed dates in timeRange would always be the same, but they would indeed be now and 5 minutes ago.

Currently, both dates are the same as we always return the same mock.

I won't block because of this though, just a tip in case you want the test to be closer to runtime behaviour.

In case you wanna read more about timers I'd recommend this link. Alternatively you can use jest-when for returning different values for the passed dates, but then I think we end-up in the same place as before as it's still a tightly coupled mock.

kibanamachine · 2021-11-10T19:23:26Z

💚 Build Succeeded

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
`observability`	378.8KB	378.9KB	+102.0B

History

💔 Build #5732 failed 274ebac
💚 Build #5351 succeeded 7b53e16
💚 Build #4310 succeeded a619074
💔 Build #4292 failed 98f9418

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

cc @shahzad31

kibanamachine · 2021-11-10T19:23:55Z

The following labels were identified as gaps in your version labels and will be added automatically:

v8.1.0

If any of these should not be on your pull request, please manually remove them.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-11-10T19:27:53Z

💚 Backport successful

Status	Branch	Result
✅	8.0
✅	7.16

The backport PRs will be merged automatically after passing CI.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Shahzad <shahzad.muhammad@elastic.co>

shahzad31 added 3 commits November 4, 2021 11:57

fix addd to case relative

98f9418

fix types

4753cf5

fix types

a619074

Merge branch 'main' into fix-add-to-case-relative-date

7b53e16

shahzad31 marked this pull request as ready for review November 9, 2021 12:13

shahzad31 requested a review from a team as a code owner November 9, 2021 12:13

shahzad31 self-assigned this Nov 9, 2021

shahzad31 added v7.16.0 v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Nov 9, 2021

shahzad31 added the release_note:skip Skip the PR/issue when compiling release notes label Nov 9, 2021

lucasfcosta reviewed Nov 9, 2021

View reviewed changes

added a test case

274ebac

shahzad31 added the auto-backport Deprecated - use backport:version if exact versions are needed label Nov 10, 2021

shahzad31 enabled auto-merge (squash) November 10, 2021 11:03

shahzad31 requested a review from lucasfcosta November 10, 2021 11:43

Merge branch 'main' into fix-add-to-case-relative-date

294def3

lucasfcosta approved these changes Nov 10, 2021

View reviewed changes

shahzad31 merged commit b389cfe into elastic:main Nov 10, 2021

kibanamachine added the v8.1.0 label Nov 10, 2021

kibanamachine mentioned this pull request Nov 10, 2021

[8.0] fix addd to case relative (#117487) #118226

Merged

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 10, 2021

fix addd to case relative (elastic#117487)

fff6663

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 10, 2021

fix addd to case relative (elastic#117487)

6a5f40e

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine mentioned this pull request Nov 10, 2021

[7.16] fix addd to case relative (#117487) #118227

Merged

kibanamachine added a commit that referenced this pull request Nov 10, 2021

fix addd to case relative (#117487) (#118226)

f062683

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Shahzad <shahzad.muhammad@elastic.co>

kibanamachine added a commit that referenced this pull request Nov 10, 2021

fix addd to case relative (#117487) (#118227)

c47017e

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Shahzad <shahzad.muhammad@elastic.co>

shahzad31 deleted the fix-add-to-case-relative-date branch November 11, 2021 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix addd to case relative #117487

fix addd to case relative #117487

shahzad31 commented Nov 4, 2021 •

edited

Loading

shahzad31 commented Nov 9, 2021

elasticmachine commented Nov 9, 2021

lucasfcosta left a comment •

edited

Loading

lucasfcosta Nov 9, 2021

shahzad31 Nov 10, 2021

lucasfcosta Nov 9, 2021

shahzad31 Nov 10, 2021

shahzad31 commented Nov 10, 2021

lucasfcosta left a comment

lucasfcosta Nov 10, 2021

kibanamachine commented Nov 10, 2021

kibanamachine commented Nov 10, 2021

kibanamachine commented Nov 10, 2021

fix addd to case relative #117487

fix addd to case relative #117487

Conversation

shahzad31 commented Nov 4, 2021 • edited Loading

Summary

shahzad31 commented Nov 9, 2021

elasticmachine commented Nov 9, 2021

lucasfcosta left a comment • edited Loading

Choose a reason for hiding this comment

lucasfcosta Nov 9, 2021

Choose a reason for hiding this comment

shahzad31 Nov 10, 2021

Choose a reason for hiding this comment

lucasfcosta Nov 9, 2021

Choose a reason for hiding this comment

shahzad31 Nov 10, 2021

Choose a reason for hiding this comment

shahzad31 commented Nov 10, 2021

lucasfcosta left a comment

Choose a reason for hiding this comment

lucasfcosta Nov 10, 2021

Choose a reason for hiding this comment

kibanamachine commented Nov 10, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Nov 10, 2021

kibanamachine commented Nov 10, 2021

💚 Backport successful

shahzad31 commented Nov 4, 2021 •

edited

Loading

lucasfcosta left a comment •

edited

Loading