Email: Sending allowed even with missing button URL

richardolsson commented 6 days ago

Description

In production, one email was sent while the buttons had no links. This should not be possible. If a user creates a button but does not add a URL there will be a small yellow warning triangle next to the button in the outline. But there should also be an error message in the "delivery" popper, which I guess in some (or all?) cases doesn't work since someone managed to get past it.

Steps to reproduce

Go to https://app.dev.zetkin.org/organize/1/projects
Go to any project (or create a new one)
Create a new email
Define any target group
Lock the targets
Go to "Compose"
Under "settings", add a subject line
Add a button with some text, but do not define a URL
Click "Delivery"

Expected Behaviour

Because the button is not fully defined (it has no URL) delivery should be prevented. There should be an error message in the delivery box saying that the email can't be scheduled or sen yet.

Actual Behaviour

The normal delivery popper shows up with a scheduling UI. There's nothing preventing the user from sending the email even though the buttons have no URLs.

Screenshots (if you have any)

richardolsson commented 6 days ago

@ziggabyte: I thought we built something for this, but maybe in the end we didn't? Do you have any idea if this is an existing feature that is malfunctioning, or the lack of a feature that should be added?

a-jaxell commented 4 days ago

I think I have discovered the cause for this bug. There seems to be a mixup between what enum to use in the blockProblems() function.

I've started attempting to solve this but If someone could explain what their individual purpose is could understand and get to a possible solution quicker :)

<src/features/emails/types.ts>

export enum BLOCK_TYPES {
  BUTTON = 'button',
  HEADER = 'header',
  LIBRARY_IMAGE = 'libraryImage',
  PARAGRAPH = 'paragraph',
}

export enum BlockKind {
  BUTTON = 'button',
  HEADER = 'header',
  IMAGE = 'image',
  PARAGRAPH = 'paragraph',
}

richardolsson commented 4 days ago

I've started attempting to solve this but If someone could explain what their individual purpose is could understand and get to a possible solution quicker :)

Great question! Judging by how it's used in the code linked below, it looks like BLOCK_TYPES contains the types that can be used inside Editor.js (the library we use for the editor), while BlockKind contains the types that are used in the data that is submitted to the Zetkin API.

https://github.com/zetkin/app.zetkin.org/blob/112995d21421a6ecd1f3bd1d41619a616addebcf/src/features/emails/utils/editorjsBlocksToZetkinBlocks.ts#L12-L21

I would be surprised if this problem is because of an enum mixup. That doesn't mean you're wrong (I could be wrong about this!) but because the bug has to do with buttons, and both enums have the same value for BUTTON (the string 'button') a comparison like BLOCK_TYPES.BUTTON == BlockKind.BUTTON should be true, unless I'm mistaken. I'm pretty sure Typescript complain if the values were incompatible.

I think you're looking in the right part of the code (blockProblems and deliveryBlockProblems), but I just want to point out if it wasn't clear in the issue that the warning triangle is showing, so that should mean that blockProblems() is working as it should.

Again, don't take my word for it because I might be wrong in one or several of my assumptions. I just want to add some more information as you continue to look for the issue.

zetkin / app.zetkin.org