https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32607
--- Comment #6 from Jonathan Druart <[email protected]> --- Quick code review: 1. There are several wrong indentations (2 vs 4 spaces) or mismatch indentation (Koha::REST::V1::ImportSources->get for instance). You should run perltidy on new perl files 2. DB tables should be plural, import_source*s* (like other tables) 3. (naming) It's not clear what is Koha::ImportSource->patron and import_source.patron_id, could we find something more meaningful? 4. Permission check in missing in admin-home.tt 5. Permission description is missing in includes/permissions.inc 6. import-sources.tt * + const RESTOAuth2ClientCredentials = [% IF Koha.Preference('RESTOAuth2ClientCredentials') %]true[% ELSE %]false[% END %]; not needed? * "erm" occurrences need to be adjusted 7. FormKit/Form => You are introducing a change that is not applied to other Vue files, and you are changing the UI, it's not consistent with other area of Koha. I don't think we are ready for that. 8. In ISEdit.vue, processSubmit. The way we are displaying the message is in then(). Same for the other AJAX requests in other components. 9. ISList.vue you should not display an empty table, to be consistent with other tables. 10. the routes: you are deciding to change the URL, admin/import-sources.pl/add We are not having the '.pl' for other vue pages (which require the apache's rewriterule, but I think we need to stay consistent) 11. import-sources.js You are using add/remove/update/getOne when we use create/delete/update/get in other fetch files. 12. In patron-api-client.js you are using "list" when we use "getAll" in ERM 13. About autocomplete, I would not use it. We already have a way to select patrons (in all the different Koha modules, and the same that we reuse in ERM). If we want an autocomplete feature I think we should then use select2 and implement bug 32474. I'd like to add that, as usual, I am not against new ways of doing things, but we need to apply them first to the existing code, and be consistent. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
