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

EVG-18784 Warn users when creating/copying projects at limit #1681

Merged
merged 10 commits into from
Feb 27, 2023

Conversation

bynn
Copy link
Contributor

@bynn bynn commented Feb 14, 2023

EVG-18784

Description

users now get a warning when creating/copying projects if limit has been reached

Screenshots

image

Testing

added test
works on staging

Evergreen PR

evergreen-ci/evergreen#6259

@bynn bynn changed the title Evg 18784 EVG-18784 Warn users when creating/copying projects at limit Feb 14, 2023
@bynn bynn requested a review from a team February 14, 2023 02:29
@cypress
Copy link

cypress bot commented Feb 14, 2023

Passing run #8233 ↗︎

0 543 4 0 Flakiness 0

Details:

Merge branch 'main' into EVG-18784
Project: Spruce Commit: 760f002142
Status: Passed Duration: 13:41 💡
Started: Feb 27, 2023 7:40 PM Ended: Feb 27, 2023 7:54 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@@ -480,25 +480,6 @@ export type LogMessage = {
version?: Maybe<Scalars["Int"]>;
};

export type LogkeeperBuild = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make sure to run yarn codegen while checking out the most recent main branch in Evergreen? I don't think this should be getting deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -29,7 +29,9 @@ export const CreateProjectModal: React.VFC<Props> = ({
owner,
repo,
}) => {
const dispatchToast = useToastContext();
// const dispatchToast = useToastContext();
const { error: errorToast, success, warning } = useToastContext();
Copy link
Contributor

Choose a reason for hiding this comment

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

we haven't been using this destructuring pattern for things like toasts. can we use the previous pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 54 to 76
const [createProject, { called, data, error, loading }] = useMutation<
CreateProjectMutation,
CreateProjectMutationVariables
>(CREATE_PROJECT, {
onCompleted({ createProject: { identifier } }) {
dispatchToast.success(`Successfully created the project: ${identifier}`);
>(CREATE_PROJECT, { errorPolicy: "all" });

useEffect(() => {
// onCompleted and onError don't provide sufficient information when used with errorPolicy: 'all', so use hook to manage behavior after confirming modal.
// https://github.com/apollographql/apollo-client/issues/6966
if (!called || loading) {
return;
}

const identifier = data?.createProject?.identifier;
if (identifier) {
if (error) {
warning(
`The project was successfully created with the following errors: ${error.message}.`,
true,
{ shouldTimeout: false }
);
} else {
success(`Successfully created the project: ${identifier}`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by what you are trying to do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if there is an error and the identifier is returned, we want to warn because the project was created but won't be able to be enabled
and it will error or not error normally.

sophie's pr might explain it better than me #1257

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, interesting thank you for explaining. If this is a pattern we want to adopt maybe we should support it more officially. apollographql/apollo-client#6966 (comment) cc @sophstad

Copy link
Contributor

Choose a reason for hiding this comment

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

I created EVG-19066 to standardize support for this pattern. I think this PR is fine as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

},
});
} else if (error) {
errorToast(`There was an error creating the project: ${error?.message}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
errorToast(`There was an error creating the project: ${error?.message}`);
dispatchToast.error(`There was an error creating the project: ${error?.message}`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@SupaJoon SupaJoon self-requested a review February 21, 2023 15:07
@@ -58,11 +58,13 @@ export const CopyProjectModal: React.VFC<Props> = ({
{ shouldTimeout: false }
);
} else {
success(`Successfully created the project: ${identifier}`);
success(`Successfully duplicated the project: ${identifier}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

@@ -58,11 +58,13 @@ export const CopyProjectModal: React.VFC<Props> = ({
{ shouldTimeout: false }
);
} else {
success(`Successfully created the project: ${identifier}`);
success(`Successfully duplicated the project: ${identifier}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was here before but can you refactor this to use the dispatchToast.status() pattern.
Just so we have consistent usage through out the code base.

@bynn bynn merged commit d5df80e into evergreen-ci:main Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants