Skip to content

Org SSO Page updates #16868

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

Merged
merged 8 commits into from
Mar 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion components/dashboard/src/components/Modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export const ModalFooter: FC<ModalFooterProps> = ({ error, warning, children })
return (
<div className="relative">
{hasAlert && (
<div className="absolute -top-12 left-0 right-0" style={alertStyles}>
<div className="absolute bottom-12 left-0 right-0" style={alertStyles}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Positioning off of the bottom allows it to grow taller if needed (still some styling issues for long error messages, but this should be a small improvement).

Copy link
Contributor

@gtsiolis gtsiolis Mar 16, 2023

Choose a reason for hiding this comment

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

issue: Using the alert component here sounds perfect, however, using alerts or error feedback inside modals can be tricky, especially after adding a scrollable area inside the modal in #15084.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I think having the alert positioned absolutely somewhere so it's kind of pinned at the footer is one way to avoid some of those issues (but does introduce some other challenges). Currently I'm using a pattern where the alert is pinned right above the footer (part of the footer actually). I've added some extra padding to the bottom of the modal body section to account for it, but that's a temporary solution and not ideal. What I'd like to do:

  • Animate the alert so it slides up from the footer.
  • Find a more robust way to deal with the alert potentially blocking content in the modal body. Some ideas:
    • Dynamically adding padding to the bottom of the modal body based on the size of the alert when it's present so content can be scrolled to in the body.
    • Making the pinned modal alert dismissible (probably the easiest solution to implement).

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Since we're having validation inline for text input, we could do the following in this or a follow PR:

  1. Skip the alert here and rely on the SSO clients list to communicate an error or a failed attempt.
  2. Migrate away from the modals to a full page layout as we do with the access tokens flow, which is more appropriate, scalable, and accessible.
Input validation Error feedback through list Error feedback in full page
Screenshot 2023-03-16 at 14 36 12 Screenshot 2023-03-16 at 14 36 46 Screenshot 2023-03-16 at 14 36 58

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points.

If we stuck with the modal, we'd need some version of an alert for any error messages returned from the api call during create/update.

I'd be on board with using full pages here. I debated going down that route, but felt like a bigger change I didn't want to introduce quite yet. It would also simplify some of the complexity in the git auth modal that deals with managing state after creation.

Either way, I think having a way to show simple error messages for a modal in a consistent way will helpful, but need to address some of the usability points we've discussed.

Copy link
Contributor

Choose a reason for hiding this comment

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

over-here

<ModalFooterAlert error={error} warning={warning} />
</div>
)}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/**
* Copyright (c) 2023 Gitpod GmbH. All rights reserved.
* Licensed under the GNU Affero General Public License (AGPL).
* See License.AGPL.txt in the project root for license information.
*/

import { useMutation, useQueryClient } from "@tanstack/react-query";
import { oidcService } from "../../service/public-api";
import { useCurrentOrg } from "../organizations/orgs-query";
import { getOIDCClientsQueryKey, OIDCClientsQueryResults } from "./oidc-clients-query";

type DeleteOIDCClientArgs = {
clientId: string;
};
export const useDeleteOIDCClientMutation = () => {
const queryClient = useQueryClient();
const organization = useCurrentOrg().data;

return useMutation({
mutationFn: async ({ clientId }: DeleteOIDCClientArgs) => {
if (!organization) {
throw new Error("No current organization selected");
}

return await oidcService.deleteClientConfig({
id: clientId,
organizationId: organization.id,
});
},
onSuccess: (_, { clientId }) => {
if (!organization) {
throw new Error("No current organization selected");
}

const queryKey = getOIDCClientsQueryKey(organization.id);
// filter out deleted client immediately
queryClient.setQueryData<OIDCClientsQueryResults>(queryKey, (clients) => {
return clients?.filter((c) => c.id !== clientId);
});

// then invalidate query
queryClient.invalidateQueries({ queryKey });
},
});
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* Copyright (c) 2023 Gitpod GmbH. All rights reserved.
* Licensed under the GNU Affero General Public License (AGPL).
* See License.AGPL.txt in the project root for license information.
*/

import { OIDCClientConfig } from "@gitpod/public-api/lib/gitpod/experimental/v1/oidc_pb";
import { useQuery } from "@tanstack/react-query";
import { oidcService } from "../../service/public-api";
import { useCurrentOrg } from "../organizations/orgs-query";

export type OIDCClientsQueryResults = OIDCClientConfig[];

export const useOIDCClientsQuery = () => {
const { data: organization, isLoading } = useCurrentOrg();

return useQuery<OIDCClientsQueryResults>({
queryKey: getOIDCClientsQueryKey(organization?.id ?? ""),
queryFn: async () => {
if (!organization) {
throw new Error("No current organization selected");
}

const { clientConfigs } = await oidcService.listClientConfigs({ organizationId: organization.id });

return clientConfigs;
},
enabled: !isLoading,
});
};

export const getOIDCClientsQueryKey = (organizationId: string) => ["oidc-clients", { organizationId }];
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/**
* Copyright (c) 2023 Gitpod GmbH. All rights reserved.
* Licensed under the GNU Affero General Public License (AGPL).
* See License.AGPL.txt in the project root for license information.
*/

import { useMutation, useQueryClient } from "@tanstack/react-query";
import { oidcService } from "../../service/public-api";
import { getOIDCClientsQueryKey } from "./oidc-clients-query";

// TODO: find a better way to type this against the API
type UpsertOIDCClientMutationArgs =
| Parameters<typeof oidcService.updateClientConfig>[0]
| Parameters<typeof oidcService.createClientConfig>[0];

export const useUpsertOIDCClientMutation = () => {
const queryClient = useQueryClient();

return useMutation({
mutationFn: async ({ config = {} }: UpsertOIDCClientMutationArgs) => {
if ("id" in config) {
return await oidcService.updateClientConfig({
config,
});
} else {
return await oidcService.createClientConfig({
config,
});
}
},
onSuccess(resp, { config = {} }) {
if (!config || !config.organizationId) {
return;
}

queryClient.invalidateQueries({ queryKey: getOIDCClientsQueryKey(config.organizationId || "") });
},
});
};
41 changes: 3 additions & 38 deletions components/dashboard/src/teams/GitIntegrationsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,48 +4,13 @@
* See License.AGPL.txt in the project root for license information.
*/

import { FunctionComponent } from "react";
import { Redirect } from "react-router";
import Header from "../components/Header";
import { SpinnerLoader } from "../components/Loader";
import { useCurrentOrg } from "../data/organizations/orgs-query";
import { GitIntegrations } from "./git-integrations/GitIntegrations";
import { OrgSettingsPage } from "./OrgSettingsPage";

export default function GitAuth() {
export default function GitAuthPage() {
return (
<OrgSettingsPageWrapper>
<OrgSettingsPage title="Git Auth" subtitle="Configure Git Auth for GitLab or Github.">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapper component no longer needed, as the loading state is handled in <OrgSettingsPage/> now.

Copy link
Contributor

Choose a reason for hiding this comment

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

issue: I wanted to mention this in #16850, but the goal of the settings page was to keep the Organizations Settings in the header around for all pages under settings, and rely on the left sidebar for navigation and less duplication of labels. Cc @AlexTugarev

Example of labels repeating
Frame 1303

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's my mistake then, I understood some of that differently. I can change this back if that's what we want in a followup pretty easily. It is kinda awkward coming up with 3 versions of the page title that are slightly different, so this would remove that 😄

<GitIntegrations />
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: We can probably update the copy in the empty state for Git integrations, but we can make these in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

praise: This is out of the scope of the changes in this PR, but wanted to say thanks for improving the copy in #16850 from the previous iteration. 🌮 🌮

</OrgSettingsPageWrapper>
);
}

// TODO: Refactor this into OrgSettingsPage so each page doesn't have to do this
export const OrgSettingsPageWrapper: FunctionComponent = ({ children }) => {
const currentOrg = useCurrentOrg();

const title = "Git Auth";
const subtitle = "Configure Git Auth for GitLab, or Github.";

// Render as much of the page as we can in a loading state to avoid content shift
if (currentOrg.isLoading) {
return (
<div className="w-full">
<Header title={title} subtitle={subtitle} />
<div className="w-full">
<SpinnerLoader />
</div>
</div>
);
}

if (!currentOrg.data?.isOwner) {
return <Redirect to={"/"} />;
}

return (
<OrgSettingsPage title={title} subtitle={subtitle}>
{children}
</OrgSettingsPage>
);
};
}
Loading