Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ability to share a preconfigured diff scene #1158

Open
wants to merge 39 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
7fa7881
Clean branch off main with changes; WIP validating param
patnir41 Aug 9, 2022
1fe3e4a
Added utils in path for verifying filter
patnir41 Aug 9, 2022
e3b7e3d
Merge with main, sdk sharing with filter sharing
patnir41 Aug 9, 2022
853531a
Refactoring and unit testing implemented
patnir41 Aug 9, 2022
2d28c7f
Fixed bug on state of TypeTagScene
patnir41 Aug 9, 2022
7c04e61
WIP creating custom reconciliation and navigation hooks
patnir41 Aug 10, 2022
863bf47
Fix to not using isSynced
patnir41 Aug 10, 2022
6e3f9ea
Refactor using isSync variable determining render
patnir41 Aug 11, 2022
6339b11
Custom sync hooks, new navigate functions in hook for params
patnir41 Aug 11, 2022
0b09d4f
Code cleanup
patnir41 Aug 11, 2022
2d54f37
Code clean up
patnir41 Aug 11, 2022
caa189c
Fixed invalid verb param on tab switching, WIP hook testing
patnir41 Aug 12, 2022
ae47fa0
WIP globalStoreSync tests
patnir41 Aug 15, 2022
70ae6e2
globalSync and tagSync testing included, refactored files
patnir41 Aug 15, 2022
376e21b
Clean up per Jax comments
patnir41 Aug 15, 2022
e1e4c0a
Fix navigation hook function naming
patnir41 Aug 15, 2022
d8bfd8a
Merge branch 'patnir41/filter-sharing-custom-hooks' into patnir41/pre…
patnir41 Aug 15, 2022
93b959a
Diff scene options in URL, testing WIP
patnir41 Aug 16, 2022
6d46d80
Added testing & functional, WIP DiffScene.spec
patnir41 Aug 16, 2022
4048143
WIP DiffScene testing
patnir41 Aug 17, 2022
ffda1ca
Merging changes from main with tag filter PR
patnir41 Aug 17, 2022
071e0b5
WIP: finalizing tests
patnir41 Aug 18, 2022
358c56c
Merge branch 'main' into patnir41/preconfig-difs-8-15
patnir41 Aug 18, 2022
91917f5
Changes to util files to fix imports
patnir41 Aug 18, 2022
a6a47ca
Merge branch 'main' into patnir41/preconfig-difs-8-15
patnir41 Aug 19, 2022
d92df04
WIP DiffScene.spec testing still, functionality bug fix
patnir41 Aug 19, 2022
377dac8
DiffScene testing bug, no default state set immediately
patnir41 Aug 24, 2022
ea02ec2
WIP unable to init store with default settings in DiffScene spec
patnir41 Aug 30, 2022
73ca49c
Merge branch 'main' into patnir41/preconfig-difs-8-15
patnir41 Aug 31, 2022
c0297ed
Setting up diffscene spec for other tests
patnir41 Aug 31, 2022
ef95f97
Removing testReduxUtils, making spec file without this test
patnir41 Sep 1, 2022
90840d4
Moving forward with finalizing document ignoring spec bug
patnir41 Sep 2, 2022
f883a97
Merge branch 'main' into patnir41/preconfig-difs-8-15
patnir41 Sep 2, 2022
268ecd3
Refactoring file and tests
patnir41 Sep 2, 2022
7e8e558
Wrap up DiffScene spec, WIP e2e
patnir41 Sep 6, 2022
a8965ea
Added 1 e2e test for DiffScene
patnir41 Sep 6, 2022
c34fdee
Refactoring diffUtils function
patnir41 Sep 7, 2022
09a0666
Merge branch 'main' into patnir41/preconfig-difs-8-15
jkaster Sep 13, 2022
c54f9b3
Merge branch 'main' into patnir41/preconfig-difs-8-15
jkaster Sep 20, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 73 additions & 2 deletions packages/api-explorer/e2e/diffScene.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const resultCardsSelector =
'section#top div[class*=SpaceVertical] div[class*=Card]'
const baseInputSelector = 'input#listbox-input-base'
const compInputSelector = 'input#listbox-input-compare'
const compOptionsInputSelector = 'input#listbox-input-options'
const globalOptionsSelector = '#modal-root [role=option] span'
const switchButtonSelector = '.switch-button'

Expand Down Expand Up @@ -225,7 +226,9 @@ describe('Diff Scene', () => {
// Check the URL
// Would like to do this earlier, but not sure what to wait on
const compUrl = page.url()
expect(compUrl).toEqual(`${BASE_URL}/3.1/diff/4.0?sdk=py`)
expect(compUrl).toEqual(
`${BASE_URL}/3.1/diff/4.0?sdk=py&opts=missing%2Cparams%2Ctype%2Cbody%2Cresponse`
)

// Check the results
const diffResultCards = await page.$$(resultCardsSelector)
Expand Down Expand Up @@ -253,7 +256,9 @@ describe('Diff Scene', () => {
await page.waitForTimeout(150)

const switchUrl = page.url()
expect(switchUrl).toEqual(`${BASE_URL}/4.0/diff/3.1?sdk=py`)
expect(switchUrl).toEqual(
`${BASE_URL}/4.0/diff/3.1?sdk=py&opts=missing%2Cparams%2Ctype%2Cbody%2Cresponse`
)

// Check the results again, even though they should be the same
const diff40to31Page1Methods = await Promise.all(
Expand All @@ -265,4 +270,70 @@ describe('Diff Scene', () => {
expect(diff40to31Page1Methods).toHaveLength(15)
expect(diff40to31Page1Methods).toContain('delete_board_item')
})

it('updates when a comparison option is toggled', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove this, but I thought it would be nice to have an integration test ensuring the DiffScene operates properly other than the unit tests which I couldn't make comprehensive due to the spec file bug.

await goToPage(`${BASE_URL}/3.1/diff/4.0`)

// expect default diff options in url
expect(page.url()).toEqual(
`${BASE_URL}/3.1/diff/4.0?sdk=py&opts=missing%2Cparams%2Ctype%2Cbody%2Cresponse`
)

// "Base" input element
const baseInputElement = await page.$(baseInputSelector)
expect(baseInputElement).not.toBeNull()

// "Comparison" input element
const compInputElement = await page.$(compInputSelector)
expect(compInputElement).not.toBeNull()

// "Comparison Options" input element
const compOptionsInputElement = await page.$(compOptionsInputSelector)
expect(compOptionsInputElement).not.toBeNull()

// Check that initial results exist with default comparison options
const initDiffResults = await page.$$(resultCardsSelector)
expect(initDiffResults).not.toHaveLength(0)
const initDiff31to40Page1Methods = await Promise.all(
initDiffResults.map((resultCard) =>
page.evaluate((el) => el.innerText.match(/^[a-z_]*/)[0], resultCard)
)
)
expect(initDiff31to40Page1Methods).toHaveLength(15)
expect(initDiff31to40Page1Methods).toContain('delete_alert')

// Click comparison input
await compOptionsInputElement!.click()
const compOptionsOnClick = await page.$$(globalOptionsSelector)
expect(compOptionsOnClick).toHaveLength(6)

// Find an option containing the text Missing
const missingOptionIndex = await page.$$eval(globalOptionsSelector, (els) =>
els.findIndex((el) => el?.textContent?.match('Missing'))
)
const missingOption = compOptionsOnClick[missingOptionIndex]
expect(missingOption).not.toBeUndefined()

// Click that option
await missingOption.click()
await page.waitForSelector(resultCardsSelector, { timeout: 5000 })

// Check the URL
// Would like to do this earlier, but not sure what to wait on
const compUrl = page.url()
expect(compUrl).toEqual(
`${BASE_URL}/3.1/diff/4.0?sdk=py&opts=params%2Ctype%2Cbody%2Cresponse`
)

// Check that there are new results
const newDiffResults = await page.$$(resultCardsSelector)
expect(newDiffResults).not.toHaveLength(0)
const newDiff31to40Page1Methods = await Promise.all(
newDiffResults.map((resultCard) =>
page.evaluate((el) => el.innerText.match(/^[a-z_]*/)[0], resultCard)
)
)
expect(newDiff31to40Page1Methods).toHaveLength(15)
expect(newDiff31to40Page1Methods).not.toContain('delete_alert')
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,10 @@ describe('SideNavMethods', () => {
const firstMethod = Object.values(methods)[0].schema.summary
expect(screen.queryByText(firstMethod)).not.toBeInTheDocument()
userEvent.click(screen.getByText(tag))
expect(mockHistoryPush).toHaveBeenCalledWith(`/${specKey}/methods/${tag}`)
expect(mockHistoryPush).toHaveBeenCalledWith({
pathname: `/${specKey}/methods/${tag}`,
search: '',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change won't be needed if navigateWithGlobalParams is used.

})
expect(screen.getByRole('link', { name: firstMethod })).toBeInTheDocument()
expect(screen.getAllByRole('link')).toHaveLength(
Object.values(methods).length
Expand All @@ -98,7 +101,10 @@ describe('SideNavMethods', () => {
Object.values(methods).length
)
userEvent.click(screen.getByText(tag))
expect(mockHistoryPush).toHaveBeenCalledWith(`/${specKey}/methods`)
expect(mockHistoryPush).toHaveBeenCalledWith({
pathname: `/${specKey}/methods`,
search: '',
})
Comment on lines +104 to +107
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

expect(screen.queryByText(firstMethod)).not.toBeInTheDocument()
expect(screen.queryByRole('link')).not.toBeInTheDocument()
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ interface MethodsProps {

export const SideNavMethods = styled(
({ className, methods, tag, specKey, defaultOpen = false }: MethodsProps) => {
const { buildPathWithGlobalParams, navigateWithGlobalParams } =
useNavigation()
const { buildPathWithGlobalParams, navigate } = useNavigation()
const searchPattern = useSelector(selectSearchPattern)
const match = useRouteMatch<{ methodTag: string }>(
`/:specKey/methods/:methodTag/:methodName?`
Expand All @@ -55,9 +54,9 @@ export const SideNavMethods = styled(
const _isOpen = !isOpen
setIsOpen(_isOpen)
if (_isOpen) {
navigateWithGlobalParams(`/${specKey}/methods/${tag}`)
navigate(`/${specKey}/methods/${tag}`, { opts: null })
Copy link
Contributor Author

@patnir41 patnir41 Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scenario to consider here (and with the same change in SideNavTypes): we are in a diff scene with selected options, and opts param in the url. When we open the sideNav and open a method/tag scene, this opts parameter should no longer persist. The reason we don't use navigateWithGlobalParams to remove this parameter is because if we started at a tag scene with a filter selected and want to select another tag, this filter should still persist when we open the new tag scene.

So, technically, the purpose of this line in plain english is more like "navigate to the tag scene and ONLY keep the tag filter variable if we have one in the URL"

This makes me reconsider what we initially had proposed where we have a specific navigateTo methods. I think in the long term there will be 2 cases:

  1. We navigate to a scene, remove all scene-specific params first, then push global params (currently the case with navigateWithGlobalParams, etc)
  2. We navigate to a scene, keeping ONLY specific parameters, and removing anything else in the URL.

To better account for case 2, I think having custom navigate methods makes the most sense. Here we'd explicitly have a navigateToTagScene instead of having to explicitly nullify any extra parameters that could persist when navigating to a tag scene from the SideNav.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not navigateWithGlobalParams?

} else {
navigateWithGlobalParams(`/${specKey}/methods`)
navigate(`/${specKey}/methods`, { opts: null })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ export const SideNavTypes = styled(
const _isOpen = !isOpen
setIsOpen(_isOpen)
if (_isOpen) {
navigate(`/${specKey}/types/${tag}`)
navigate(`/${specKey}/types/${tag}`, { opts: null })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

} else {
navigate(`/${specKey}/types`)
navigate(`/${specKey}/types`, { opts: null })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

}
}

Expand Down
199 changes: 199 additions & 0 deletions packages/api-explorer/src/scenes/DiffScene/DiffScene.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
/*

MIT License

Copyright (c) 2021 Looker Data Sciences, Inc.

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.

*/
import React from 'react'
import { screen, waitFor, within } from '@testing-library/react'

import type { SpecItem } from '@looker/sdk-codegen'
import userEvent from '@testing-library/user-event'
import { getLoadedSpecs } from '../../test-data'
import {
createTestStore,
renderWithRouterAndReduxProvider,
} from '../../test-utils'
import { getApixAdaptor } from '../../utils'
import { DiffScene } from './DiffScene'

const mockHistoryPush = jest.fn()
jest.mock('react-router-dom', () => {
const location = {
pathname: '/3.1/diff',
search: '',
hash: '',
state: {},
key: '',
}
const ReactRouter = jest.requireActual('react-router-dom')
return {
__esModule: true,
...ReactRouter,
useHistory: () => ({
push: mockHistoryPush,
location,
}),
useLocation: jest.fn().mockReturnValue(location),
}
})

const specs = getLoadedSpecs()
class MockApixAdaptor {
async fetchSpec(spec: SpecItem) {
return new Promise(() => specs[spec.key])
}
}

const mockApixAdaptor = new MockApixAdaptor()
jest.mock('../../utils/apixAdaptor', () => {
const apixAdaptor = jest.requireActual('../../utils/apixAdaptor')
return {
__esModule: true,
...apixAdaptor,
getApixAdaptor: jest.fn(),
}
})

describe('DiffScene', () => {
beforeEach(() => {
jest.clearAllMocks()
})
;(getApixAdaptor as jest.Mock).mockReturnValue(mockApixAdaptor)
Element.prototype.scrollTo = jest.fn()
Element.prototype.scrollIntoView = jest.fn()

const toggleNavigation = () => false

test('rendering with no url opts param results in default comparison options toggled', () => {
const store = createTestStore({
specs: { specs, currentSpecKey: '3.1' },
})
renderWithRouterAndReduxProvider(
<DiffScene toggleNavigation={toggleNavigation} />,
['/3.1/diff'],
store
)

expect(
screen.getByRole('option', {
name: 'Missing Delete',
})
).toBeInTheDocument()
expect(
screen.getByRole('option', {
name: 'Parameters Delete',
})
).toBeInTheDocument()
expect(
screen.getByRole('option', {
name: 'Type Delete',
})
).toBeInTheDocument()
expect(
screen.getByRole('option', {
name: 'Body Delete',
})
).toBeInTheDocument()
expect(
screen.queryByRole('option', {
name: 'Status Delete',
})
).not.toBeInTheDocument()
expect(
screen.getByRole('option', {
name: 'Response Delete',
})
).toBeInTheDocument()
})

test('selecting a comparison option pushes it to url opts param', async () => {
const store = createTestStore({
specs: { specs, currentSpecKey: '3.1' },
settings: { diffOptions: [] },
})
renderWithRouterAndReduxProvider(
<DiffScene toggleNavigation={toggleNavigation} />,
['/3.1/diff'],
store
)
userEvent.click(screen.getByPlaceholderText('Comparison options'))
await waitFor(() => {
expect(screen.getByRole('dialog')).toBeInTheDocument()
})
userEvent.click(
screen.getByRole('option', {
name: 'Missing',
})
)
await waitFor(() => {
expect(mockHistoryPush).toHaveBeenLastCalledWith({
pathname: '/3.1/diff',
search: 'opts=missing',
})
})
})

test('unselecting comparison option will remove it from url opts param', async () => {
const store = createTestStore({
specs: { specs, currentSpecKey: '3.1' },
settings: { diffOptions: ['missing', 'params'] },
})
renderWithRouterAndReduxProvider(
<DiffScene toggleNavigation={toggleNavigation} />,
['/3.1/diff'],
store
)
const missingOption = screen.getByRole('option', {
name: 'Missing Delete',
})
userEvent.click(within(missingOption).getByRole('button'))
await waitFor(() => {
expect(mockHistoryPush).toHaveBeenLastCalledWith({
pathname: '/3.1/diff',
search: 'opts=params',
})
})
})

test('selecting clear option will remove all params from url opts param', async () => {
const store = createTestStore({
specs: { specs, currentSpecKey: '3.1' },
})
renderWithRouterAndReduxProvider(
<DiffScene toggleNavigation={toggleNavigation} />,
['/3.1/diff'],
store
)
userEvent.click(
screen.getByRole('button', {
name: 'Clear Field',
})
)
await waitFor(() => {
expect(mockHistoryPush).toHaveBeenLastCalledWith({
pathname: '/3.1/diff',
search: '',
})
})
})
})
Loading