Skip to content

feat: add 'impending deletion' badges to workspaces page #7530

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
May 15, 2023
Merged

feat: add 'impending deletion' badges to workspaces page #7530

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0ba53bd
update deleting logic
Kira-Pilot May 11, 2023
1255b07
added status badge on workspaces page
Kira-Pilot May 12, 2023
3618c5d
licensing and feature flagging
Kira-Pilot May 12, 2023
d6adbd9
preset filter for failed workspaces
Kira-Pilot May 12, 2023
e00ed2a
remove comment
Kira-Pilot May 15, 2023
2dfbb50
PR feedback
Kira-Pilot May 15, 2023
3c72d11
Revert "PR feedback"
Kira-Pilot May 15, 2023
fc8e51c
PR feedback 2
Kira-Pilot May 15, 2023
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
Prev Previous commit
Next Next commit
PR feedback
  • Loading branch information
@Kira-Pilot
Kira-Pilot committed May 15, 2023
commit 2dfbb50acd2a564d92e39c7cea2886c3e876972c
76 changes: 0 additions & 76 deletions site/src/components/WorkspaceStatusBadge/WorkspaceStatusBadge.stories.tsx
View file
Open in desktop

This file was deleted.

29 changes: 19 additions & 10 deletions site/src/components/WorkspaceStatusBadge/WorkspaceStatusBadge.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,11 @@ export type WorkspaceStatusBadgeProps = {
}

const ImpendingDeletionBadge: FC<
PropsWithChildren<Partial<WorkspaceStatusBadgeProps>>
> = ({ className }) => {
const { entitlements, experiments } = useDashboard()
const allowAdvancedScheduling =
entitlements.features["advanced_template_scheduling"].enabled
// This check can be removed when https://github.com/coder/coder/milestone/19
// is merged up
const allowWorkspaceActions = experiments.includes("workspace_actions")

Partial<WorkspaceStatusBadgeProps> & {
allowAdvancedScheduling: boolean
allowWorkspaceActions: boolean
}
> = ({ allowAdvancedScheduling, allowWorkspaceActions, className }) => {
if (!allowAdvancedScheduling || !allowWorkspaceActions) {
return null
}
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Since we are already checking if we should or not display the badge on displayImpendingDeletion I think we should move those checks to be there since they are doing the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

@BrunoQuaresma those checks rely on a hook, which has to be called in a component. Would you rather I call the hook in the WorkspaceStatusBadge component? I definitely could; I just didn't want to screw up all the storybook stories.

Alternatively, I thought about calling this hook in WorkspacesPage and then passing down props from there - but there are like 7 layers of prop passing and it just seemed like a lot to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The thing for me is to have the "should display or not" check in one single place. Splitting this check into different places looks confusing to me. But if it is hard, I would add a comment so that in the future, if we want to change the display behavior, we know there are two places to look for.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed - as the ideal behavior is to have the check in one place, I think the best path forward may be to call the hook in the badge component and then ignore Storybook's complaints, rather than let our testing tool dictate how we code.
I'll adjust.

Expand All @@ -119,8 +115,21 @@ const ImpendingDeletionBadge: FC<
export const WorkspaceStatusBadge: FC<
PropsWithChildren<WorkspaceStatusBadgeProps>
> = ({ workspace, className }) => {
const { entitlements, experiments } = useDashboard()
const allowAdvancedScheduling =
entitlements.features["advanced_template_scheduling"].enabled
// This check can be removed when https://github.com/coder/coder/milestone/19
// is merged up
const allowWorkspaceActions = experiments.includes("workspace_actions")

if (displayImpendingDeletion(workspace)) {
return <ImpendingDeletionBadge className={className} />
return (
<ImpendingDeletionBadge
className={className}
allowAdvancedScheduling={allowAdvancedScheduling}
allowWorkspaceActions={allowWorkspaceActions}
/>
)
}

const { text, icon, type } = getStatus(workspace.latest_build.status)
Expand Down