Skip to content
This repository was archived by the owner on Dec 22, 2020. It is now read-only.

Fix/111895 event venue separation logic presentation #238

Merged

Conversation

paulmskim
Copy link
Contributor

@paulmskim paulmskim added the status:code-review Issue or pull request is waiting for a Developer to take a second look label Aug 13, 2018
@bordoni bordoni added review:merge Code has been approved and is waiting to be merged and removed status:code-review Issue or pull request is waiting for a Developer to take a second look labels Aug 13, 2018
Copy link
Contributor

@mitogh mitogh left a comment

Choose a reason for hiding this comment

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

Just a minor adjustment on a props call.

} );

// TODO: need to remove the use of "maybe" functions as they hold logic they
// ultimately should not.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be ideal to add in expand on this comment a bit on what's specific set of logic the comment is referring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a general comment for all maybe functions, the logic in each one differs. In this case, it's the maybeRemoveEntry() function which has checks for empty details. We should be doing this check outside the function instead (or simply check for venue id as the id and details should exist together) and call removeEntry(). The maybe obscures the logic and makes the code difficult to follow.

I've created a ticket here for this: https://central.tri.be/issues/112316

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! Great thanks for the heads up just wanted to make sure it's clear in case someone else works on this.

// another block, the venue id persists because of the setInitialState()
// function. This will perform as intended once setInitialState() is
// removed.
this.props.removeVenue();
Copy link
Contributor

Choose a reason for hiding this comment

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

You can call this on the container instead using the prop onBlockRemoved during the map of state to props.

Example:

Copy link
Contributor Author

@paulmskim paulmskim Aug 14, 2018

Choose a reason for hiding this comment

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

This won't work either. As my comment says, the venue id persists from the saved state when the editor was first opened. You can replicated this by doing this:

  • Create a new event or edit an existing event
  • Add a venue block and add a venue or create a new venue.
  • Save the event. Leave the page.
  • Edit the event again. This time, the saved meta should populate the state through the setInitialState() function.
  • Delete the venue block.
  • Add another venue block. The old venue will populate the venue block automatically.

This happens because the saved meta still has the old venue id, and every time the venue block is created, the setInitialState() populates the block's initial state. Because the event was not saved after deleting the block, the database still has the venue id saved in the event meta. This is what populates the venue block again when creating a new one.

We need to deal with removing the setInitialState() function and treat the redux state as the single source of truth rather than fetching information from the database upon block creation.

I'd love to fix this in this ticket, but it's an entirely other issue in itself, so I've created a ticket to do some discovery into removing setInitialState(): https://central.tri.be/issues/112209

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, so maybe this one might be happening because of two reasons.

  1. null is not consider as empty value when saving the attributes values so maybe we should use undefined (doesn't have to be on this ticket)
  2. This might be related to this bug from core. Add hook for block deletion so post meta can be removed WordPress/gutenberg#5626 but can't say for sure.

Copy link
Contributor Author

@paulmskim paulmskim Aug 14, 2018

Choose a reason for hiding this comment

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

removeVenue() sets the venue id to undefined already, not null. But either way, it shouldn't be grabbing a venue from a null id. Once the setInitialState() function is removed and the initial state is set properly, this problem shouldn't persist.

@mitogh mitogh added review:enhance Code needs some changes to be accepted and removed review:merge Code has been approved and is waiting to be merged labels Aug 14, 2018
@mitogh mitogh added review:merge Code has been approved and is waiting to be merged and removed review:enhance Code needs some changes to be accepted labels Aug 14, 2018
@paulmskim paulmskim merged commit 6bb07b5 into master Aug 14, 2018
@paulmskim paulmskim deleted the fix/111895-event-venue-separation-logic-presentation branch August 14, 2018 14:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
review:merge Code has been approved and is waiting to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants