Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
1752e01
Update Linode interface to include capabilities property; add BLOCK_S…
DevDW Aug 26, 2024
6f04ede
Conditional display of client library copy in Create & Attach view, d…
DevDW Aug 27, 2024
22adfad
Clear client library copy when user toggles view inside Volume Add dr…
DevDW Aug 27, 2024
c143d1f
Reintroduce useVolumeQuery; logic in LinodeVolumeAttachForm to contro…
DevDW Aug 27, 2024
3b87504
Linter, util, factory updates; test coverage; VolumeSelect.tsx refact…
DevDW Aug 29, 2024
c828e21
Disable 'Attach Volume' button when linode requires client library up…
DevDW Aug 29, 2024
32b47b4
Passing create-volume.spec.ts
DevDW Aug 30, 2024
2c00ca2
Merge in latest develop and resolve conflict, fix LinodeVolumeAddDraw…
DevDW Aug 30, 2024
174d7a7
Added changeset: Change 'bs_encryption_supported' property on Linode …
DevDW Aug 30, 2024
77f9232
Added changeset: Unit test for LinodeVolumeAddDrawer and E2E test for…
DevDW Aug 30, 2024
1fa0029
Added changeset: Support Volume Encryption and associated notices in …
DevDW Aug 30, 2024
3d71087
Update E2E test to reflect 'Create Volume' --> 'Add Volume' change
DevDW Aug 30, 2024
b8dd4c3
Add invalidations for individual volume query & refactor useDetachVol…
DevDW Sep 5, 2024
4d02471
Revert useDetachVolumeMutation changes; use setQueryData in relevant …
DevDW Sep 6, 2024
8012b0c
Remove stray leftover cy.log()
DevDW Sep 6, 2024
3126161
Specific volume invalidation in volumeEventsHandler
DevDW Sep 6, 2024
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
8 changes: 4 additions & 4 deletions docs/tooling/react-query.md
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,14 @@ export const useCreateLinodeMutation = () => {

## Frequently Asked Questions

### Are we storing dupdate data in the cache? Why?
### Are we storing duplicated data in the cache? Why?

Yes, there is potential for the same data to exist many times in the cache.

For example, we have a query `useVolumesQuery` with the query key `["volumes", "paginated", { page: 1 }]` that contains the first 100 volumes on your account.
One of those same volumes could also be stored in the cache by using `useVolumeQuery` with query key `["linodes", "linode", 5]`.
This creates a senerio where the same volume is cached by React Query under multiple query keys.
One of those same volumes could also be stored in the cache by using `useVolumeQuery` with query key `["volumes", "volume", 5]`.
This creates a scenario where the same volume is cached by React Query under multiple query keys.

This is a legitimate disadvantage of React Query's caching strategy. **We must be aware of this when we perform cache updates (using invalidations or manually updating the cache) so that the entity is update everywhere in the cache.**
This is a legitimate disadvantage of React Query's caching strategy. **We must be aware of this when we perform cache updates (using invalidations or manually updating the cache) so that the entity is updated everywhere in the cache.**

Some data fetching tools like Apollo Client are able to intelligently detect duplicate entities and merge them. React Query does not do this. See [this tweet](https://twitter.com/tannerlinsley/status/1557395389531074560) from the creator of React Query.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/api-v4": Upcoming Features
---

Change 'bs_encryption_supported' property on Linode object to 'capabilities' ([#10837](https://github.com/linode/manager/pull/10837))
2 changes: 1 addition & 1 deletion packages/api-v4/src/linodes/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export interface Linode {
id: number;
alerts: LinodeAlerts;
backups: LinodeBackups;
bs_encryption_supported?: boolean; // @TODO BSE: Remove optionality once BSE is fully rolled out
capabilities?: string[]; // @TODO BSE: Remove optionality once BSE is fully rolled out
Copy link
Contributor Author

Choose a reason for hiding this comment

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

May not be reflected in the API spec (yet), but this is how it was actually implemented. Reach out for Slack convo if needed.

created: string;
disk_encryption?: EncryptionStatus; // @TODO LDE: Remove optionality once LDE is fully rolled out
region: string;
Expand Down
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-10837-tests-1725037321098.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Tests
---

Unit test for LinodeVolumeAddDrawer and E2E test for client library update notices in Create/Attach Volume drawer ([#10837](https://github.com/linode/manager/pull/10837))
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Upcoming Features
---

Support Volume Encryption and associated notices in Create/Attach Volume drawer ([#10837](https://github.com/linode/manager/pull/10837))
93 changes: 91 additions & 2 deletions packages/manager/cypress/e2e/core/volumes/create-volume.spec.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
import type { Linode } from '@linode/api-v4';
import type { Linode, Region } from '@linode/api-v4';
import { createTestLinode } from 'support/util/linodes';
import { createLinodeRequestFactory } from 'src/factories/linodes';
import { authenticate } from 'support/api/authentication';
import { cleanUp } from 'support/util/cleanup';
import { interceptCreateVolume } from 'support/intercepts/volumes';
import {
interceptCreateVolume,
mockGetVolume,
mockGetVolumes,
} from 'support/intercepts/volumes';
import { randomNumber, randomString, randomLabel } from 'support/util/random';
import { chooseRegion } from 'support/util/regions';
import { ui } from 'support/ui';
import { mockAppendFeatureFlags } from 'support/intercepts/feature-flags';
import { accountFactory, regionFactory, volumeFactory } from 'src/factories';
import { mockGetAccount } from 'support/intercepts/account';
import { mockGetRegions } from 'support/intercepts/regions';

// Local storage override to force volume table to list up to 100 items.
// This is a workaround while we wait to get stuck volumes removed.
Expand All @@ -15,6 +23,18 @@ const pageSizeOverride = {
PAGE_SIZE: 100,
};

const mockRegions: Region[] = [
regionFactory.build({
capabilities: ['Linodes', 'Block Storage', 'Block Storage Encryption'],
id: 'us-east',
label: 'Newark, NJ',
site_type: 'core',
}),
];

const CLIENT_LIBRARY_UPDATE_COPY =
'This Linode requires a client library update and will need to be rebooted prior to attaching an encrypted volume.';

authenticate();
describe('volume create flow', () => {
before(() => {
Expand Down Expand Up @@ -141,6 +161,75 @@ describe('volume create flow', () => {
);
});

/*
* - Checks for Block Storage Encryption notices in the Create/Attach Volume drawer from the
'Storage' details page of an existing Linode.
*/
it('displays a warning notice re: rebooting for client library updates under the appropriate conditions', () => {
// Conditions: Block Storage encryption feature flag is on; user has Block Storage Encryption capability; Linode does not support Block Storage Encryption and the user is trying to attach an encrypted volume

// Mock feature flag -- @TODO BSE: Remove feature flag once BSE is fully rolled out
mockAppendFeatureFlags({
blockStorageEncryption: true,
}).as('getFeatureFlags');

// Mock account response
const mockAccount = accountFactory.build({
capabilities: ['Linodes', 'Block Storage Encryption'],
});

mockGetAccount(mockAccount).as('getAccount');
mockGetRegions(mockRegions).as('getRegions');

const volume = volumeFactory.build({
region: mockRegions[0].id,
encryption: 'enabled',
});

const linodeRequest = createLinodeRequestFactory.build({
label: randomLabel(),
root_pass: randomString(16),
region: mockRegions[0].id,
booted: false,
});

cy.defer(() => createTestLinode(linodeRequest), 'creating Linode').then(
(linode: Linode) => {
mockGetVolumes([volume]).as('getVolumes');
mockGetVolume(volume);

cy.visitWithLogin(`/linodes/${linode.id}/storage`);
cy.wait(['@getFeatureFlags', '@getAccount']);

// Click "Add Volume" button
cy.findByText('Add Volume').click();

cy.get('[data-qa-drawer="true"]').within(() => {
cy.get('[data-qa-checked]').should('be.visible').click();
});

cy.findByText(CLIENT_LIBRARY_UPDATE_COPY).should('be.visible');

// Ensure notice is cleared when switching views in drawer
cy.get('[data-qa-radio="Attach Existing Volume"]').click();
cy.wait(['@getVolumes']);
cy.findByText(CLIENT_LIBRARY_UPDATE_COPY).should('not.exist');

// Ensure notice is displayed in "Attach Existing Volume" view when an encrypted volume is selected
cy.findByPlaceholderText('Select a Volume')
.should('be.visible')
.click()
.type(`${volume.label}{downarrow}{enter}`);
ui.autocompletePopper
.findByTitle(volume.label)
.should('be.visible')
.click();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jdamore-linode It seems that everything is good up until this point, and then the assertion on L228 causes the failure. Did some console.log'ing with the Cypress test running and clientLibraryCopyVisible in LinodeVolumeAddDrawer.tsx was never getting set to true for the "Attach Existing Volume" flow. I'm not sure why that is; it definitely works using the actual UI πŸ€”

Copy link
Contributor

Choose a reason for hiding this comment

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

I see! When Cypress selects the Volume, Cloud fires off a GET request to /volumes/:id for the selected Volume. Since we're using a mock Volume, the response is 404ing so the attach form doesn't know that the Volume is encrypted.

Mocking that request fixes the issue, but surprisingly we don't have a mock util for that endpoint. You can add one to cypress/support/intercepts/volumes.ts:

/**
 * Intercepts GET request to fetch a Volume and mocks response.
 *
 * @param volume - Volume with which to mock response.
 *
 * @returns Cypress chainable.
 */
export const mockGetVolume = (volume: Volume): Cypress.Chainable<null> => {
  return cy.intercept('GET', apiMatcher(`volumes/${volume.id}`), makeResponse(volume));
};

Then you can set it up right around where you mock the other Volumes request at line ~198:

mockGetVolumes([volume]).as('getVolumes');
mockGetVolume(volume);

That caused the library update message to display for me and allowed the test to pass!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, thanks for helping get to the bottom of that!


cy.findByText(CLIENT_LIBRARY_UPDATE_COPY).should('be.visible');
}
);
});

/*
* - Creates a volume from the 'Storage' details page of an existing Linode.
* - Confirms that volume is listed correctly on Linode 'Storage' details page.
Expand Down
15 changes: 15 additions & 0 deletions packages/manager/cypress/support/intercepts/volumes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,21 @@ export const mockGetVolumes = (volumes: Volume[]): Cypress.Chainable<null> => {
return cy.intercept('GET', apiMatcher('volumes*'), paginateResponse(volumes));
};

/**
* Intercepts GET request to fetch a Volume and mocks response.
*
* @param volume - Volume with which to mock response.
*
* @returns Cypress chainable.
*/
export const mockGetVolume = (volume: Volume): Cypress.Chainable<null> => {
return cy.intercept(
'GET',
apiMatcher(`volumes/${volume.id}`),
makeResponse(volume)
);
};

/**
* Intercepts POST request to create a Volume.
*
Expand Down
46 changes: 25 additions & 21 deletions packages/manager/cypress/support/util/linodes.ts
Original file line number Diff line number Diff line change
@@ -1,29 +1,30 @@
import { createLinode, getLinodeConfigs } from '@linode/api-v4';
import { createLinodeRequestFactory } from '@src/factories';
import { findOrCreateDependencyFirewall } from 'support/api/firewalls';
import { pageSize } from 'support/constants/api';
import { SimpleBackoffMethod } from 'support/util/backoff';
import { pollLinodeDiskStatuses, pollLinodeStatus } from 'support/util/polling';
import { randomLabel, randomString } from 'support/util/random';
import { chooseRegion } from 'support/util/regions';

import { depaginate } from './paginate';
import { pageSize } from 'support/constants/api';

import type {
Config,
CreateLinodeRequest,
InterfacePayload,
Linode,
} from '@linode/api-v4';
import { findOrCreateDependencyFirewall } from 'support/api/firewalls';

/**
* Linode create interface to configure a Linode with no public internet access.
*/
export const linodeVlanNoInternetConfig: InterfacePayload[] = [
{
purpose: 'vlan',
primary: false,
label: randomLabel(),
ipam_address: null,
label: randomLabel(),
primary: false,
purpose: 'vlan',
},
];

Expand All @@ -40,30 +41,30 @@ export const linodeVlanNoInternetConfig: InterfacePayload[] = [
*/
export type CreateTestLinodeSecurityMethod =
| 'firewall'
| 'vlan_no_internet'
| 'powered_off';
| 'powered_off'
| 'vlan_no_internet';

/**
* Options to control the behavior of test Linode creation.
*/
export interface CreateTestLinodeOptions {
/** Whether to wait for created Linode disks to be available before resolving. */
waitForDisks: boolean;
/** Method to use to secure the test Linode. */
securityMethod: CreateTestLinodeSecurityMethod;

/** Whether to wait for created Linode to boot before resolving. */
waitForBoot: boolean;

/** Method to use to secure the test Linode. */
securityMethod: CreateTestLinodeSecurityMethod;
/** Whether to wait for created Linode disks to be available before resolving. */
waitForDisks: boolean;
}

/**
* Default test Linode creation options.
*/
export const defaultCreateTestLinodeOptions = {
waitForDisks: false,
waitForBoot: false,
securityMethod: 'firewall',
waitForBoot: false,
waitForDisks: false,
};

/**
Expand Down Expand Up @@ -106,20 +107,20 @@ export const createTestLinode = async (

const resolvedCreatePayload = {
...createLinodeRequestFactory.build({
label: randomLabel(),
booted: false,
image: 'linode/debian11',
label: randomLabel(),
region: chooseRegion().id,
booted: false,
}),
...(createRequestPayload || {}),
...securityMethodPayload,

// Override given root password; mitigate against using default factory password, inadvertent logging, etc.
root_pass: randomString(64, {
lowercase: true,
numbers: true,
spaces: true,
symbols: true,
numbers: true,
lowercase: true,
uppercase: true,
}),
};
Expand Down Expand Up @@ -169,21 +170,24 @@ export const createTestLinode = async (
}

Cypress.log({
name: 'createTestLinode',
message: `Create Linode '${linode.label}' (ID ${linode.id})`,
consoleProps: () => {
return {
linode,
options: resolvedOptions,
payload: {
...resolvedCreatePayload,
root_pass: '(redacted)',
},
linode,
};
},
message: `Create Linode '${linode.label}' (ID ${linode.id})`,
name: 'createTestLinode',
});

return linode;
return {
...linode,
capabilities: [],
};
};

/**
Expand Down
19 changes: 10 additions & 9 deletions packages/manager/src/components/Autocomplete/Autocomplete.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import MuiAutocomplete from '@mui/material/Autocomplete';
import React from 'react';

import { Box } from 'src/components/Box';
import { TextField, TextFieldProps } from 'src/components/TextField';
import { TextField } from 'src/components/TextField';

import { CircleProgress } from '../CircleProgress';
import { InputAdornment } from '../InputAdornment';
Expand All @@ -15,6 +15,7 @@ import {
} from './Autocomplete.styles';

import type { AutocompleteProps } from '@mui/material/Autocomplete';
import type { TextFieldProps } from 'src/components/TextField';

export interface EnhancedAutocompleteProps<
T extends { label: string },
Expand All @@ -25,6 +26,8 @@ export interface EnhancedAutocompleteProps<
AutocompleteProps<T, Multiple, DisableClearable, FreeSolo>,
'renderInput'
> {
/** Removes "select all" option for multiselect */
disableSelectAll?: boolean;
/** Provides a hint with error styling to assist users. */
errorText?: string;
/** Provides a hint with normal styling to assist users. */
Expand All @@ -38,8 +41,6 @@ export interface EnhancedAutocompleteProps<
placeholder?: string;
/** Label for the "select all" option. */
selectAllLabel?: string;
/** Removes "select all" option for mutliselect */
disableSelectAll?: boolean;
textFieldProps?: Partial<TextFieldProps>;
}

Expand Down Expand Up @@ -71,6 +72,7 @@ export const Autocomplete = <
clearOnBlur,
defaultValue,
disablePortal = true,
disableSelectAll = false,
errorText = '',
helperText,
label,
Expand All @@ -88,7 +90,6 @@ export const Autocomplete = <
selectAllLabel = '',
textFieldProps,
value,
disableSelectAll = false,
...rest
} = props;

Expand All @@ -103,6 +104,11 @@ export const Autocomplete = <

return (
<MuiAutocomplete
options={
multiple && !disableSelectAll && options.length > 0
? optionsWithSelectAll
: options
}
renderInput={(params) => (
<TextField
errorText={errorText}
Expand Down Expand Up @@ -169,11 +175,6 @@ export const Autocomplete = <
multiple={multiple}
noOptionsText={noOptionsText || <i>You have no options to choose from</i>}
onBlur={onBlur}
options={
multiple && !disableSelectAll && options.length > 0
? optionsWithSelectAll
: options
}
popupIcon={<KeyboardArrowDownIcon />}
value={value}
{...rest}
Expand Down
Loading