I've adjusted the dependency to put this one on top of the other Vue works/refactoring.

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.

I'm about to start working on 32474 again now that a lot of the ERM refactoring work is done - I'm aiming to pick that up towards the end of this week

Agustin: we agreed on renaming this 'Record sources' as it will be reused with the overlay rules eventually, and we need to leave room for an API-driven cataloguing module, which is not, to be precise, an import source, and we will want to identify it as well.

Some remarks from comment 6 are still valid.

(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

(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

Created attachment 154006 [details] [review]
Bug 32607: Add table and permissions

Adds a new table called import_sources and a user permission called manage_import_sources

Created attachment 154007 [details] [review]
Bug 32607: [DO NOT PUSH] Koha schemas

Created attachment 154008 [details] [review]
Bug 32607: Add CRUD for import sources

This patch adds vue components to create, read, update and delete import sources

To test:
1. apply patches
2. updatedatabase
3. yarn build_js
4. go to Administration -> Import sources
5. play around with the ui and create, update, remove and search import sources
6. Sign off ;)

Created attachment 154009 [details] [review]
Bug 32607: [DO NOT PUSH] Koha schemas

Created attachment 154010 [details] [review]
Bug 32607: (follow-up) Apply changes suggested

1. Atomic update contains SQL error:
ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'SET @saved_cs_client     = @@character_set_client */;

2. There are warnings in the console

[Vue warn]: Data property "leftMenu" is already defined in Props. 
  at <Page left-menu=false title="" > 
  at <Main>

[Vue warn]: Vue received a Component which was made a reactive object. This can lead to unnecessary performance overhead, and should be avoided by marking the component with `markRaw` or using `shallowRef` instead of `ref`

3. It's missing the "loading" when we fetch data. It's what we do in other places to avoid a "blink"

4. I think the breadcrumb is missing "administration" at the beginning.

5. If you create a new source without patron, you can submit the form (even if marked as required) and you get a 500 ('DBD::mysql::st execute failed: Field 'patron_id' doesn't have a default value')

6. CONSTRAINT `record_source_fk_1` FOREIGN KEY (`patron_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE

Do we really want the 'on delete cascade' clause here?

7. Found 2 UI bugs but not related to this, reported on bug 34465 and bug 34466.
Not blocker of course.

8. Why did you name the directory of the components "record-sources" instead of "RecordSources"? Isn't the standard to have CamelCase for component's names (and this top directory is kind of part of the component name).

9. Cancel link is not a pointer.
Cancel link from here
            <a @click="doCancel($event)" class="router-link-active cancel">{{
                $__("Cancel")
            }}</a>

vs Cancel link from AgreementsFormAdd
                    <router-link
                        :to="{ name: 'AgreementsList' }"
                        role="button"
                        class="cancel"
                        >{{ $__("Cancel") }}</router-link
                    >

10.
in the routes file
                    title: "List",
you are missing $__("")

11. You are not using Breadcrumbs but setTitle in Main, why that?

Created attachment 154438 [details] [review]
Bug 32607: (follow-up) Apply new suggestions

Hi Agustín, 

I received this error in the atomic update:

DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_32607.pl  [15:37:26]: Bug 32607 - Add record_sources table
ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Can't create table `koha_kohadev`.`record_sources` (errno: 150 "Foreign key constraint is incorrectly formed") at /kohadevbox/koha/C4/Installer.pm line 741

Created attachment 155682 [details] [review]
Bug 32607: Add table and permissions

This patch adds:

* A new table: `record_sources`.
* A new user permission: `manage_record_sources`.

Record sources will contain a name and might be linked to a patron. This
patron will be the one that has permission to use the assigned token
through API calls. The `api_token` will be generated on Koha side on
store.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 155683 [details] [review]
Bug 32607: Add Koha::RecordSource(s) classes

This patch introduces classes for handling record sources. The
Koha::RecordSource->store method takes care of generating the
`api_token` attribute if required.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/RecordSource.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 155684 [details] [review]
Bug 32607: DBIC schema [DO NOT PUSH]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 155685 [details] [review]
Bug 32607: Add /record_sources endpoints

This patch introduces endpoints for managing record sources. This is
done on top of Koha::RecordSource(s) following the current coding style.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/record_sources.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 155686 [details] [review]
Bug 32607: Add record sources admin page

This patch introduces a Vue.js based record sources managing page. To
test it:

1. Apply this patch
2. Build the Vue.js stuff:
   $ ktd --shell
  k$ yarn js:build
3. On the staff interface, go to Administration > Record sources
4. Play with the interface and the offered actions
=> SUCCESS: Things go well
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Hi, I've just submitted a reworked version of the original patchset from Agustín. I tried to cover Jonathan's concerns even though I'm not (yet) versed on Vue.js.

I aligned everything with our coding style and guidelines and hope the result is what is expected this time.

Comments welcome, Vue.js pointers or follow-ups too.

Thanks everyone!

Did you ... test? Is it working for you?

I am getting a DT error right after I create a new source.

DataTables warning: table id=DataTables_Table_0 - Ajax error. For more information about this error, please see http://datatables.net/tn/7

Also, cypress tests are missing.

There is a 500 in the Koha logs

[WARN] OpenAPI >>> GET api/v1/record_sources [{"message":"Properties not allowed: patron.","path":"\/body\/0"}]

(In reply to Jonathan Druart from comment #30)
> There is a 500 in the Koha logs
> 
> [WARN] OpenAPI >>> GET api/v1/record_sources [{"message":"Properties not
> allowed: patron.","path":"\/body\/0"}]

Looking

(In reply to Jonathan Druart from comment #29)
> Did you ... test? Is it working for you?

Yeah, a lot :-D

> I am getting a DT error right after I create a new source.

Will look.

> Also, cypress tests are missing.

I totally forgot about that. Any ideas on what it should cover?

(In reply to Jonathan Druart from comment #30)
> There is a 500 in the Koha logs
> 
> [WARN] OpenAPI >>> GET api/v1/record_sources [{"message":"Properties not
> allowed: patron.","path":"\/body\/0"}]

I added the `additionalAttributes: false` entry last minute and that's how I introduced the error. Sending a fixed patch.

Created attachment 155958 [details] [review]
Bug 32607: Add /record_sources endpoints

This patch introduces endpoints for managing record sources. This is
done on top of Koha::RecordSource(s) following the current coding style.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/record_sources.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Patches no longer applies 8-(...

kohadev-koha@kohadevbox:koha(bz32607)$ git bz apply 32607

Bug 32607 - Add import sources CRUD

155682 - Bug 32607: Add table and permissions
155683 - Bug 32607: Add Koha::RecordSource(s) classes
155684 - Bug 32607: DBIC schema [DO NOT PUSH]
155686 - Bug 32607: Add record sources admin page
155958 - Bug 32607: Add /record_sources endpoints

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 32607: Add table and permissions
Applying: Bug 32607: Add Koha::RecordSource(s) classes
Applying: Bug 32607: DBIC schema [DO NOT PUSH]
Using index info to reconstruct a base tree...
M	Koha/Schema/Result/Borrower.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/Schema/Result/Borrower.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/Borrower.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 32607: DBIC schema [DO NOT PUSH]

Created attachment 157756 [details] [review]
Bug 32607: Add table and permissions

This patch adds:

* A new table: `record_sources`.
* A new user permission: `manage_record_sources`.

Record sources will contain a name and might be linked to a patron. This
patron will be the one that has permission to use the assigned token
through API calls. The `api_token` will be generated on Koha side on
store.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 157757 [details] [review]
Bug 32607: Add Koha::RecordSource(s) classes

This patch introduces classes for handling record sources. The
Koha::RecordSource->store method takes care of generating the
`api_token` attribute if required.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/RecordSource.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 157758 [details] [review]
Bug 32607: DBIC schema [DO NOT PUSH]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 157759 [details] [review]
Bug 32607: Add /record_sources endpoints

This patch introduces endpoints for managing record sources. This is
done on top of Koha::RecordSource(s) following the current coding style.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/record_sources.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 157760 [details] [review]
Bug 32607: Add record sources admin page

This patch introduces a Vue.js based record sources managing page. To
test it:

1. Apply this patch
2. Build the Vue.js stuff:
   $ ktd --shell
  k$ yarn js:build
3. On the staff interface, go to Administration > Record sources
4. Play with the interface and the offered actions
=> SUCCESS: Things go well
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 157761 [details] [review]
Bug 32607: Add record sources admin page

This patch introduces a Vue.js based record sources managing page. To
test it:

1. Apply this patch
2. Build the Vue.js stuff:
   $ ktd --shell
  k$ yarn js:build
3. On the staff interface, go to Administration > Record sources
4. Play with the interface and the offered actions
=> SUCCESS: Things go well
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

(In reply to David Nind from comment #34)
> Patches no longer applies 8-(...
>
> Applying: Bug 32607: DBIC schema [DO NOT PUSH]

Thanks for trying! That [DO NOT PUSH] patch is usually source for trouble. In most cases you can just skip it when applying, and run the `dbic` alias inside `ktd` to re-generate the changes for testing purposes.

Rebased now.

Created attachment 157762 [details] [review]
Bug 32607: Add cypress tests

(In reply to Tomás Cohen Arazi from comment #41)
> (In reply to David Nind from comment #34)
> > Patches no longer applies 8-(...
> >
> > Applying: Bug 32607: DBIC schema [DO NOT PUSH]
> 
> Thanks for trying! That [DO NOT PUSH] patch is usually source for trouble.
> In most cases you can just skip it when applying, and run the `dbic` alias
> inside `ktd` to re-generate the changes for testing purposes.
> 
> Rebased now.

Thanks Tomás for that tip - have never been quite sure!

I think I've setup things correctly: apply patch, updatedatabase, dbic, yarn js:build, flush_memcached, restart_all

However, when I go to Administration > Catalog > Record sources, I get a 404 page not found error.

URL accessed: http://127.0.0.1:8081/cgi-bin/koha/admin/record-sources

All the tests pass!

(In reply to David Nind from comment #43)
> (In reply to Tomás Cohen Arazi from comment #41)
> > (In reply to David Nind from comment #34)
> > > Patches no longer applies 8-(...
> > >
> > > Applying: Bug 32607: DBIC schema [DO NOT PUSH]
> > 
> > Thanks for trying! That [DO NOT PUSH] patch is usually source for trouble.
> > In most cases you can just skip it when applying, and run the `dbic` alias
> > inside `ktd` to re-generate the changes for testing purposes.
> > 
> > Rebased now.
> 
> Thanks Tomás for that tip - have never been quite sure!
> 
> I think I've setup things correctly: apply patch, updatedatabase, dbic, yarn
> js:build, flush_memcached, restart_all
> 
> However, when I go to Administration > Catalog > Record sources, I get a 404
> page not found error.

You need to reset_all. Because some apache files need to be updated. That's why you get the 404

(In reply to Tomás Cohen Arazi from comment #44)
> 
> You need to reset_all. Because some apache files need to be updated. That's
> why you get the 404

Thanks Tomás - that worked!

Some comments from testing:

1. I'm not sure that I understand what this does, but I assume that it sets the foundation for being able to limit the editing of bibliographic records from specific sources to specific staff patrons. More to come... I'll leave it to someone else to write the release note.

2. Tests pass:
   - prove t/db_dependent/Koha/RecordSource.t
   - prove t/db_dependent/api/v1/record_sources.t

3. Editing a record source - I can't edit Name or User fields (the only two options), so I'm not sure why editing is an option, as you get this error message: "Something went wrong: Error: Read-only."

4. Applying the patch and getting things to work:
   4.1 Apply the patch: use interactive mode and # out the DBIC patch
   4.2 Run updatedatabase
   4.3 Run dbic
   4.4 reset_all

(In reply to David Nind from comment #46)
> Some comments from testing:
> 
> 1. I'm not sure that I understand what this does, but I assume that it sets
> the foundation for being able to limit the editing of bibliographic records
> from specific sources to specific staff patrons. More to come... I'll leave
> it to someone else to write the release note.

Right, this will pair with a new column for the bibliographic metadata table, telling where the record came from. API requests adding/modifying records will need to provide the generated token in the headers to be accepted to be identified as the that particular record source. The patron linking is added so only that patron is allowed to 'represent' that record source (in case of the token being shared, etc).

The idea is to later merge the sources listed in the overlay rules as system-defined sources as well. 

> 2. Tests pass:
>    - prove t/db_dependent/Koha/RecordSource.t
>    - prove t/db_dependent/api/v1/record_sources.t
> 
> 3. Editing a record source - I can't edit Name or User fields (the only two
> options), so I'm not sure why editing is an option, as you get this error
> message: "Something went wrong: Error: Read-only."

This is an error. Will try to take a look ASAP.

> 4. Applying the patch and getting things to work:
>    4.1 Apply the patch: use interactive mode and # out the DBIC patch
>    4.2 Run updatedatabase
>    4.3 Run dbic
>    4.4 reset_all

Thanks for taking your time on this one!

Created attachment 161362 [details] [review]
Bug 32607: DB changes

Created attachment 161363 [details] [review]
Bug 32607: Add Koha::RecordSource(s) classes

Created attachment 161364 [details] [review]
Bug 32607: Add /record_sources endpoints

Created attachment 161365 [details] [review]
Bug 32607: Add record sources admin page

Created attachment 161366 [details] [review]
Bug 32607: DBIC schema [DO NOT PUSH]

Created attachment 161367 [details] [review]
Bug 32607: Add cypress tests

You should rebase on top of bug 35329. It should be quite trivial to use the modal for the patron search. Let me know if you need help.

What are the cypress tests supposed to be testing? The "Edit", "List" and "Delete" tests are all identical and just appear to be checking what the final breadcrumb is rather than the CRUD operations as the "describe" element suggests.

Created attachment 161435 [details] [review]
Bug 32607: DB changes

This patch adds:

* A new table: `record_sources`.
* A new user permission: `manage_record_sources`.
* A FK on record_sources.recour_source_id on the biblio_metadata tables

Record sources will contain a name and (for now) a flag telling if
records from the specific source can be manually edited.

Created attachment 161436 [details] [review]
Bug 32607: Add Koha::RecordSource(s) classes

Created attachment 161437 [details] [review]
Bug 32607: Add /record_sources endpoints

This patch introduces endpoints for managing record sources. This is
done on top of Koha::RecordSource(s) following the current coding style.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/api/v1/record_sources.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Created attachment 161438 [details] [review]
Bug 32607: Add record sources admin page

This patch introduces a Vue.js based record sources managing page. To
test it:

1. Apply this patch
2. Build the Vue.js stuff:
   $ ktd --shell
  k$ yarn js:build
  k$ restart_all
3. On the staff interface, go to Administration > Record sources
4. Play with the interface and the offered actions
=> SUCCESS: Things go well
5. Sign off :-D

Created attachment 161439 [details] [review]
Bug 32607: DBIC schema [DO NOT PUSH]

Created attachment 161440 [details] [review]
Bug 32607: Add cypress tests

Created attachment 161441 [details] [review]
Bug 32607: Add Koha::Biblio::Metadata->can_be_edited

(In reply to Jonathan Druart from comment #54)
> You should rebase on top of bug 35329. It should be quite trivial to use the
> modal for the patron search. Let me know if you need help.

I dropped that feature for now. Too much work, too much time already :-D

Created attachment 161474 [details] [review]
Bug 32607: Add record sources admin page

This patch introduces a Vue.js based record sources managing page. To
test it:

1. Apply this patch
2. Build the Vue.js stuff:
   $ ktd --shell
  k$ yarn js:build
  k$ restart_all
3. On the staff interface, go to Administration > Record sources
4. Play with the interface and the offered actions
=> SUCCESS: Things go well
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

(In reply to Matt Blenkinsop from comment #55)
> What are the cypress tests supposed to be testing? The "Edit", "List" and
> "Delete" tests are all identical and just appear to be checking what the
> final breadcrumb is rather than the CRUD operations as the "describe"
> element suggests.

Yeah, I think I just forgot about them. Sending a real one.

(In reply to Tomás Cohen Arazi from comment #63)
> (In reply to Jonathan Druart from comment #54)
> > You should rebase on top of bug 35329. It should be quite trivial to use the
> > modal for the patron search. Let me know if you need help.
> 
> I dropped that feature for now. Too much work, too much time already :-D

Too bad, it was really easy to implement.

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161404

*** This bug has been marked as a duplicate of bug 35919 ***