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

clearHandlerCache when remote method added or disabled #4011

Conversation

MohammedEssehemy
Copy link
Contributor

@MohammedEssehemy MohammedEssehemy commented Sep 24, 2018

Description

fixes issue that Handler Cache won't change when remote method is added or disabled.

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide

@slnode
Copy link

slnode commented Sep 24, 2018

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@MohammedEssehemy MohammedEssehemy force-pushed the fix/clear-cache-handler-after-method-add branch 5 times, most recently from 7aa7191 to 14f00f8 Compare September 24, 2018 21:35
@bajtos bajtos self-assigned this Sep 25, 2018
@bajtos
Copy link
Member

bajtos commented Sep 25, 2018

@slnode ok to test

@bajtos
Copy link
Member

bajtos commented Sep 25, 2018

@MohammedEssehemy thank you for the pull request. The change looks good, could you please add tests to verify the fix and prevent regressions in the future?

See the following two commits for inspiration: b466bb9, b466bb9

@MohammedEssehemy
Copy link
Contributor Author

MohammedEssehemy commented Sep 25, 2018

@bajtos I found that these test were changed in this commit f43273e and this is the available form now in master.

so should this be tested in app.test.js such as rest rebuild after add model test
or in rest.middleware.test.js such as rest rebuild after delete model test

I think it's more relevant to be in rest.middleware.test.js, so I can refactor rest rebuild after add model test to be in rest.middleware.test.js to have the following 4 tests in sequence
1- updates REST API when a new model is added.
2- updates REST API when a model is deleted.
3- updates REST API when a remote method is added.
4- updates REST API when a remote method is disabled.

@bajtos what do you think?

@bajtos
Copy link
Member

bajtos commented Sep 25, 2018

so should this be tested in app.test.js such as rest rebuild after add model test
or in rest.middleware.test.js such as rest rebuild after delete model test

Either way is fine with me.

I think it's more relevant to be in rest.middleware.test.js, so I can refactor rest rebuild after add model test to be in rest.middleware.test.js to have the following 4 tests in sequence
1- updates REST API when a new model is added.
2- updates REST API when a model is deleted.
3- updates REST API when a remote method is added.
4- updates REST API when a remote method is disabled.

Sounds great 👍 Thank you!

@MohammedEssehemy
Copy link
Contributor Author

Okay, I'll implement this tonight.

@MohammedEssehemy
Copy link
Contributor Author

@bajtos Done.

@MohammedEssehemy MohammedEssehemy changed the title clearHandlerCache when remote method added or deleted clearHandlerCache when remote method added or disabled Sep 25, 2018
@MohammedEssehemy
Copy link
Contributor Author

@bajtos any updates?

@MohammedEssehemy
Copy link
Contributor Author

@bajtos still waiting.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Sorry for the delay 🙈

The changes look great now!

@bajtos bajtos force-pushed the fix/clear-cache-handler-after-method-add branch from 8806d43 to e33d10f Compare October 9, 2018 13:04
@bajtos
Copy link
Member

bajtos commented Oct 9, 2018

I squashed your two commits into a single one, let's wait for CI results before landing.

@bajtos bajtos merged commit 0488cb7 into strongloop:master Oct 9, 2018
@bajtos
Copy link
Member

bajtos commented Oct 9, 2018

Landed 🎉 Thank you for your first contribution! ❤️

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.

3 participants