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

[WIP] Change HTTP PATH method by PUT on UPDATE_MANY action #2382

Merged
merged 1 commit into from
Nov 29, 2018

Conversation

Mclovinn
Copy link
Contributor

@Mclovinn Mclovinn commented Oct 2, 2018

  • Fix HTTP method
  • Check regresion
  • Run prettier

See #2372

@Mclovinn
Copy link
Contributor Author

Mclovinn commented Oct 2, 2018

@Kmaschta maybe you can help me.
How can I test my little change? Any idea?

@Kmaschta
Copy link
Contributor

Kmaschta commented Oct 2, 2018

This package doesn't have unit tests. Yet!
We use jest for unit tests. It is already configured and you just have to run it with make test-unit-watch.
To test this package, you have to create a new file index.spec.js next to the index.js you edited.
There is a lot of these files, you can take them as examples.

Data providers like ra-data-simple-rest are basically a function which takes an httpClient and return a closure containing a big switch to build a request.

For your case, in order to test your change, you'll have to:

  • mock the HTTP client
  • call the data provider with the correct fixtures simpleRestProvider('url', mockedHttpClient)(UPDATE_MANY, ...)
  • test that mocked HTTP client is correctly called with the correct arguments, including the method PUT

Copy link
Contributor

@kfern kfern left a comment

Choose a reason for hiding this comment

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

Hi! Could it be configurable to use PUT or PATCH?

@fzaninotto
Copy link
Member

As explained in #2372, this is not just a fix, it's a potential BC break. That being said, I'm willing to accept it on a minor release since it's only a BC break for people who replicated (imperfectly) the API of JSONRestServer instead of using their lib.

However, this needs to be done on the next branch rather than on master - even a minor BC beak is a no-go for a bugfix release. Can you please rebase on next?

@fzaninotto fzaninotto merged commit 97f10d5 into marmelab:master Nov 29, 2018
@fzaninotto fzaninotto added this to the 2.5.0 milestone Nov 29, 2018
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.

4 participants