[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 #14 from Jonathan Druart <jonathan.druart+koha at gmail.com> ---
(In reply to Agustín Moyano from comment #13)

A global response would be (and sorry if I am rambling here) that we enforce
consistency.
We must not have 2 ways to do the same thing.
If you want to add something new to the Vue code in Koha, it must be done
globally. Otherwise we end up with divergences.
We cannot allow a module that would do something differently.

> > 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?

I am bad at naming, keep patron_id for now.
(Is this correct to have 1-N instead of N-N then?)

> > 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.

Yes, of course it would be nice to have such mechanisms to ease writing of our
form.
But as said previously this new code should not introduce something new it
would be the only one to use.
First we adapt our code, then we use it in new module. In your position it
would be easier to stick to the exist patterns.

> > 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

Yes ofc, I know. But still the same answer, it's a different pattern...

> > 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

See debian/templates/apache-shared-intranet.conf:RewriteRule
^/cgi-bin/koha/erm/.*$ /cgi-bin/koha/erm/erm.pl [PT]

> > 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()

Yes, that's not a problem, we have it for patron as well, see
components/ERM/UserRoles.vue

    const client = APIClient.patron
    // FIXME We are missing a "loading..."
    client.patrons.get(selected_patron_id).then(p => {

We can imagine to have something related to patron but that is not "patrons"
(like the attributes for instance).
Stick to the existing patterns please ;)

> 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

Yes I agree, we could rethink the verbs. But that's for another bug.

> > 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.

Yes, we discussed that recently on #koha.
We should have an autocomplete on select2 (with infinite scrolling) to search
for patrons.
And a "more options" link that would open the new window (that should be a
modal).
But that is for all the different patron searches in Koha, and so that's a big
change, out of the scope here. You should stick to the existing patterns.

> > 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

Yes, that's all my point! :D

-- 
You are receiving this mail because:
You are watching all bug changes.