Description
Pedro Amorim
2025-05-14 12:32:56 UTC
Created attachment 182439 [details] [review] Bug 39900: Preparation: Refactor public_query_search_params for reusability Ensure no regressions are introduced, run: prove t/db_dependent/Koha/AdditionalContents.t Created attachment 182440 [details] [review] Bug 39900: Preparation: Move html customizations options to class for reusability Created attachment 182441 [details] [review] Bug 39900: REST API specs and class changes Created attachment 182442 [details] [review] Bug 39900: Add translated_contents embed Created attachment 182443 [details] [review] Bug 39900: Add tests prove t/db_dependent/api/v1/additional_contents.t Created attachment 182887 [details] [review] Bug 39900: Preparation: Refactor public_query_search_params for reusability Ensure no regressions are introduced, run: prove t/db_dependent/Koha/AdditionalContents.t Signed-off-by: David Nind <david@davidnind.com> Created attachment 182888 [details] [review] Bug 39900: Preparation: Move html customizations options to class for reusability Signed-off-by: David Nind <david@davidnind.com> Created attachment 182889 [details] [review] Bug 39900: REST API specs and class changes Signed-off-by: David Nind <david@davidnind.com> Created attachment 182890 [details] [review] Bug 39900: Add translated_contents embed Signed-off-by: David Nind <david@davidnind.com> Created attachment 182891 [details] [review] Bug 39900: Add tests prove t/db_dependent/api/v1/additional_contents.t Signed-off-by: David Nind <david@davidnind.com> I tested by running the tests. If this is insufficient, please change the status back to needs signoff. Testing notes (using KTD): 1. Apply the patches. 2. Rebuild everything (not sure that this step is required): yarn build 3. Ran the tests and these passed: - prove t/db_dependent/Koha/AdditionalContents.t - prove t/db_dependent/api/v1/additional_contents.t Applying: Bug 39900: Preparation: Move html customizations options to class for reusability error: sha1 information is lacking or useless (Koha/AdditionalContents.pm). error: could not build fake ancestor Created attachment 184722 [details] [review] Bug 39900: Preparation: Refactor public_query_search_params for reusability Ensure no regressions are introduced, run: prove t/db_dependent/Koha/AdditionalContents.t Signed-off-by: David Nind <david@davidnind.com> Created attachment 184723 [details] [review] Bug 39900: Preparation: Move html customizations options to class for reusability Signed-off-by: David Nind <david@davidnind.com> Created attachment 184724 [details] [review] Bug 39900: REST API specs and class changes Signed-off-by: David Nind <david@davidnind.com> Created attachment 184725 [details] [review] Bug 39900: Add translated_contents embed Signed-off-by: David Nind <david@davidnind.com> Created attachment 184726 [details] [review] Bug 39900: Add tests prove t/db_dependent/api/v1/additional_contents.t Signed-off-by: David Nind <david@davidnind.com> Looking here Looks good to me. Two questions remaining: [1] The test with the translation embed should imo contain some translation data. Now we are just comparing empty arrays. So add two translations to Localizations. [2] What I am actually missing is the possibility to ask for a specific translation by passing a language code. Now translated_contents just contains everything and the receiver should open all elements to check the language. Changing status to reflect need for feedback. Created attachment 184978 [details] [review] Bug 39900: (QA follow-up): Add translated_contents to tests Also add tests to searching for specific language code Created attachment 184979 [details] [review] Bug 39900: (QA follow-up): Add translated_contents to tests Also add tests to searching for specific language code prove t/db_dependent/api/v1/additional_contents.t Created attachment 185081 [details] [review] Bug 39900: Preparation: Refactor public_query_search_params for reusability Ensure no regressions are introduced, run: prove t/db_dependent/Koha/AdditionalContents.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 185082 [details] [review] Bug 39900: Preparation: Move html customizations options to class for reusability Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 185083 [details] [review] Bug 39900: REST API specs and class changes Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 185084 [details] [review] Bug 39900: Add translated_contents embed Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 185085 [details] [review] Bug 39900: Add tests prove t/db_dependent/api/v1/additional_contents.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 185086 [details] [review] Bug 39900: (QA follow-up): Add translated_contents to tests Also add tests to searching for specific language code prove t/db_dependent/api/v1/additional_contents.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Nice work everyone! Pushed to main for 25.11 Can you please add release notes? There are a few failures due to this bug, I think: xt/api.t is failing: # Failed test 'No tag errors in the spec' # at /kohadevbox/koha/xt/api.t line 128. # Structures begin differing at: # $got->[0] = 'get /public/additional_contents -> uses tag 'additional_contents' not present in top level list' # $expected->[0] = Does not exist get /public/additional_contents -> uses tag 'additional_contents' not present in top level list # Looks like you failed 1 test of 1. not ok 4 - tags tests And also: xt_find_missing_new_lines_at_the_end_of_file_t.api/v1/swagger/paths/additional_contents.yaml should end with a new line (504) Created attachment 185453 [details] [review] Bug 39900: (QA follow-up) Add missing tag description Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 185455 [details] [review] Bug 39900: (follow-up) Add a newline to end of additional_contents.yaml follow-up's pushed to main |