obuchmann / odoo-jsonrpc

PHP Odoo Json-RPC connector, prepared for laravel integration
MIT License
34 stars 11 forks source link

Add count method to complement the search method #6

Closed xewl closed 2 years ago

xewl commented 2 years ago

closes #5

obuchmann commented 2 years ago

Thank you for your PR. Could you please add a test in the OdooTest class.

xewl commented 2 years ago

Things I notice:

Odoo Request classes like Search, SearchRead expect a Domain even though the wrapper methods for them seem to have them nullable.

Seems like there's also more tests to add, outside of this one, even though Count can perfectly be done with the Search Request class on its own, as long as a default/empty Domain is provided instead of null.

By this logic, I also adjusted the ObjectEndpoint class, so the following methods also null coalesce a default empty Domain object, like the model method does, and their Request classes and the Odoo API respectively expect:

I renamed 4 tests that are actually testing the Model way, to include "Model"

and added 4 new ones:

These however may need to be split to separate files, as the layer of their use is different (Direct calls vs Eloquent-like).


> ./vendor/bin/phpunit
PHPUnit 9.5.11 by Sebastian Bergmann and contributors.

......................................SS....                      44 / 44 (100%)

Time: 00:37.249, Memory: 10.00 MB

OK, but incomplete, skipped, or risky tests!
Tests: 44, Assertions: 72, Skipped: 2.
obuchmann commented 2 years ago

Hi @xewl, i'm sorry, i have been very busy and had no time to review.

Your PR looks very good! Good point with splitting the tests, we should consider this in a new PR.

Thank you for your contribution!

xewl commented 2 years ago

Happy NY @obuchmann No worries, glad you found the time eventually :D Thanks for the merge & version bump!