[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 #13 from Agustín Moyano <agustinmoyano at 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.