[Koha-bugs] [Bug 32607] Add import sources CRUD

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32607

--- Comment #6 from Jonathan Druart <jonathan.druart+koha at gmail.com> ---
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.