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

Convert ZetkinBlocks to editorJsBlocks to make DeliveryErrors handle properly #2267

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

a-jaxell
Copy link
Collaborator

Description

When investigating #2263 I found that content blocks in deliveryProblems.ts was passed to
blockProblems.ts without being converted to match the proper data type used.

This resulted in blockProblems() returning an empty array because it could not find any type properties when
kind properties were actually present.

Screenshots

blockbug.mov

Changes

  • Adds a conversion of blocks in deliveryProblems.ts

Notes to reviewer

Set as draft until tests are passing locally

My tests are failing locally atm, and I am not sure why at the moment.
When logging output in the test blockProblems() returns an empty array.

  1. Go to http://localhost:3000/organize/1/projects/35/emails
  2. Create edit any email
  3. Add a button
  4. Click Delivery - Content Error should be displayed
  5. Fill in text and link on button
  6. Click Delivery - Content Error should not be displayed

Related issues

Resolves #2263

@a-jaxell a-jaxell marked this pull request as draft October 17, 2024 07:04
@richardolsson
Copy link
Member

Well spotted @a-jaxell! As you've seen, the tests are failing. In the future, I would recommend using TDD for issues like these, where there are simple tests.

When you find a bug like this, you would first create a test that reproduces it, and then write a fix for that test. In most cases, arriving at a functional fix will be faster, and once you consider that by the time you're done you already have a test written, the time saved will be even bigger.

@a-jaxell
Copy link
Collaborator Author

Thanks for the input, I'll give it a go as soon as I'm able. 🙂

@ziggabyte ziggabyte marked this pull request as ready for review December 2, 2024 15:54
@ziggabyte
Copy link
Contributor

Some investigation led me to see that when I created the tests for this function and written them, I'd mistaken the datatypes and written tests with data in the wrong shape. Have updated the test now!
The problem you discovered with the wrong type of the data that was parsed from JSON was the key to the mystery @a-jaxell !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Email: Sending allowed even with missing button URL
3 participants