In order to implement bug 31791, a way to defined record sources is needed. This bug replaces bug 32607 (it will be closed) in order to make it easier to track things.
*** Bug 32607 has been marked as a duplicate of this bug. ***
Created attachment 161531 [details] [review] Bug 35919: 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. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 161532 [details] [review] Bug 35919: Add Koha::RecordSource(s) classes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 161533 [details] [review] Bug 35919: 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 161534 [details] [review] Bug 35919: 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>
Created attachment 161535 [details] [review] Bug 35919: DBIC schema [DO NOT PUSH] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 161537 [details] [review] Bug 35919: Add Koha::Biblio::Metadata->can_be_edited Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 161538 [details] [review] Bug 35919: Add cypress tests This patch adds Cypress tests for the `record sources` CRUD as required by QA. I tried to cover all the UI interactions and behaviours. To test: 1. Have all the patches applied 2. Run: $ ktd --shell k$ cypress run --spec t/cypress/integration/Admin/RecordSources_spec.ts => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 161577 [details] [review] Bug 35919: 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. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 161578 [details] [review] Bug 35919: Add Koha::RecordSource(s) classes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 161579 [details] [review] Bug 35919: 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> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 161580 [details] [review] Bug 35919: 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> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 161581 [details] [review] Bug 35919: DBIC schema [DO NOT PUSH] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 161582 [details] [review] Bug 35919: Add Koha::Biblio::Metadata->can_be_edited prove t/db_dependent/Koha/Biblio/Metadata.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 161583 [details] [review] Bug 35919: Add cypress tests This patch adds Cypress tests for the `record sources` CRUD as required by QA. I tried to cover all the UI interactions and behaviours. To test: 1. Have all the patches applied 2. Run: $ ktd --shell k$ cypress run --spec t/cypress/integration/Admin/RecordSources_spec.ts => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
The UI works as advertised and the unit tests pass. I also found the unit test for t/db_dependent/Koha/Biblio/Metadata.t which I've added to one of the commit messages
(In reply to Tomás Cohen Arazi from comment #0) > This bug replaces bug 32607 (it will be closed) in order to make it easier > to track things. Easier by hiding the history?...
(In reply to Jonathan Druart from comment #17) > (In reply to Tomás Cohen Arazi from comment #0) > > This bug replaces bug 32607 (it will be closed) in order to make it easier > > to track things. > > Easier by hiding the history?... - Some generalizations got dropped. - Some generalizations were taken care of on separate bugs already in master. - Some features were simplified to make this easier to match the client's MVP, and future enhancements will be dealt with on follow-up bugs in which pertinent discussions can take place in a better way. I think that's the best way to do it.
Created attachment 162489 [details] [review] Bug 35919: Add superlibrarian permission to Permissions.t Permissions.t was failing when running a jenkins build to another branch with this bug applied. Permissions.t was missing the permission for record sources and is now added in this patch Test plan: 1) prove t/Koha/Auth/Permissions.t
Created attachment 162594 [details] [review] Bug 35919: Fix label for 'can be edited' checkbox
Created attachment 162595 [details] [review] Bug 35919: Fix edit route to match existing ones
Created attachment 162596 [details] [review] Bug 35919: Adjust RewriteRule to match existing ones
Created attachment 162599 [details] [review] Bug 35919: Adjust RewriteRule and routes to match existing ones
I don't understand why you stubborn and obstinate to do things differently. I have said that 10x on bug 32607, then here again. 1. computed title instead of the logic in template 2. retrieving the client api from setup() 3. _has_value should not be needed, we have form validation already 4. you should clean the api-client, why do you deal with headers and json stringify? we don't need it in other places! 5. in the routes: BeforeUnloadEvent() { window.location.href = "/cgi-bin/koha/admin/admin-home.pl"; }, Why? 6. Why do you name the add/edit component "Edit" when all others are named "AddForm"? 7. Why calling the client api this way: .then(() => something()) when all existing occurrences do: .then( success => something(), error => {}, ); 8.Edit.vue Object.keys(response).forEach(key => { this.row[key] = response[key] }) What's the point of this?? Some are blocker, some are weirdness, others are better than what exist but different. I let you decide the status of this.
Also wondering if the routes should not have a more specific names: "Edit", "List", could clash by other modules.
(In reply to Jonathan Druart from comment #24) > I don't understand why you stubborn and obstinate to do things differently. > I have said that 10x on bug 32607, then here again. Maybe because some things didn't exist when this was submitted in the first place? Or maybe some things were rewritten by the time this got (finally) some eyes on it. I really thought I had covered all the remaining things. I removed all of the contentious snippets. I will try to understand your concerns. Vue is not my whellhouse. This is the first time I work with Vue, trying to help someone else's dev to move forward. And even learnt Cypress and wrote the darn tests for a trivial CRUD page! > 1. computed title instead of the logic in template > 2. retrieving the client api from setup() > 3. _has_value should not be needed, we have form validation already > 4. you should clean the api-client, why do you deal with headers and json > stringify? we don't need it in other places! > 5. in the routes: > BeforeUnloadEvent() { > window.location.href = "/cgi-bin/koha/admin/admin-home.pl"; > }, > Why? > 6. Why do you name the add/edit component "Edit" when all others are named > "AddForm"? > > 7. Why calling the client api this way: > .then(() => something()) > when all existing occurrences do: > .then( > success => something(), > error => {}, > ); > 8.Edit.vue > Object.keys(response).forEach(key => { > this.row[key] = response[key] > }) > What's the point of this?? > > Some are blocker, some are weirdness, others are better than what exist but > different. > > I let you decide the status of this.
Just ask if you have questions or need help!
If no one beats me to it, will be looking here "soon"
(In reply to Marcel de Rooy from comment #28) > If no one beats me to it, will be looking here "soon" I will try to address Jonathan's QA concerns.
Created attachment 163348 [details] [review] Bug 35919: 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. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 163349 [details] [review] Bug 35919: Add Koha::RecordSource(s) classes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 163350 [details] [review] Bug 35919: 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> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 163351 [details] [review] Bug 35919: 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> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 163352 [details] [review] Bug 35919: DBIC schema [DO NOT PUSH] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 163353 [details] [review] Bug 35919: Add Koha::Biblio::Metadata->can_be_edited prove t/db_dependent/Koha/Biblio/Metadata.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 163354 [details] [review] Bug 35919: Add cypress tests This patch adds Cypress tests for the `record sources` CRUD as required by QA. I tried to cover all the UI interactions and behaviours. To test: 1. Have all the patches applied 2. Run: $ ktd --shell k$ cypress run --spec t/cypress/integration/Admin/RecordSources_spec.ts => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Created attachment 163355 [details] [review] Bug 35919: Add superlibrarian permission to Permissions.t Permissions.t was failing when running a jenkins build to another branch with this bug applied. Permissions.t was missing the permission for record sources and is now added in this patch Test plan: 1) prove t/Koha/Auth/Permissions.t
Created attachment 163360 [details] [review] Bug 35919: Add superlibrarian permission to Permissions.t Permissions.t was failing when running a jenkins build to another branch with this bug applied. Permissions.t was missing the permission for record sources and is now added in this patch Test plan: 1) prove t/Koha/Auth/Permissions.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Marking NSO again, as I rewrote most of the Vue part of this dev. Some things I'm not comfortable with, but happy to follow the current standard practice for now, and evolve later. I kept the URL rewrite rule we originally submitted. I think requiring the trailing slash (/) if my rewrite avoids it, doesn't make sense. I can submit a bug for redoing the rest of the rewrites this way if required by QA (if this applies, as I'm not familiar with the little details the other modules might have making them requiring this). I also liked it more when the API client was instantiated globally instead on each place it is used. As it is a stateless object it should cause no issues. I moved it to match the current practice and will file a separate bug to restore the original implementation and discuss it in place. Thanks everyone!
Created attachment 163369 [details] [review] Bug 35919: Add Koha::Biblio::Metadata->source_allows_editing This patch adds a convenient method that tells if the record is editable, according to it's record source. To test: 1. Apply this patch 2. Run: $ ktd k$ prove t/db_dependent/Koha/Biblio/Metadata.t => SUCCESS: Tests pass! Use cases covered! 3. Sign off :-D Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Bug 31791: Rename can_be_edited -> source_allows_editing
1. Create a record source, edit it Modify the name h1 is getting modified. I don't think it's expected.
Created attachment 163389 [details] [review] Bug 35919: Adjust h1
Created attachment 163390 [details] [review] Bug 35919: Fix cypress tests And remove "remove", "delete" inconsistency
Created attachment 163391 [details] [review] Bug 35919: 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. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 163392 [details] [review] Bug 35919: Add Koha::RecordSource(s) classes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 163393 [details] [review] Bug 35919: 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> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 163394 [details] [review] Bug 35919: 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> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 163395 [details] [review] Bug 35919: DBIC schema [DO NOT PUSH] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 163396 [details] [review] Bug 35919: Add cypress tests This patch adds Cypress tests for the `record sources` CRUD as required by QA. I tried to cover all the UI interactions and behaviours. To test: 1. Have all the patches applied 2. Run: $ ktd --shell k$ cypress run --spec t/cypress/integration/Admin/RecordSources_spec.ts => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 163397 [details] [review] Bug 35919: Add superlibrarian permission to Permissions.t Permissions.t was failing when running a jenkins build to another branch with this bug applied. Permissions.t was missing the permission for record sources and is now added in this patch Test plan: 1) prove t/Koha/Auth/Permissions.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 163398 [details] [review] Bug 35919: Add Koha::Biblio::Metadata->source_allows_editing This patch adds a convenient method that tells if the record is editable, according to it's record source. To test: 1. Apply this patch 2. Run: $ ktd k$ prove t/db_dependent/Koha/Biblio/Metadata.t => SUCCESS: Tests pass! Use cases covered! 3. Sign off :-D Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Bug 31791: Rename can_be_edited -> source_allows_editing Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 163399 [details] [review] Bug 35919: Adjust h1 Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 163400 [details] [review] Bug 35919: Fix cypress tests And remove "remove", "delete" inconsistency Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment on attachment 163399 [details] [review] Bug 35919: Adjust h1 Review of attachment 163399 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/js/vue/components/Admin/RecordSources/FormAdd.vue @@ +5,5 @@ > + {{ > + $__("Edit record source #%s").format( > + record_source.record_source_id > + ) > + }} I get the change from the name to the ID for consistency. But I would like to know how you end up with that coding style so I do it right next time. I used whatever the editor suggested, and then used the prettify tool blindly so far.
If it's not already, we should document how to setup editors for these prettier styling requirements.
I am just using git commit and the pre-commit hook is doing its thing (ie. calling prettier). What should be done for you, as you should have the hook setup as well.
It's actually splitting because the line is longer I guess.
(In reply to Jonathan Druart from comment #57) > It's actually splitting because the line is longer I guess. +1 that's it
(In reply to Tomás Cohen Arazi from comment #39) > Marking NSO again, as I rewrote most of the Vue part of this dev. > > Some things I'm not comfortable with, but happy to follow the current > standard practice for now, and evolve later. > > I kept the URL rewrite rule we originally submitted. I think requiring the > trailing slash (/) if my rewrite avoids it, doesn't make sense. > > I can submit a bug for redoing the rest of the rewrites this way if required > by QA (if this applies, as I'm not familiar with the little details the > other modules might have making them requiring this). > > I also liked it more when the API client was instantiated globally instead > on each place it is used. As it is a stateless object it should cause no > issues. I moved it to match the current practice and will file a separate > bug to restore the original implementation and discuss it in place. > > Thanks everyone! Did you open the bugs?
Created attachment 164708 [details] [review] Bug 35919: 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. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 164709 [details] [review] Bug 35919: Add Koha::RecordSource(s) classes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 164710 [details] [review] Bug 35919: 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> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 164711 [details] [review] Bug 35919: 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> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 164712 [details] [review] Bug 35919: DBIC schema [DO NOT PUSH] Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 164713 [details] [review] Bug 35919: Add cypress tests This patch adds Cypress tests for the `record sources` CRUD as required by QA. I tried to cover all the UI interactions and behaviours. To test: 1. Have all the patches applied 2. Run: $ ktd --shell k$ cypress run --spec t/cypress/integration/Admin/RecordSources_spec.ts => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 164714 [details] [review] Bug 35919: Add superlibrarian permission to Permissions.t Permissions.t was failing when running a jenkins build to another branch with this bug applied. Permissions.t was missing the permission for record sources and is now added in this patch Test plan: 1) prove t/Koha/Auth/Permissions.t Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 164715 [details] [review] Bug 35919: Add Koha::Biblio::Metadata->source_allows_editing This patch adds a convenient method that tells if the record is editable, according to it's record source. To test: 1. Apply this patch 2. Run: $ ktd k$ prove t/db_dependent/Koha/Biblio/Metadata.t => SUCCESS: Tests pass! Use cases covered! 3. Sign off :-D Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Bug 31791: Rename can_be_edited -> source_allows_editing Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 164716 [details] [review] Bug 35919: Adjust h1 Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 164717 [details] [review] Bug 35919: Fix cypress tests And remove "remove", "delete" inconsistency Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I am sorry, some very small issues, but also a bigger one I couldn't get around (5): 1) Page title We since changed the markup there for better translatability among other things. Please adapt: +<title> + Record sources › Koha +</title> 2) Missing permissions.inc Guess everyone has only been testing as a superlibrarian again? ;) Wanted to fix in a follow-up, but got stuck on 5) :( 3) Reconsider sequence on admin page Currently this shows as the last entry below the Elasticsearch config, but I feel it would be better suited with the matching/overlay rules, what do you think? 4) Missing from side navigation The left hand admin navigation panel was missed. Sequence should match admin page, so good to do 3) first :) 5) Apache config? When trying to access the admin/record_sources I have a 404 error. I tried yarn build, restart_all and reset_all. I figure this line here might not be right? RewriteRule ^/cgi-bin/koha/admin/record_sources(.*)?$ /cgi-bin/koha/admin/record_sources.pl$1 [PT] I verified it showed in /etc/koha/apache-shared-intranet.conf
Created attachment 165274 [details] [review] Bug 35919: (QA follow-up) Address QA concerns This patch: * Adds the missing entry in permissions.inc * Moves the configuration entry above 'Record overlay rules' * Adds the 'Record sources' link to the left navigation column, in the right position. * Makes the <title> element be constructed the same way the 'cities' page is. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Katrin Fischer from comment #70) > 1) Page title > > We since changed the markup there for better translatability among other > things. Please adapt: > > +<title> > + Record sources › Koha > +</title> Fixed. It would be great if it was documented somewhere or there was something in the guidelines about this. I copied what's done in the other Vue apps as requested by QA, and they don't implement this currently. Done anyway. > 2) Missing permissions.inc > > Guess everyone has only been testing as a superlibrarian again? ;) > > Wanted to fix in a follow-up, but got stuck on 5) :( Fixed. > 3) Reconsider sequence on admin page > > Currently this shows as the last entry below the Elasticsearch config, but I > feel it would be better suited with the matching/overlay rules, what do you > think? I agree. Done. > 4) Missing from side navigation > > The left hand admin navigation panel was missed. Sequence should match admin > page, so good to do 3) first :) Right, good catch. Done. Preserving the order discussed above. > 5) Apache config? > > When trying to access the admin/record_sources I have a 404 error. > > I tried yarn build, restart_all and reset_all. I'm not sure what's going on on your KTD, but I was playing with 'main' and just: ```shell git bz apply 35919 ktd --shell reset-all ``` and the record sources page is there.
> 5) Apache config? > > When trying to access the admin/record_sources I have a 404 error. I applied the patch, did a restart_all, and it looks to be working for me!
Pushed for 24.05! Well done everyone, thank you!
Not backported to 23.11.x
I did my best to understand and describe this new feature (along with bug 31791). Please see the merge request and let me know if I what I wrote is wrong! https://gitlab.com/koha-community/koha-manual/-/merge_requests/887