Description
Tomás Cohen Arazi (tcohen)
2024-03-20 14:29:17 UTC
+1 that would be handy! Also, tomas forgot to mention, the api may set the record source upon creation of a record (which is great) but there is no way to change it (even with the api) once it's created. Then the only way to change it is through a direct access to the database. In some use-cases it may be needed that a librarian "unlock" a record to let a fellow edit the record. Created attachment 166161 [details] [review] Bug 36372: provide an interface to set the source of a record from the staff interface (In reply to Fridolin Somers from comment #3) > Isn't it a done by Bug 35919 ? it is not, Bug 35919 provides all the logic but no interface. Which this bug does (based on the logic provided by bug 35919). Created attachment 168054 [details] [review] Bug 36372 : trigger a record indexing when the record source is set Created attachment 168195 [details] [review] Bug 36372 : Cypress tests Created attachment 168338 [details] [review] Bug 36372: provide an interface to set the source of a record from the staff interface Test plan : - apply Cypress patch - running cypress patch in ktd will fail - apply both code patches, cypress tests will pass - open any record in the staff interface - with superlibrarian user, click on the "Edit" button group. - you should be able to click on the "Set record source" below the "Edit record button". - you will be given a form to choose between any defined record source. If none defined there would be a link to configure the record sources. - change a users permissions to have cataloguing permissions but without the "set_record_source" permissions. - login as this forementionned user and go to any record view. - the "Set record source" button should now be greyed out and not possible to click.# Created attachment 168339 [details] [review] Bug 36372: trigger a record indexing when the record source is set Created attachment 168340 [details] [review] Bug 36372: Cypress tests Created attachment 168430 [details] [review] Bug 36372: provide an interface to set the source of a record from the staff interface Test plan : - apply Cypress patch - running cypress patch in ktd will fail - apply both code patches, cypress tests will pass - open any record in the staff interface - with superlibrarian user, click on the "Edit" button group. - you should be able to click on the "Set record source" below the "Edit record button". - you will be given a form to choose between any defined record source. If none defined there would be a link to configure the record sources. - change a users permissions to have cataloguing permissions but without the "set_record_source" permissions. - login as this forementionned user and go to any record view. - the "Set record source" button should now be greyed out and not possible to click.# Created attachment 168510 [details] [review] Bug 36372: Cypress tests Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Created attachment 168511 [details] [review] Bug 36372: provide an interface to set the source of a record from the staff interface Test plan : - apply Cypress patch - running cypress patch in ktd will fail - apply both code patches, cypress tests will pass - open any record in the staff interface - with superlibrarian user, click on the "Edit" button group. - you should be able to click on the "Set record source" below the "Edit record button". - you will be given a form to choose between any defined record source. If none defined there would be a link to configure the record sources. - change a users permissions to have cataloguing permissions but without the "set_record_source" permissions. - login as this forementionned user and go to any record view. - the "Set record source" button should now be greyed out and not possible to click.# Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Created attachment 168512 [details] [review] Bug 36372: trigger a record indexing when the record source is set Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Post test remarks: 0) Works as declared. 1) To make cypress test happy for the second run, I had to manually delete Poop source created in the first run. 2) It was necessary to apply interactively "trigger a record indexing when the record source is set" only after "provide an interface to set the source of a record from the staff interface" was already applied. Otherwise an error results. 3) QA scripts unhappy. Created attachment 168897 [details] [review] Bug 36372: Cypress tests Created attachment 168898 [details] [review] Bug 36372: provide an interface to set the source of a record from the staff interface Test plan : - apply Cypress patch - running cypress patch in ktd will fail - apply both code patches, cypress tests will pass - open any record in the staff interface - with superlibrarian user, click on the "Edit" button group. - you should be able to click on the "Set record source" below the "Edit record button". - you will be given a form to choose between any defined record source. If none defined there would be a link to configure the record sources. - change a users permissions to have cataloguing permissions but without the "set_record_source" permissions. - login as this forementionned user and go to any record view. - the "Set record source" button should now be greyed out and not possible to click.# Created attachment 168899 [details] [review] Bug 36372: trigger a record indexing when the record source is set Created attachment 168900 [details] [review] Bug 36372: (QA follow-up) perltidy cataloguing/record_source.pl, fix permissions issues, add html filters to new template# Hi, I have re-uploaded the patch in the correct order + made a followup patch to fix the issues from the qa scripts. Can anyone make signoff again? Created attachment 168902 [details] [review] Bug 36372: (QA follow-up) perltidy cataloguing/record_source.pl, fix permissions issues, add html filters to new template# Arthur, it does not work for me. After clicking the button "Set record source" from /cgi-bin/koha/cataloguing/record_source.pl?biblionumber=111, I'm getting: /cgi-bin/koha/cataloguing/record_source.pl?biblionumber=&record_source_id=4&save=Set+record+source Note, without biblionumber. And in record_source.pl you are not checking if the $biblionumber is present and correct nor if Koha::Biblios->find gave you a valid object. As a result my ktd exploded with: Can't call method "metadata" on an undefined value at /kohadevbox/koha/cataloguing/record_source.pl line 45 After the recent correction of 2024-07-12 13:01 UTC it seems to work. But there is a serious issue with deleting the record source--when you delete a record source already used in some records, it seem delete also biblio_metadata rows linked with that biblio_metadata.record_source_id. The biblio and biblioitems rows remain, but biblio_metadata is gone. Could you confirm it please? Is it universal problem or is it connected only with some local circumstances? Created attachment 169268 [details] [review] Bug 36372: Cypress tests Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Created attachment 169269 [details] [review] Bug 36372: provide an interface to set the source of a record from the staff interface Test plan : - apply Cypress patch - running cypress patch in ktd will fail - apply both code patches, cypress tests will pass - open any record in the staff interface - with superlibrarian user, click on the "Edit" button group. - you should be able to click on the "Set record source" below the "Edit record button". - you will be given a form to choose between any defined record source. If none defined there would be a link to configure the record sources. - change a users permissions to have cataloguing permissions but without the "set_record_source" permissions. - login as this forementionned user and go to any record view. - the "Set record source" button should now be greyed out and not possible to click.# Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Created attachment 169270 [details] [review] Bug 36372: trigger a record indexing when the record source is set Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Created attachment 169271 [details] [review] Bug 36372: (QA follow-up) perltidy cataloguing/record_source.pl, fix permissions issues, add html filters to new template# Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Signed-off, but still some issues with tidiness: WARN cataloguing/record_source.pl WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 2) Arthur, just another remark. It seems that the template cataloguing/record_source.tt was prepared to display the current record source if already set. But it is not shown because the current record source (current_source.record_source_id) is not passed in the template call. The following change would be a possible fix: diff --git a/cataloguing/record_source.pl b/cataloguing/record_source.pl index 1005e1aa1f..61579ef5d6 100755 --- a/cataloguing/record_source.pl +++ b/cataloguing/record_source.pl @@ -62,6 +62,7 @@ my $record_sources = Koha::RecordSources->search(); $template->param( biblio => $biblio, record_sources => $record_sources, + current_source => { record_source_id => $biblio->metadata->record_source_id }, ); Created attachment 169542 [details] [review] Bug 36372: (QA followup) pass current record-source to template while editing Created attachment 169831 [details] [review] Bug 36372: (QA followup) pass current record-source to template while editing Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Created attachment 172369 [details] [review] Bug 36372: Cypress tests Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Created attachment 172370 [details] [review] Bug 36372: provide an interface to set the source of a record from the staff interface Test plan : - apply Cypress patch - running cypress patch in ktd will fail - apply both code patches, cypress tests will pass - open any record in the staff interface - with superlibrarian user, click on the "Edit" button group. - you should be able to click on the "Set record source" below the "Edit record button". - you will be given a form to choose between any defined record source. If none defined there would be a link to configure the record sources. - change a users permissions to have cataloguing permissions but without the "set_record_source" permissions. - login as this forementionned user and go to any record view. - the "Set record source" button should now be greyed out and not possible to click.# Signed-off-by: Janusz Kaczmarek <januszop@gmail.com> Created attachment 172371 [details] [review] Bug 36372: Fix style issues in dropdown Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> A few remarks: * I don't think we should introduce a new set of .pl+.tt this should be handled in a simple modal. * The new page style is weird. Maybe a side-effect of the move to Bootstrap 5, cannot tell. * The form for setting the record source is sending a GET (should be post) * The form for setting the record source is not passing a CSRF token It otherwise works as expected. My follow-up just makes it look the same as the rest of the dropdown (again, bootstrap 5 change side-effect). Created attachment 176743 [details] [review] Bug 36372: (QA follow-up) moving specific .pl and .tt to a modal. Created attachment 176744 [details] [review] Bug 36372: (QA follow-up) fix Cypress tests Hello, It's been a while :) I've made the advised changes and moved everything to a modal and to detail.pl script. Best wishes for 2025, Arthur have a bit of reworking to do on this one (squashing) I will have to squash because after rework, some new pl and tt were removed (made as a modal instead). QA scripts complaining, have to squash and push-force. Created attachment 177467 [details] [review] Bug 36372: Cypress tests Created attachment 177468 [details] [review] Bug 36372: provide an interface to set the source of a record from the staff interface Test plan : - apply Cypress patch - running cypress patch in ktd will fail - apply code patches, cypress tests will pass - open any record in the staff interface - with superlibrarian user, click on the "Edit" button group. - you should be able to click on the "Set record source" below the "Edit record button". - you will be given a form to choose between any defined record source. If none defined there would be a link to configure the record sources. - change a users permissions to have cataloguing permissions but without the "set_record_source" permissions. - login as this forementionned user and go to any record view. - the "Set record source" button should now be greyed out and not possible to click. Squashed done, sorry for squashing but had to do it to make qa scripts happy. Waiting for signoff/qa |