https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32607 --- Comment #13 from Agustín Moyano <agustinmoyano@theke.io> --- (In reply to Jonathan Druart from comment #6)
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
Will check that out
2. DB tables should be plural, import_source*s* (like other tables)
Thanks, will rename things as "record source", and will take this into account
3. (naming) It's not clear what is Koha::ImportSource->patron and import_source.patron_id, could we find something more meaningful?
It is the patron account that is allowed to create records with this source.. I'm not sure what could we call it.. Koha::RecordSource->allowed_patron and record_sources.allowed_patron_id?
4. Permission check in missing in admin-home.tt 5. Permission description is missing in includes/permissions.inc
Ok, will check both of these things
6. import-sources.tt * + const RESTOAuth2ClientCredentials = [% IF Koha.Preference('RESTOAuth2ClientCredentials') %]true[% ELSE %]false[% END %]; not needed?
not anymore.. will delete that, thanks
* "erm" occurrences need to be adjusted
copy/paste leftovers.. sorry
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.
I believe Formkit or another form library it's a good idea, and we are in a good time, in that we are at the very beginning of using Vue. I believe it can help a lot with the maintainability of the components. For example, if we apply a library that creates forms from a yaml or json definition, theoretically we could be able to reduce ERM components to 4: a Main component, a List component, a Form component and a Show component, and what get's displayed and the actions could be just parameters that are passed or picked up by those components. If your concern is that it's not implemented in other vue files, we can make other bugs to implement them. But if it is about the UI, css classes can always be modified.
8. In ISEdit.vue, processSubmit. The way we are displaying the message is in then(). Same for the other AJAX requests in other components.
It is implemented in the "then" part.. you see, in Javascript if you are in an async function doing client.agreements.update(agreement, agreement_id).then( success => { setMessage(this.$__("Agreement updated")) this.$router.push({ name: "AgreementsList" }) }, error => { setError(error) } ) Is exactly the same as doing try { const success = await client.agreements.update(agreement, agreement_id) setMessage(this.$__("Agreement updated")) this.$router.push({ name: "AgreementsList" }) } catch(error) { setError(error) } In ISEdit.vue the setMessage is after an "await" statement, so it is in the "then" part. Generally it is recomended to use async/await to improve readability
9. ISList.vue you should not display an empty table, to be consistent with other tables.
Ok, I'll check that
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)
I forgot about this part, and am not sure how to implement this, but definitely will take a look
11. import-sources.js You are using add/remove/update/getOne when we use create/delete/update/get in other fetch files.
I cannot use create/delete/update/get because http-client implement those same methods.. one way would be to add a new level like return { record_source: { get: () .... create: () ... update: () ... delete: () ... } } but it's kind of redundant since I don't have many things to fetch as in ERM.. and I would end up calling the actions like record_source.record_source.get() The other way would be to rename the methods in http-client to _create/_delete/_update/_get and there would be no collision in the names
12. In patron-api-client.js you are using "list" when we use "getAll" in ERM
Ups, I did not realize that... "list" seems to me as a more intuitive name for it, but have absolutely no problem in changing the name
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 did not know about that way to select patrons, but it involves using a popup window. I would not recommend that, as it may get blocked by the browser. I will definitely check select2, but as we are using Vue, I would try to minimize as much as possible the use of jQuery and jQuery components.
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.
I have no problem implementing new features to the existing code, but I think it better to be done in another bug so things don't get messed up Thanks again for checking this out -- You are receiving this mail because: You are watching all bug changes.