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

feat: additional error messages for duplicate flow name and moving a flow #4244

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
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
27 changes: 20 additions & 7 deletions editor.planx.uk/src/pages/FlowEditor/lib/store/editor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import omitBy from "lodash/omitBy";
import { customAlphabet } from "nanoid-good";
import en from "nanoid-good/locale/en";
import { type } from "ot-json0";
import { slugify } from "utils";
import type { StateCreator } from "zustand";
import { persist } from "zustand/middleware";

Expand Down Expand Up @@ -177,7 +178,11 @@ export interface EditorStore extends Store.Store {
setLastPublishedDate: (date: string) => void;
isFlowPublished: boolean;
makeUnique: (id: NodeId, parent?: NodeId) => void;
moveFlow: (flowId: string, teamSlug: string) => Promise<any>;
moveFlow: (
flowId: string,
teamName: string,
flowName: string,
) => Promise<any>;
moveNode: (
id: NodeId,
parent?: NodeId,
Expand Down Expand Up @@ -485,7 +490,8 @@ export const editorStore: StateCreator<
send(ops);
},

moveFlow(flowId: string, teamSlug: string) {
moveFlow(flowId: string, teamName: string, flowName: string) {
const teamSlug = slugify(teamName);
Copy link
Member

Choose a reason for hiding this comment

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

Why is moveFlow now accepting a teamName rather than teamSlug? How is slugify(teamName) handling examples like "Barking & Dagenham", "Epsom & Ewell"?

Copy link
Member

Choose a reason for hiding this comment

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

Assuming you've swapped teamSlug to teamName for the sake of error message formatting alone, I don't think it's a problem for the raw team slug to be in the error message (it's a very basic native alert still afterall!)

const valid = get().canUserEditTeam(teamSlug);
if (!valid) {
alert(
Expand All @@ -507,11 +513,18 @@ export const editorStore: StateCreator<
},
)
.then((res) => alert(res?.data?.message))
.catch(() =>
alert(
"Failed to move this flow. Make sure you're entering a valid team name and try again",
),
);
.catch(({ response }) => {
const { data } = response;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Iterated on this a few times to get the cleanest code, I settled on this as it balanced explicit variable naming with concise lines of code in what is quite a long function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would appreciate feedback here, other iterations included a similar conditional check for when to display the 'Failed to move this flow.' alert, but I am, as ever, treading lightly.

Copy link
Member

Choose a reason for hiding this comment

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

This feels like clear, conditional error handling and meets requirements of more specific error messages I think 👍

(A frank question: would these few lines alone satisfy the original Trello ticket?)

if (data.error.toLowerCase().includes("uniqueness violation")) {
alert(
`The ${teamName} team already has a service with name '${flowName}'. Rename the service and try again `,
Copy link
Member

Choose a reason for hiding this comment

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

nit: The ${teamName} team already has... can probably just be ${teamName} already has... ? I don't think "Lambeth team" matches how a user would call their workspace.

Is it also worth keeping the "Failed to move flow" prefix, in which case we should use "flow" not "service" consistently?

);
} else {
alert(
"Failed to move this flow. Make sure you're entering a valid team name and try again",
);
}
});
},

moveNode(
Expand Down
53 changes: 31 additions & 22 deletions editor.planx.uk/src/pages/Team.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,10 @@ const FlowItem: React.FC<FlowItemProps> = ({
refreshFlows();
});
};
const handleMove = (newTeam: string) => {
const handleMove = (newTeam: string, flowName: string) => {
useStore
.getState()
.moveFlow(flow.id, newTeam)
.moveFlow(flow.id, newTeam, flowName)
.then(() => {
refreshFlows();
});
Expand Down Expand Up @@ -182,11 +182,11 @@ const FlowItem: React.FC<FlowItemProps> = ({
onClick: async () => {
const newName = prompt("New name", flow.name);
if (newName && newName !== flow.name) {
const newSlug = slugify(newName);
const duplicateFlowName = flows?.find(
(flow: any) => flow.slug === newSlug,
const verifiedNameAndSlug = checkForDuplicateFlowName(
Copy link
Member

Choose a reason for hiding this comment

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

nit: "uniqueFlow" (then "uniqueFlow.slug", "uniqueFlow.name") would be more clear to me than "verified". We also have a mix of team and flow names/slugs throughout this file, so nice to be specific about which we're dealing with!

newName,
flows,
);
if (!duplicateFlowName) {
if (verifiedNameAndSlug) {
await client.mutate({
mutation: gql`
mutation UpdateFlowSlug(
Expand All @@ -209,16 +209,12 @@ const FlowItem: React.FC<FlowItemProps> = ({
variables: {
teamId: teamId,
slug: flow.slug,
newSlug: newSlug,
newName: newName,
newSlug: verifiedNameAndSlug.slug,
newName: verifiedNameAndSlug.name,
},
});

refreshFlows();
} else if (duplicateFlowName) {
alert(
`The flow "${newName}" already exists. Enter a unique flow name to continue`,
);
}
}
},
Expand All @@ -240,7 +236,7 @@ const FlowItem: React.FC<FlowItemProps> = ({
`This flow already belongs to ${teamSlug}, skipping move`,
);
} else {
handleMove(slugify(newTeam));
handleMove(newTeam, flow.name);
}
}
},
Expand Down Expand Up @@ -279,6 +275,21 @@ const GetStarted: React.FC<{ flows: FlowSummary[] }> = ({ flows }) => (
</Box>
);

const checkForDuplicateFlowName = (name: string, flows: FlowSummary[]) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is part of me which questions the position of this function in the file and if it should be placed in a file outside of the main component, but considering the other work happening right now with this file for Filters and Search, it seems better to consider this after these have been added and polished

Copy link
Member

@jessicamcinchak jessicamcinchak Feb 4, 2025

Choose a reason for hiding this comment

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

nit: the name of this function confuses me on a couple levels:

  • checkFor... does not naturally indicate that it's going to return a name and slug, or possibly prompt the user for new input (at least in my opinion)
  • ..DuplidateFlowName fails to acknowledge that the actual comparison logic references flow slugs

Would getUniqueFlow better communicate what is going to be returned? Like we talked about it another recent PR, a return type would also help communicate what's happening / possible here - eg { flow: string; slug: string; } | undefined !

Copy link
Member

Choose a reason for hiding this comment

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

Also more to your original question - I don't mind this helper function being added into this existing file right now, but I do prefer helper functions to be added to the bottom rather than in between component definitions.

const newFlowSlug = slugify(name);
const duplicateFlowName = flows?.find((flow) => flow.slug === newFlowSlug);

if (duplicateFlowName) {
const updatedName = prompt(
`A service already exists with the name '${name}', enter another name`,
name,
);
Copy link
Member

Choose a reason for hiding this comment

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

nit: again I think we're using "service" and "flow" inconsistently and should keep our prior language here? Eg The flow "${newName}" already exists. Enter a unique flow name to continue

if (!updatedName) return;
return checkForDuplicateFlowName(updatedName, flows);
}
return { slug: newFlowSlug, name: name };
};

const AddFlowButton: React.FC<{ flows: FlowSummary[] }> = ({ flows }) => {
const { navigate } = useNavigation();
const { teamId, createFlow, teamSlug } = useStore();
Expand All @@ -287,18 +298,16 @@ const AddFlowButton: React.FC<{ flows: FlowSummary[] }> = ({ flows }) => {
const newFlowName = prompt("Service name");
if (!newFlowName) return;

const newFlowSlug = slugify(newFlowName);
const duplicateFlowName = flows?.find((flow) => flow.slug === newFlowSlug);
const verifiedNameAndSlug = checkForDuplicateFlowName(newFlowName, flows);

if (duplicateFlowName) {
alert(
`The flow "${newFlowName}" already exists. Enter a unique flow name to continue`,
if (verifiedNameAndSlug) {
const newId = await createFlow(
teamId,
verifiedNameAndSlug.slug,
verifiedNameAndSlug.name,
);
return;
navigate(`/${teamSlug}/${newId}`);
}

const newId = await createFlow(teamId, newFlowSlug, newFlowName);
navigate(`/${teamSlug}/${newId}`);
};

return <AddButton onClick={addFlow}>Add a new service</AddButton>;
Expand Down
Loading