feat(app-deploy): Frontend support for undeploying an application in a selected environment #14494
Conversation
|
Warning Rate limit exceeded@framitdavid has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive implementation of an application undeployment feature across multiple frontend components. The changes include creating new components like Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
area/app-deploy
framitdavid
changed the title
…altinn-studio into feature/undeployFrontEnd
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14494 +/- ##
=======================================
Coverage 95.69% 95.69%
=======================================
Files 1893 1902 +9
Lines 24634 24704 +70
Branches 2823 2825 +2
=======================================
+ Hits 23573 23641 +68
- Misses 800 802 +2
Partials 261 261 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (13)
frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/utils/isItemWithLink.test.ts (1)
4-14: Enhance test coverage with edge cases.While the basic cases are covered, consider adding tests for:
- Objects with undefined/null link values
- Malformed input objects
- Objects with unexpected link types
it('should return false for object with undefined link', () => { const item = { textKey: 'key', link: undefined }; expect(isItemWithLink(item)).toBe(false); }); it('should return false for malformed input', () => { const item = { textKey: 'key', link: null }; expect(isItemWithLink(item)).toBe(false); });frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/Section/Section.tsx (1)
9-18: Consider adding aria-label for better accessibility.While the component structure is good, consider enhancing accessibility by adding an aria-label to the section div.
- <div className={classes.section}> + <div className={classes.section} aria-label={title}>frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/Section/Section.test.tsx (1)
5-17: Enhance test coverage with additional test cases.While the basic rendering test is good, consider adding tests for:
- Empty title handling
- Multiple children rendering
- Long content truncation (if implemented)
Example additional test:
test('handles empty title gracefully', () => { const children = <li>Item 1</li>; render(<Section title="">{children}</Section>); expect(screen.getByRole('heading')).toBeInTheDocument(); });frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/consequences.data.ts (1)
38-38: Consider making the documentation URL language-agnostic.The hardcoded Norwegian language path (
/nb/) in the documentation URL might cause issues for users with different language preferences.- link: 'https://docs.altinn.studio//nb/altinn-studio/reference/logic/instantiation/', + link: 'https://docs.altinn.studio/altinn-studio/reference/logic/instantiation/',frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/DialogContent.tsx (1)
14-32: Consider adding error boundaries for robust error handling.While the implementation is clean, consider wrapping the mapping operations in an error boundary to gracefully handle potential runtime errors (e.g., malformed data).
+import { ErrorBoundary } from '@studio/components'; + export const DialogContent = ({ environment }: DialogContentProps) => { const { t } = useTranslation(); return ( <> + <ErrorBoundary> {consequencesDialogData.map((section) => ( <Section key={section.titleKey} title={t(section.titleKey)}> {section.items.map((item) => isItemWithLink(item) ? ( <ListItemWithLink key={item.textKey} textKey={item.textKey} link={item.link} /> ) : ( <StudioList.Item key={item.textKey}>{t(item.textKey)}</StudioList.Item> ), )} </Section> ))} + </ErrorBoundary> <ConfirmUndeployDialog environment={environment} /> </> ); };frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/DialogContent.test.tsx (1)
10-33: Enhance test coverage with edge cases.While the basic test cases are good, consider adding tests for:
- Empty sections or items
- Missing translations
- Verification of link rendering for
ItemWithLinkit('should handle empty sections gracefully', () => { // Mock empty data jest.spyOn(consequencesModule, 'consequencesDialogData').mockReturnValue([ { titleKey: 'empty_section', items: [] } ]); renderDialogContent('unit-test-env'); expect(screen.getByRole('heading', { name: textMock('empty_section') })).toBeInTheDocument(); }); it('should render links correctly for ItemWithLink', () => { renderDialogContent('unit-test-env'); // Find the link from consequencesDialogData const link = screen.getByRole('link', { name: textMock('app_deployment.unpublish_alternatives_change_access') }); expect(link).toHaveAttribute('href', expect.stringContaining('docs.altinn.studio')); });frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/UndeployConsequenceDialog.test.tsx (1)
10-27: Add test coverage for modal interactions and error states.While the basic modal opening test is good, consider adding tests for:
- Modal closing behavior
- Error handling
- Environment prop validation
it('should close modal when close button is clicked', async () => { const user = userEvent.setup(); renderUndeployConsequenceDialog('production'); // Open modal await user.click(screen.getByRole('button', { name: textMock('app_deployment.undeploy_button') })); // Close modal const closeButton = screen.getByRole('button', { name: textMock('common.close_button') }); await user.click(closeButton); expect(screen.queryByRole('heading', { name: textMock('app_deployment.unpublish_consequence_dialog_title') })).not.toBeInTheDocument(); }); it('should validate environment prop', () => { const consoleError = jest.spyOn(console, 'error').mockImplementation(); renderUndeployConsequenceDialog(''); expect(consoleError).toHaveBeenCalled(); consoleError.mockRestore(); });frontend/app-development/features/appPublish/components/DeployMoreOptionsMenu/DeployMoreOptionsMenu.tsx (1)
8-11: Props type could be more descriptiveConsider renaming the props type to better reflect its purpose and adding JSDoc comments for better documentation.
-type DeployMoreOptionsMenuProps = { +/** + * Props for the DeployMoreOptionsMenu component + * @property {string} environment - The deployment environment (e.g., 'production', 'staging') + * @property {string} linkToEnv - The URL to access the deployed application + */ +type DeploymentOptionsMenuProps = { environment: string; linkToEnv: string; };frontend/app-development/features/appPublish/components/DeployMoreOptionsMenu/DeployMoreOptionsMenu.test.tsx (2)
7-19: Add keyboard interaction testsWhile the click interactions are well tested, consider adding tests for keyboard navigation (Tab, Enter, Escape keys) to ensure accessibility compliance.
+ it('should handle keyboard navigation', async () => { + const user = userEvent.setup(); + renderMenu(linkToEnv); + + // Tab to focus the trigger + await user.tab(); + expect(screen.getByRole('button')).toHaveFocus(); + + // Enter to open menu + await user.keyboard('{Enter}'); + expect(screen.getAllByRole('listitem')).toHaveLength(2); + + // Escape to close menu + await user.keyboard('{Escape}'); + expect(screen.queryByRole('listitem')).not.toBeInTheDocument(); + });
49-51: Consider adding error boundary test casesThe
renderMenuhelper could be enhanced to test error scenarios.-function renderMenu(linkToEnv: string): void { +function renderMenu(linkToEnv: string, options = {}): void { + const { throwError = false } = options; render(<DeployMoreOptionsMenu linkToEnv={linkToEnv} environment='unit-test-env' />); } + +it('should handle errors gracefully', () => { + const consoleSpy = jest.spyOn(console, 'error').mockImplementation(); + expect(() => renderMenu('invalid-url', { throwError: true })).not.toThrow(); + consoleSpy.mockRestore(); +});frontend/app-development/features/appPublish/components/ConfirmUndeployDialog/ConfirmUndeployDialog.test.tsx (2)
53-72: Enhance mutation test coverage.While the test verifies that the mutation is called, consider adding:
- Verification of the loading state during mutation
- Error handling test case
- Success callback test case
Example enhancement:
it('should trigger undeploy when undeploy button is clicked', async () => { const user = userEvent.setup(); + const onSuccessMock = jest.fn(); + const onErrorMock = jest.fn(); renderConfirmUndeployDialog(); await openDialog(); const mutateFunctionMock = jest.fn(); (useUndeployMutation as jest.Mock).mockReturnValue({ mutate: mutateFunctionMock, + isLoading: false, }); const confirmTextField = getConfirmTextField(); await user.type(confirmTextField, app); await user.click(getUndeployButton()); expect(mutateFunctionMock).toBeCalledTimes(1); expect(mutateFunctionMock).toHaveBeenCalledWith( expect.objectContaining({ environment: 'unit-test-env' }), - expect.anything(), + expect.objectContaining({ + onSuccess: expect.any(Function), + onError: expect.any(Function), + }), ); + + // Test loading state + (useUndeployMutation as jest.Mock).mockReturnValue({ + mutate: mutateFunctionMock, + isLoading: true, + }); + expect(getUndeployButton()).toBeDisabled(); }); + +it('should handle errors during undeploy', async () => { + const user = userEvent.setup(); + const onErrorMock = jest.fn(); + renderConfirmUndeployDialog(); + await openDialog(); + + const mutateFunctionMock = jest.fn().mockImplementation((_, { onError }) => { + onError(new Error('Test error')); + }); + (useUndeployMutation as jest.Mock).mockReturnValue({ + mutate: mutateFunctionMock, + }); + + const confirmTextField = getConfirmTextField(); + await user.type(confirmTextField, app); + await user.click(getUndeployButton()); + + expect(onErrorMock).toHaveBeenCalled(); +});
91-95: Consider additional test scenarios for the render function.Add test cases for:
- Different environment values
- URL validation
- Provider context variations
Example:
+it('should handle different environment values', () => { + const environments = ['test', 'prod', 'staging']; + environments.forEach((env) => { + const { unmount } = renderConfirmUndeployDialog(env); + expect(screen.getByRole('dialog')).toBeInTheDocument(); + unmount(); + }); +}); + +it('should validate URL in provider context', () => { + renderConfirmUndeployDialog('test-env', { + startUrl: `${APP_DEVELOPMENT_BASENAME}/invalid/path`, + }); + expect(screen.getByRole('dialog')).toBeInTheDocument(); +});frontend/language/src/nb.json (1)
134-140: Consider adding more specific consequences about data retention.While the translations effectively communicate the immediate consequences of undeploying, consider adding information about:
- Data retention policies
- Whether the action can be reversed
- How long the data remains accessible through the Storage API
"app_deployment.unpublish_considerations_storage_api": "Du må hente ut data fra appens eksemplarer rett fra Storage API.", + "app_deployment.unpublish_considerations_data_retention": "Data vil være tilgjengelig i Storage API i X dager etter avpublisering.", + "app_deployment.unpublish_considerations_reversible": "Merk at avpublisering ikke kan angres.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
frontend/app-development/features/appPublish/components/ConfirmUndeployDialog/ConfirmUndeployDialog.module.css(1 hunks)frontend/app-development/features/appPublish/components/ConfirmUndeployDialog/ConfirmUndeployDialog.test.tsx(1 hunks)frontend/app-development/features/appPublish/components/ConfirmUndeployDialog/ConfirmUndeployDialog.tsx(1 hunks)frontend/app-development/features/appPublish/components/ConfirmUndeployDialog/index.ts(1 hunks)frontend/app-development/features/appPublish/components/DeployMoreOptionsMenu/DeployMoreOptionsMenu.module.css(1 hunks)frontend/app-development/features/appPublish/components/DeployMoreOptionsMenu/DeployMoreOptionsMenu.test.tsx(1 hunks)frontend/app-development/features/appPublish/components/DeployMoreOptionsMenu/DeployMoreOptionsMenu.tsx(1 hunks)frontend/app-development/features/appPublish/components/DeploymentEnvironment.tsx(0 hunks)frontend/app-development/features/appPublish/components/DeploymentEnvironmentStatus.module.css(0 hunks)frontend/app-development/features/appPublish/components/DeploymentEnvironmentStatus.tsx(2 hunks)frontend/app-development/features/appPublish/components/UnDeploy/UnDeploy.test.tsx(0 hunks)frontend/app-development/features/appPublish/components/UnDeploy/UnDeploy.tsx(0 hunks)frontend/app-development/features/appPublish/components/UnDeploy/index.ts(0 hunks)frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/UndeployConsequenceDialog.test.tsx(1 hunks)frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/UndeployConsequenceDialog.tsx(1 hunks)frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/DialogContent.test.tsx(1 hunks)frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/DialogContent.tsx(1 hunks)frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/ListItemWithLink.tsx(1 hunks)frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/Section/Section.module.css(1 hunks)frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/Section/Section.test.tsx(1 hunks)frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/Section/Section.tsx(1 hunks)frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/Section/index.ts(1 hunks)frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/consequences.data.ts(1 hunks)frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/utils/isItemWithLink.test.ts(1 hunks)frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/utils/isItemWithLink.ts(1 hunks)frontend/app-development/hooks/mutations/useUndeployMutation.test.ts(1 hunks)frontend/app-development/hooks/mutations/useUndeployMutation.ts(1 hunks)frontend/language/src/nb.json(2 hunks)frontend/packages/shared/src/api/mutations.ts(2 hunks)frontend/packages/shared/src/api/paths.js(1 hunks)frontend/packages/shared/src/mocks/queriesMock.ts(1 hunks)
💤 Files with no reviewable changes (5)
- frontend/app-development/features/appPublish/components/UnDeploy/index.ts
- frontend/app-development/features/appPublish/components/UnDeploy/UnDeploy.test.tsx
- frontend/app-development/features/appPublish/components/DeploymentEnvironment.tsx
- frontend/app-development/features/appPublish/components/UnDeploy/UnDeploy.tsx
- frontend/app-development/features/appPublish/components/DeploymentEnvironmentStatus.module.css
✅ Files skipped from review due to trivial changes (5)
- frontend/app-development/features/appPublish/components/ConfirmUndeployDialog/ConfirmUndeployDialog.module.css
- frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/Section/index.ts
- frontend/app-development/features/appPublish/components/ConfirmUndeployDialog/index.ts
- frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/Section/Section.module.css
- frontend/app-development/features/appPublish/components/DeployMoreOptionsMenu/DeployMoreOptionsMenu.module.css
🔇 Additional comments (17)
frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/utils/isItemWithLink.ts (1)
3-5: Well-implemented type guard!The type guard implementation is clean, focused, and follows TypeScript best practices by correctly using the type predicate syntax.
frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/Section/Section.tsx (1)
5-8: LGTM! Well-structured props interface.The props interface is clear and properly typed, following TypeScript best practices.
frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/consequences.data.ts (1)
3-14: LGTM! Well-structured type definitions.The type definitions are clear, extensible, and follow TypeScript best practices. Good use of type composition with
ItemWithLinkextendingItem.frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/DialogContent.tsx (1)
10-13: LGTM! Clean prop type definition.The prop type is well-defined and follows TypeScript best practices.
frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/components/DialogContent.test.tsx (1)
35-39: LGTM! Clean test helper implementation.The helper function is well-structured and provides good reusability for the tests.
frontend/app-development/features/appPublish/components/UndeployConsequenceDialog/UndeployConsequenceDialog.test.tsx (1)
29-33: LGTM! Well-structured test helper.The helper function provides a clean way to render the component with necessary context.
frontend/app-development/features/appPublish/components/DeployMoreOptionsMenu/DeployMoreOptionsMenu.tsx (2)
19-27: LGTM! Good accessibility implementationThe menu trigger button is properly implemented with:
- Appropriate ARIA label for screen readers
- Consistent button sizing
- Clear visual feedback through the icon
34-46: LGTM! Secure external link implementationThe external link button correctly implements security best practices:
- Uses
rel="noopener noreferrer"for security- Opens in a new tab
- Includes a visual indicator (ExternalLinkIcon)
frontend/app-development/features/appPublish/components/DeploymentEnvironmentStatus.tsx (1)
52-54: LGTM! Clean feature flag implementationThe DeployMoreOptionsMenu is properly gated behind both a feature flag and the presence of deployment version, ensuring controlled rollout.
frontend/app-development/features/appPublish/components/ConfirmUndeployDialog/ConfirmUndeployDialog.test.tsx (2)
1-12: Well-structured test setup with appropriate mocks and imports.The test file follows testing best practices by:
- Using appropriate testing libraries
- Properly mocking the mutation hook
- Organizing imports logically
75-89: Well-structured helper functions improving test maintainability.The helper functions are:
- Clearly named and focused
- Reusable across test cases
- Using proper testing-library queries
frontend/packages/shared/src/api/mutations.ts (1)
99-99: Clean implementation of undeployment mutation.The mutation follows the established patterns:
- Consistent parameter ordering (org, app, environment)
- Uses POST method appropriately
- Properly structured payload
frontend/packages/shared/src/api/paths.js (1)
103-103: Well-placed and consistent path definition.The path:
- Is grouped with related deployment paths
- Follows the established URL pattern
- Uses consistent parameter naming
frontend/packages/shared/src/mocks/queriesMock.ts (1)
253-253: Appropriate mock implementation.The mock:
- Follows the established mocking pattern
- Returns a resolved promise as expected
- Is properly placed in the mutations section
frontend/language/src/nb.json (3)
84-84: LGTM: Deploy menu translations are clear and consistent.The translations for the deployment menu options are well-written and accurately describe the functionality:
- "Åpne meny med tilgang til miljø og mulighet for avpublisering" clearly indicates the menu's purpose
- "Gå til miljøet" is a concise action text
Also applies to: 89-89
124-128: LGTM: Undeploy button and confirmation dialog translations are clear and user-friendly.The translations effectively guide users through the undeployment process:
- Consistent use of "avpubliser" terminology
- Clear confirmation dialog that requires typing the app name
- Well-structured warning about the consequences
129-133: LGTM: Alternative actions are well explained.The translations provide clear alternatives to undeploying:
- Each alternative action is clearly explained
- Good use of links in the text (e.g.,
<a>Endre innstillingen for tilgang eller oppstart</a>)- Maintains consistent terminology
frontend/app-development/hooks/mutations/useUndeployMutation.ts
Outdated
Show resolved
Hide resolved
...ent/features/appPublish/components/UndeployConsequenceDialog/components/ListItemWithLink.tsx
Outdated
Show resolved
Hide resolved
...pment/features/appPublish/components/UndeployConsequenceDialog/UndeployConsequenceDialog.tsx
Outdated
Show resolved
Hide resolved
...pment/features/appPublish/components/UndeployConsequenceDialog/UndeployConsequenceDialog.tsx
Show resolved
Hide resolved
...p-development/features/appPublish/components/ConfirmUndeployDialog/ConfirmUndeployDialog.tsx
Outdated
Show resolved
Hide resolved
...p-development/features/appPublish/components/ConfirmUndeployDialog/ConfirmUndeployDialog.tsx
Outdated
Show resolved
Hide resolved
...p-development/features/appPublish/components/ConfirmUndeployDialog/ConfirmUndeployDialog.tsx
Show resolved
Hide resolved
| { textKey: 'app_deployment.unpublish_alternatives_make_unavailable' }, | ||
| { | ||
| textKey: 'app_deployment.unpublish_alternatives_change_access', | ||
| link: 'https://docs.altinn.studio//nb/altinn-studio/reference/logic/instantiation/', |
There was a problem hiding this comment.
There is a double // before nb here. Don't think it actually causes an issue, but I think we have a utils function that returns the docs base path with the correct language that we might use instead?
...p-development/features/appPublish/components/DeployMoreOptionsMenu/DeployMoreOptionsMenu.tsx
Show resolved
Hide resolved
Yes, I saw the comments from CodeRabbit, many of them will be implemented asap. :) |
Co-authored-by: Nina Kylstad <[email protected]>
Co-authored-by: Nina Kylstad <[email protected]>
| { textKey: 'app_deployment.unpublish_alternatives_make_unavailable' }, | ||
| { | ||
| textKey: 'app_deployment.unpublish_alternatives_change_access', | ||
| link: 'app_deployment.unpublish_alternatives_change_access_link', |
There was a problem hiding this comment.
We have a function in shared/src/ext-urls calles altinnDocsUrl that takes in a language and relative URL, and returns the fully qualified url to the docs. Perhaps that could be used here rather than putting a static link in the text resources?
Description
This PR implements a context that allows users to choose between going to the app in the environment or undeploying the app. When the user selects the undeploy option, a dialog is shown to inform them of the consequences of undeploying the app. If the user proceeds, they are then prompted with a confirmation dialog to ensure they understand the impact of undeploying. This flow ensures that users are fully aware of the actions they are taking and the potential consequences.
Related Issue(s)
Verification
Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
New Features
UndeployConsequenceDialogfor user confirmations.DeployMoreOptionsMenufor additional deployment actions.Improvements
Bug Fixes
Documentation