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

RODO94 · 2025-02-03T13:35:00Z

Where does this PR come from?

This PR is linked to this Trello ticket: https://trello.com/c/Lsjy4ZgQ/3195-add-descriptive-error-message-for-duplicate-slug-when-copying-a-service-and-moving-it-to-a-new-team

It comes from a situation where a flow was copied and the user tried to move it to another team, but the other team already had a flow with the same name. No useful error was shown to point the user towards how to resolve it. This is what I have added here.

What has been added?

To guide users towards resolving errors themselves, I have added two new pathways / error messages.

For duplicate names within a team, I have added a new prompt pre-populated with the first attempted name

This should help users tweak their initial trial to push it through.

I have added a conditional alert for the uniqueness violation error messages which points towards a slug duplication within a team.

The struggle I had here was how to efficiently compare the slug name with that of the new team. This approach seemed the lightest without adding a new getFlows() with the new team ID, which could maybe be added at the api level or in the frontend, but this seemed to grow the boundaries of this task. I was also thinking of a future where we use the Dialog components we have created for user lists for services, changing the way users interact with these actions.

editor.planx.uk/src/pages/Team.tsx

RODO94 · 2025-02-03T13:41:33Z

editor.planx.uk/src/pages/FlowEditor/lib/store/editor.ts

-        ),
-      );
+      .catch(({ response }) => {
+        const { data } = response;


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

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.

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?)

Yeah this alone would, the other error message changes were something I ran into myself, and thought it would be an easy change to add in. Do you think I should remove the other changes?

Not at all asking you to remove the other changes, I agree it's ultimately a nicer user experience, just trying to gauge how requirements versus nice-to-haves were interpreted here.

It does seem like the additional prompt added more time, questions, and complexity, so it's important to be able to identify when & how we make decisions to prioritise additional "nice to haves" when reflecting on sprint tasks !

github-actions · 2025-02-03T14:00:23Z

Pizza

Deployed 290bb64 to https://4244.planx.pizza.

Useful links:

jessicamcinchak

Unique error messages are a definite improvement here!

A few initial comments & bugs - please try moving a flow to the valid team name "Barking & Dagenham" on the pizza 🐛

jessicamcinchak · 2025-02-04T20:15:47Z

editor.planx.uk/src/pages/FlowEditor/lib/store/editor.ts

@@ -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);


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

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!)

Yeah I switched it so we could take in a teamName so it can be shown in the error messages, which I got feedback on from Jonny, I had it like you suggested, but he did ask that we show the teamName as its added. I did find it hard cause at one point you want to show the user that what they've entered isn't working, but then it obscures the fact we actually use the slug for matching it.

On the Barking & Dagenham one, this is interesting because the slug uses and but the name uses & - but when you slugify('Barking & Dagenham') you get barking-dagenham not barking-and-dagenham. Has this been manually added in the database?

I tried the same thing in staging and I get the same error, so I don't think it's a bug I've introduced here? And I'm not sure how I'd resolve it, other than updating how we handle & in the database slug

This error does not exist on staging because staging prompts us for the slug ! I can successfully move a flow to B&G, which does not work on your pizza if I have to enter the team name and it gets automatically slugified

From memory, team slugs do not follow the automatic slugify convention because they need to be able to coform to/match council subdomains.

In my opinion, the prompt should still take a team slug, which editors can always find in the URL.

jessicamcinchak · 2025-02-04T20:39:54Z

editor.planx.uk/src/pages/FlowEditor/lib/store/editor.ts

-        ),
-      );
+      .catch(({ response }) => {
+        const { data } = response;


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?)

editor.planx.uk/src/pages/FlowEditor/lib/store/editor.ts

editor.planx.uk/src/pages/Team.tsx

RODO94 · 2025-02-05T15:20:38Z

editor.planx.uk/src/pages/Team.tsx

                  if (newTeam) {
                    if (slugify(newTeam) === teamSlug) {
                      alert(
                        `This flow already belongs to ${teamSlug}, skipping move`,
                      );
                    } else {
-                      handleMove(slugify(newTeam));


As we chatted about, I removed the adding of teamName here and reverted it back to original state with slugify(teamName), but with flow.name which seems harmless.

We should consider a future PR improving this error state though

RODO94 · 2025-02-05T15:20:59Z

editor.planx.uk/src/pages/Team.tsx

@@ -233,14 +226,16 @@ const FlowItem: React.FC<FlowItemProps> = ({
              {
                label: "Move",
                onClick: () => {
-                  const newTeam = prompt("New team");
+                  const newTeam = prompt(
+                    "Add the new team's slug. A slug is a way to represent a team name, for example 'Barking & Dagenham' would be 'barking-and-dagenham'. ",


Added more specificity here and using B&D as an example as it's probably the biggest source of error in this flow.

nit: I would say "Enter the new team's slug. A slug is the URL name for a team, for example..." (then users know where to look to find theres!)

jessicamcinchak

Thanks for changes here ! All looks good and pizza behaving as expected

jessicamcinchak · 2025-02-05T16:21:21Z

editor.planx.uk/src/pages/Team.tsx

@@ -233,14 +226,16 @@ const FlowItem: React.FC<FlowItemProps> = ({
              {
                label: "Move",
                onClick: () => {
-                  const newTeam = prompt("New team");
+                  const newTeam = prompt(
+                    "Add the new team's slug. A slug is a way to represent a team name, for example 'Barking & Dagenham' would be 'barking-and-dagenham'. ",


nit: I would say "Enter the new team's slug. A slug is the URL name for a team, for example..." (then users know where to look to find theres!)

jessicamcinchak · 2025-02-05T16:22:14Z

editor.planx.uk/src/pages/Team.tsx

+    return getUniqueFlow(updatedName, flows);
+  }
+  return { slug: newFlowSlug, name: name };
+};


This is much clearer ➕

RODO94 · 2025-02-06T09:14:34Z

@jessicamcinchak at the end of the day yesterday, Jonny said "destination" team sounds better than "new" team, since "new" might indicate they are making a new team. It seems a good switch to me and quite minor still, so I've implemented it.

I'll mention it at Dev call to check your thoughts prior to merge

jessicamcinchak · 2025-02-06T09:19:47Z

@RODO94 my priority was reviewing/approving code here, Jonny's final wording suggestion is fine and there's no reason to wait to bring this to dev call in my opinion - let's please get this one merged so it's marked as "done" ahead of planning this afternoon !

initial error message additions

ae7223b

RODO94 commented Feb 3, 2025

View reviewed changes

editor.planx.uk/src/pages/Team.tsx Outdated Show resolved Hide resolved

RODO94 commented Feb 3, 2025

View reviewed changes

RODO94 added 4 commits February 3, 2025 16:32

refine wording of error message

608b331

refine error messaging

d982327

use user given teamName for error messaging

cf5fde2

remove semi colon

ef10d45

RODO94 marked this pull request as ready for review February 4, 2025 15:18

jessicamcinchak reviewed Feb 4, 2025

View reviewed changes

RODO94 added 3 commits February 5, 2025 11:59

add in changes from Jess

6e0ca97

switch from service to flow

013d88f

add back in slugify() to handleFlow()

912d836

RODO94 commented Feb 5, 2025

View reviewed changes

jessicamcinchak approved these changes Feb 5, 2025

View reviewed changes

update prompt message

1a1c5cb

RODO94 merged commit 7ee3154 into main Feb 6, 2025
13 checks passed

RODO94 deleted the rory/add-flow-error branch February 6, 2025 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RODO94 commented Feb 3, 2025

RODO94 Feb 3, 2025

RODO94 Feb 3, 2025

jessicamcinchak Feb 4, 2025

RODO94 Feb 5, 2025

jessicamcinchak Feb 5, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025 •

edited

Loading

jessicamcinchak left a comment

jessicamcinchak Feb 4, 2025

jessicamcinchak Feb 4, 2025

RODO94 Feb 5, 2025

jessicamcinchak Feb 5, 2025

jessicamcinchak Feb 4, 2025

RODO94 Feb 5, 2025

RODO94 Feb 5, 2025

jessicamcinchak Feb 5, 2025

jessicamcinchak left a comment

jessicamcinchak Feb 5, 2025

jessicamcinchak Feb 5, 2025

RODO94 commented Feb 6, 2025 •

edited

Loading

jessicamcinchak commented Feb 6, 2025

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

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

Conversation

RODO94 commented Feb 3, 2025

Where does this PR come from?

What has been added?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicamcinchak Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 3, 2025 • edited Loading

Pizza

jessicamcinchak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessicamcinchak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RODO94 commented Feb 6, 2025 • edited Loading

jessicamcinchak commented Feb 6, 2025

jessicamcinchak Feb 5, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025 •

edited

Loading

RODO94 commented Feb 6, 2025 •

edited

Loading