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

fix: call execPostHooks on internalTransaction #161

Merged

Conversation

BasKiers
Copy link
Contributor

@BasKiers BasKiers commented May 2, 2019

I noticed that when using an internalTransaction (the default behaviour when no transaction is given) for the update method the transaction.execPostHooks function is never called.
I fixed this by adding a line to the internalTransaction check and appending errors to entity[ERR_HOOKS]

I initially tried to add a test but found it non-trivial to mock the transaction used in the update method.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 97.284% when pulling 7b74b80 on BasKiers:fix/update-internal-transaction-execPostHooks into 349ffe9 on sebelga:master.

@sebelga
Copy link
Owner

sebelga commented May 3, 2019

Hi,

Thanks a lot for this! I must say that I am surprised to not find any reference to the symbol gstore.ERR_HOOKS in the code... I might have got lost in a refactor.

For the test, I would probably have gone the "integration" path as it seems indeed very complex to stub the internal transaction. For that, you need to launch an instance of Redis (on the default port) and then in parallel launch the local datastore with yarn local-datastore or npm run local-datastore. Then, to execute those tests it is

yarn test --int
# or
npm run test -- --int

If it's too complex, don't worry and let me know. We can merge this PR and I will add the tests in a second pass.

@BasKiers
Copy link
Contributor Author

Hi,

I am not planning to write tests for this PR due to the added complexity of creating integration tests.

@sebelga
Copy link
Owner

sebelga commented May 20, 2019

HI,
Sorry for the delay, I was out for a week. Ok no worries, I will merge the PR and see if I can add some tests.
Thanks again for fixing this!

@sebelga sebelga merged commit 7b132cf into sebelga:master May 20, 2019
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.

None yet

3 participants