Skip to content
This repository has been archived by the owner on Aug 16, 2019. It is now read-only.

12 Fix bug deleting entities #19

Merged
merged 1 commit into from
Jun 1, 2018
Merged

Conversation

mikestone14
Copy link
Member

This commit:

  • Fixes a bug in which entities deleted by the server were not deleted in state.

This happened when the server did not respond with the deleted entity’s ID, which is a likely scenario causing a bug in most cases.

To delete an entity, dispatch the destroy thunk action passing in the entity to delete as the parameter. The entity to delete must have an id key.

closes #12

This commit:

* Fixes a bug in which entities deleted by the server were not deleted in state.

This happened when the server did not respond with the deleted entity’s ID, which is a likely scenario causing a bug in most cases.

To delete an entity, dispatch the `destroy` thunk action passing in the entity to delete as the parameter. The entity to delete must have an `id` key.
const state = mockStore.getState();

expect(destroySuccessAction.payload).toEqual({ data: { id: userStub.id } });
expect(config.reducer(state, destroySuccessAction)).toEqual(store);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is how to test the full reducer/thunk implementation using redux-mock-store. Since the mock store cannot be configured with reducers we need to explicitly call the reducer here.

More info here: reduxjs/redux-mock-store#71 (comment)

@@ -107,6 +107,8 @@ class BaseConfig {
...this._genericActions(TYPES.DESTROY),

Choose a reason for hiding this comment

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

Do we still need this, or is what you added below taking its place?

Copy link
Member Author

Choose a reason for hiding this comment

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

We get a couple additional actions with this, and additions below override the necessary actions.

Choose a reason for hiding this comment

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

oh okay. I figured i was missing something im trying to catch up with whats going on and also review lol

Copy link

@violet-graves violet-graves left a comment

Choose a reason for hiding this comment

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

As far as i can tell this does what it says it does. 👍

@darkmoves
Copy link

I concur with Jacques 👍

@mikestone14 mikestone14 merged commit f20c0dd into master Jun 1, 2018
@mikestone14 mikestone14 deleted the ms/12-fix-bug-deleting-entities branch June 1, 2018 15:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix bug with deleting entities
3 participants