Description
Tomás Cohen Arazi (tcohen)
2019-10-07 18:53:32 UTC
Created attachment 93831 [details] [review] Bug 23770: Add Koha::Object(s)->to_api method Generic method that changes the attribute names of an object in the presence of the to_api_mapping method. It otherwise falls back to returning the TO_JSON output. This is WIP submitted early for scrutiny. Tests and example usage in an API controller coming. +1 Thanks Tomás. I do think that's the way we should go. (In reply to Josef Moravec from comment #2) > +1 > > Thanks Tomás. > > I do think that's the way we should go. Thanks for the feedback! Created attachment 93881 [details] [review] Bug 23770: Add Koha::Object(s)->to_api method Generic method that changes the attribute names of an object in the presence of the to_api_mapping method. It otherwise falls back to returning the TO_JSON output. This is WIP submitted early for scrutiny. Tests and example usage in an API controller coming. Created attachment 93882 [details] [review] Bug 23770: Add to_api_mapping method to Koha::City Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 93883 [details] [review] Bug 23770: Unit tests This patch introduces tests for the new to_api method introduced in Koha::Object(s). It uses Koha::City as a sample for simplicity. And it also uses Koha::Illrequests to test the case in which there's no to_api_mapping method defined on the class, to highlight that a fallback to calling TO_JSON is done. [1] [1] This is done under the assumption Illrequests controller code doesn't use any kind of mapping as the rest of the API controllers do, so there's little chance it would be added in a future. To test: 1. Apply this patchset 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object.t \ t/db_dependent/Koha/Objects.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Sponsored-by: ByWater Solutions Created attachment 93888 [details] [review] Bug 23770: (follow-up) Add tests for Koha::Objects->TO_JSON Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 93890 [details] [review] Bug 23770: (follow-up) Use ->to_api in /cities This patch aims to prove the behaviour does not differ. So you can run the tests for the endpoints, and the behaviour remains. The mappings and methods cannot be removed until $c->objects->search is fixed to make use of them as well. That will happen once all used Koha::Object-based classes implement the mappings. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/cities.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 93891 [details] [review] Bug 23770: Add Koha::Object(s)->to_api method Generic method that changes the attribute names of an object in the presence of the to_api_mapping method. It otherwise falls back to returning the TO_JSON output. This is WIP submitted early for scrutiny. Tests and example usage in an API controller coming. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 93892 [details] [review] Bug 23770: Add to_api_mapping method to Koha::City Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 93893 [details] [review] Bug 23770: Unit tests This patch introduces tests for the new to_api method introduced in Koha::Object(s). It uses Koha::City as a sample for simplicity. And it also uses Koha::Illrequests to test the case in which there's no to_api_mapping method defined on the class, to highlight that a fallback to calling TO_JSON is done. [1] [1] This is done under the assumption Illrequests controller code doesn't use any kind of mapping as the rest of the API controllers do, so there's little chance it would be added in a future. To test: 1. Apply this patchset 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object.t \ t/db_dependent/Koha/Objects.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Sponsored-by: ByWater Solutions Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 93894 [details] [review] Bug 23770: (follow-up) Add tests for Koha::Objects->TO_JSON Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 93895 [details] [review] Bug 23770: (follow-up) Use ->to_api in /cities This patch aims to prove the behaviour does not differ. So you can run the tests for the endpoints, and the behaviour remains. The mappings and methods cannot be removed until $c->objects->search is fixed to make use of them as well. That will happen once all used Koha::Object-based classes implement the mappings. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/cities.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> QAing Depending on strict mode I have two results of the Object.t test: not ok 1 - Exception thrown correctly # Failed test 'Exception thrown correctly' # at t/db_dependent/Koha/Object.t line 411. # expecting: Koha::Exceptions::Object::BadValue # found: normal exit # Looks like you failed 1 test of 1. not ok 16 - Bad value tests # Failed test 'Exception thrown correctly' # at t/db_dependent/Koha/Object.t line 411. # expecting: Koha::Exceptions::Object::BadValue # found: DBIx::Class::Exception (DBIx::Class::Storage::DBI::_dbh_execute(): Incorrect datetime value: 'wrong_value' for column `koha_master`.`borrowers`.`lastseen` at row 1 at /usr/share/koha/Koha/Object.pm line 156 # ) # Looks like you failed 1 test of 1. Somehow does not meet: elsif( $_->{msg} =~ /Incorrect (?<type>\w+) value: '(?<value>.*)' for column '(?<property>\w+)'/ ) { Looks great to me! What's next? * Can we now remove ->_to_api (and so $to_api_mappinp) from Koha/REST/V1/Cities.pm * What about ->_to_model (and so $to_model_mapping)? (In reply to Jonathan Druart from comment #17) > Looks great to me! > > * Can we now remove ->_to_api (and so $to_api_mappinp) from > Koha/REST/V1/Cities.pm we still can't. > * What about ->_to_model (and so $to_model_mapping)? I was thinking of: Koha::Object->new_from_api Koha::Object->update_from_api > What's next? The next step is to file a bug in which we do the same for the other controllers. And then another one for implementing those two methods I propose. And then another one depending on these, for changing Koha::REST::Plugins::Objects so it uses the internal methods instead of expecting the to_api and to_model params. (In reply to Tomás Cohen Arazi from comment #18) > (In reply to Jonathan Druart from comment #17) > > Looks great to me! > > > > * Can we now remove ->_to_api (and so $to_api_mappinp) from > > Koha/REST/V1/Cities.pm > > we still can't. Why? That seems to be the very next step to me :) (In reply to Jonathan Druart from comment #19) > (In reply to Tomás Cohen Arazi from comment #18) > > (In reply to Jonathan Druart from comment #17) > > > Looks great to me! > > > > > > * Can we now remove ->_to_api (and so $to_api_mappinp) from > > > Koha/REST/V1/Cities.pm > > > > we still can't. > > Why? That seems to be the very next step to me :) Yes, yes. But we can't until we adapt Koha::REST::Plugin::Objects->search or provide an alternative implementation (maybe in Koha::Objects->search_from_api) (In reply to Marcel de Rooy from comment #15) > Depending on strict mode I have two results of the Object.t test: > > not ok 1 - Exception thrown correctly > # Failed test 'Exception thrown correctly' > # at t/db_dependent/Koha/Object.t line 411. > # expecting: Koha::Exceptions::Object::BadValue > # found: normal exit > # Looks like you failed 1 test of 1. > not ok 16 - Bad value tests > > # Failed test 'Exception thrown correctly' > # at t/db_dependent/Koha/Object.t line 411. > # expecting: Koha::Exceptions::Object::BadValue > # found: DBIx::Class::Exception > (DBIx::Class::Storage::DBI::_dbh_execute(): Incorrect datetime value: > 'wrong_value' for column `koha_master`.`borrowers`.`lastseen` at row 1 at > /usr/share/koha/Koha/Object.pm line 156 > # ) > # Looks like you failed 1 test of 1. Ok, but that's not something related to this bug, you just found that if we manage not to run the tests in strict mode on a dev env (i.e. using perl instead of prove) the exception is not thrown, fair enough. I'm setting this one back to Signed-off. (In reply to Tomás Cohen Arazi from comment #21) > Ok, but that's not something related to this bug, you just found that if we > manage not to run the tests in strict mode on a dev env (i.e. using perl > instead of prove) the exception is not thrown, fair enough. > > I'm setting this one back to Signed-off. Hmm. Did you misread my comment? I tried both modes. And I see two failures. Please confirm. (In reply to Marcel de Rooy from comment #22) > (In reply to Tomás Cohen Arazi from comment #21) > > Ok, but that's not something related to this bug, you just found that if we > > manage not to run the tests in strict mode on a dev env (i.e. using perl > > instead of prove) the exception is not thrown, fair enough. > > > > I'm setting this one back to Signed-off. > > Hmm. Did you misread my comment? I tried both modes. And I see two failures. > Please confirm. I confirm I get the failure on master :) (In reply to Marcel de Rooy from comment #22) > (In reply to Tomás Cohen Arazi from comment #21) > > Ok, but that's not something related to this bug, you just found that if we > > manage not to run the tests in strict mode on a dev env (i.e. using perl > > instead of prove) the exception is not thrown, fair enough. > > > > I'm setting this one back to Signed-off. > > Hmm. Did you misread my comment? I tried both modes. And I see two failures. > Please confirm. I only managed to see failures when I had sql_strict_modes disabled and run the tests using 'perl' instead of 'prove'. I set it back to SO because the failure I found was not related to the patchset. Will re-check. Using koha-testing-docker here. See bug 23825. I get the failure using prove. Created attachment 94396 [details] [review] Bug 23770: Add Koha::Object(s)->to_api method Generic method that changes the attribute names of an object in the presence of the to_api_mapping method. It otherwise falls back to returning the TO_JSON output. This is WIP submitted early for scrutiny. Tests and example usage in an API controller coming. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 94397 [details] [review] Bug 23770: Add to_api_mapping method to Koha::City Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 94398 [details] [review] Bug 23770: Unit tests This patch introduces tests for the new to_api method introduced in Koha::Object(s). It uses Koha::City as a sample for simplicity. And it also uses Koha::Illrequests to test the case in which there's no to_api_mapping method defined on the class, to highlight that a fallback to calling TO_JSON is done. [1] [1] This is done under the assumption Illrequests controller code doesn't use any kind of mapping as the rest of the API controllers do, so there's little chance it would be added in a future. To test: 1. Apply this patchset 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object.t \ t/db_dependent/Koha/Objects.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Sponsored-by: ByWater Solutions Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 94399 [details] [review] Bug 23770: (follow-up) Add tests for Koha::Objects->TO_JSON Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 94400 [details] [review] Bug 23770: (follow-up) Use ->to_api in /cities This patch aims to prove the behaviour does not differ. So you can run the tests for the endpoints, and the behaviour remains. The mappings and methods cannot be removed until $c->objects->search is fixed to make use of them as well. That will happen once all used Koha::Object-based classes implement the mappings. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/cities.t => SUCCESS: Tests pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Tomás Cohen Arazi from comment #24) > I set it back to SO because the failure I found was not related to the > patchset. Will re-check. Using koha-testing-docker here. Although I can cause a failure with prove too, I will not block this patch set since it seems that the regex in sub store should be improved and we opened a report for that. Nice work! Pushed to master for 19.11.00 |