Somehow it got more complicated than required.
Created attachment 124619 [details] [review] Bug 28959: Move category => public Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124620 [details] [review] Bug 28959: DBIC update
Created attachment 124621 [details] [review] Bug 28959: Add virtualshelves.public as a boolean This patchset moves the 'category' attribute for virtual shelves, that takes values of 1 and 2 (private and public respectively) into a boolean for public. The DBRev is trivial, and the changes to the code are as well. To test: 1. have some known public and private lists 2. Apply this patches 3. Run: $ updatedatabase => SUCCESS: Public lists have public=1, private have public=0 4. Run: $ kshell k$ prove t/db_dependent/Utils/Datatables_Virtualshelves.t \ t/db_dependent/Virtualshelves.t => SUCCESS: Tests pass! 5. Try the feature in staff and OPAC => SUCCESS: All good 6. Sign off :-D
Created attachment 124622 [details] [review] Bug 28959: Move category => public Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124623 [details] [review] Bug 28959: DBIC update
Created attachment 124624 [details] [review] Bug 28959: Add virtualshelves.public as a boolean This patchset moves the 'category' attribute for virtual shelves, that takes values of 1 and 2 (private and public respectively) into a boolean for public. The DBRev is trivial, and the changes to the code are as well. To test: 1. have some known public and private lists 2. Apply this patches 3. Run: $ updatedatabase => SUCCESS: Public lists have public=1, private have public=0 4. Run: $ kshell k$ prove t/db_dependent/Utils/Datatables_Virtualshelves.t \ t/db_dependent/Virtualshelves.t => SUCCESS: Tests pass! 5. Try the feature in staff and OPAC => SUCCESS: All good 6. Sign off :-D
Created attachment 124625 [details] [review] WIP
Created attachment 124663 [details] [review] Bug 28959: (follow-up) Adjust all places in which 'category' was used Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
The tests pass. However, there is a change to the way lists are displayed, and I can't easily add items from a search result: 1. Private lists appear under both 'Your lists' and 'Public lists' in the drop down list when you attempt to add an item(s) to a list from search results. 2. You can't see the public lists either. I created three lists: - LISTA = private - LISTB = public (owner only able to make changes) - LISTC = public (anyone seeing can make changes) After applying the patch I have updated the database, flush_memcached, and cleared cached for browser. Now in the staff interface when doing a search and attempting to add items to lists (search > bar at the top with 'Add to list') I only see: Your lists LISTA Public lists LISTA I can't see any of the public lists to add things to. I can create a new list (LISTD) (either private or public) but I still only see the private lists to add things to, and then the private list appears under both headings: Your lists LISTD LISTA Public lists LISTD LISTA The same thing happens in the OPAC. Also, I'm not sure what the sort order for lists should be, but A-Z would seem logical to me. In the OPAC I can see the public lists from Lists > Your lists, then change to the Public lists tab. In the staff interface I can see the public lists from Home > Lists > Public lists tab.
Created attachment 124788 [details] [review] Bug 28959: (follow-up 2) Adjust more places in which 'category' was used Some links still had category. I propose we use explicit public=1 and public=0 when links to public/private are side by side. Otherwise whe just use /cgi-bin/koha/opac-shelves.pl?op=list using default value 0.
I've added a followup 2, otherwise seems to work fine. Maybe we could change in form : "Category: Private/Public" with "Public: No/Yes"
(In reply to Tomás Cohen Arazi from comment #0) > Somehow it got more complicated than required. This report oversimplifies things? Private lists include shared lists too. Saying public Y/N implicitly may include the wrong message.
Created attachment 124799 [details] [review] Bug 28959: Move category => public Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124800 [details] [review] Bug 28959: DBIC update Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124801 [details] [review] Bug 28959: Add virtualshelves.public as a boolean This patchset moves the 'category' attribute for virtual shelves, that takes values of 1 and 2 (private and public respectively) into a boolean for public. The DBRev is trivial, and the changes to the code are as well. To test: 1. have some known public and private lists 2. Apply this patches 3. Run: $ updatedatabase => SUCCESS: Public lists have public=1, private have public=0 4. Run: $ kshell k$ prove t/db_dependent/Utils/Datatables_Virtualshelves.t \ t/db_dependent/Virtualshelves.t => SUCCESS: Tests pass! 5. Try the feature in staff and OPAC => SUCCESS: All good 6. Sign off :-D Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124802 [details] [review] Bug 28959: (follow-up) Adjust all places in which 'category' was used Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 124803 [details] [review] Bug 28959: (follow-up 2) Adjust more places in which 'category' was used Some links still had category. I propose we use explicit public=1 and public=0 when links to public/private are side by side. Otherwise whe just use /cgi-bin/koha/opac-shelves.pl?op=list using default value 0. Signed-off-by: David Nind <david@davidnind.com>
(In reply to Marcel de Rooy from comment #12) > (In reply to Tomás Cohen Arazi from comment #0) > > Somehow it got more complicated than required. > > This report oversimplifies things? > Private lists include shared lists too. > Saying public Y/N implicitly may include the wrong message. This patch does not alter the message we are giving out, we already say before this patch explicitly in the OPAC GUI whether a list is Public or not. So I wouldn't take that into consideration whether to accept this patch or not. If we want to use another naming it can be done even after this patch, this patch doesn't make it any harder as far as I can tell.
Created attachment 124807 [details] [review] Bug 28959: Move category => public Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 124808 [details] [review] Bug 28959: DBIC update Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 124809 [details] [review] Bug 28959: Add virtualshelves.public as a boolean This patchset moves the 'category' attribute for virtual shelves, that takes values of 1 and 2 (private and public respectively) into a boolean for public. The DBRev is trivial, and the changes to the code are as well. To test: 1. have some known public and private lists 2. Apply this patches 3. Run: $ updatedatabase => SUCCESS: Public lists have public=1, private have public=0 4. Run: $ kshell k$ prove t/db_dependent/Utils/Datatables_Virtualshelves.t \ t/db_dependent/Virtualshelves.t => SUCCESS: Tests pass! 5. Try the feature in staff and OPAC => SUCCESS: All good 6. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 124810 [details] [review] Bug 28959: (follow-up) Adjust all places in which 'category' was used Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 124811 [details] [review] Bug 28959: (follow-up) Adjust more places in which 'category' was used Some links still had category. I propose we use explicit public=1 and public=0 when links to public/private are side by side. Otherwise whe just use /cgi-bin/koha/opac-shelves.pl?op=list using default value 0. Signed-off-by: David Nind <david@davidnind.com> JK: Adjust commit title Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
(In reply to Joonas Kylmälä from comment #18) > This patch does not alter the message we are giving out, we already say > before this patch explicitly in the OPAC GUI whether a list is Public or > not. So I wouldn't take that into consideration whether to accept this patch > or not. If we want to use another naming it can be done even after this > patch, this patch doesn't make it any harder as far as I can tell. So, we implicitly already have three categories. And some requests show that people are still shaking the tree. Might extending categories solve that? Not sure. But we choose to put that aside here and make it a boolean. I have no strong arguments against it but just have the intuitive feeling that this may not be the right direction :)
(In reply to Marcel de Rooy from comment #24) > (In reply to Joonas Kylmälä from comment #18) > > > This patch does not alter the message we are giving out, we already say > > before this patch explicitly in the OPAC GUI whether a list is Public or > > not. So I wouldn't take that into consideration whether to accept this patch > > or not. If we want to use another naming it can be done even after this > > patch, this patch doesn't make it any harder as far as I can tell. > > So, we implicitly already have three categories. And some requests show that > people are still shaking the tree. Might extending categories solve that? > Not sure. But we choose to put that aside here and make it a boolean. I have > no strong arguments against it but just have the intuitive feeling that this > may not be the right direction :) I also had the feeling we could keep a place for a third value. But the need here is for API routes to know if datas can be open to the public or not. That is binary.
(In reply to Fridolin Somers from comment #25) > (In reply to Marcel de Rooy from comment #24) > > (In reply to Joonas Kylmälä from comment #18) > > > > > This patch does not alter the message we are giving out, we already say > > > before this patch explicitly in the OPAC GUI whether a list is Public or > > > not. So I wouldn't take that into consideration whether to accept this patch > > > or not. If we want to use another naming it can be done even after this > > > patch, this patch doesn't make it any harder as far as I can tell. > > > > So, we implicitly already have three categories. And some requests show that > > people are still shaking the tree. Might extending categories solve that? > > Not sure. But we choose to put that aside here and make it a boolean. I have > > no strong arguments against it but just have the intuitive feeling that this > > may not be the right direction :) > > I also had the feeling we could keep a place for a third value. > But the need here is for API routes to know if datas can be open to the > public or not. That is binary. Isn't the fact that a list is shared just another boolean? What if we decided to make lists shareable with specific patrons or categories? We need stop doing things implicitly.
Created attachment 127046 [details] [review] Bug 28959: Move category => public Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 127047 [details] [review] Bug 28959: DBIC update Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 127048 [details] [review] Bug 28959: Add virtualshelves.public as a boolean This patchset moves the 'category' attribute for virtual shelves, that takes values of 1 and 2 (private and public respectively) into a boolean for public. The DBRev is trivial, and the changes to the code are as well. To test: 1. have some known public and private lists 2. Apply this patches 3. Run: $ updatedatabase => SUCCESS: Public lists have public=1, private have public=0 4. Run: $ kshell k$ prove t/db_dependent/Utils/Datatables_Virtualshelves.t \ t/db_dependent/Virtualshelves.t => SUCCESS: Tests pass! 5. Try the feature in staff and OPAC => SUCCESS: All good 6. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 127049 [details] [review] Bug 28959: (follow-up) Adjust all places in which 'category' was used Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 127050 [details] [review] Bug 28959: (follow-up) Adjust more places in which 'category' was used Some links still had category. I propose we use explicit public=1 and public=0 when links to public/private are side by side. Otherwise whe just use /cgi-bin/koha/opac-shelves.pl?op=list using default value 0. Signed-off-by: David Nind <david@davidnind.com> JK: Adjust commit title Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Remaining occurrences of category in opac-shelves.pl I am getting a 500 when sharing a list The method Koha::Virtualshelf->category is not covered by tests! Koha::Object::AUTOLOAD('Koha::Virtualshelf=HASH(0x560389011140)') called at /kohadevbox/koha/opac/opac-shareshelf.pl line 84
(In reply to Jonathan Druart from comment #32) > Remaining occurrences of category in opac-shelves.pl I meant opac/opac-shareshelf.pl
Created attachment 127059 [details] [review] Bug 28959: (follow-up) Adjust more places in which 'category' was used Some links still had category. I propose we use explicit public=1 and public=0 when links to public/private are side by side. Otherwise whe just use /cgi-bin/koha/opac-shelves.pl?op=list using default value 0. Signed-off-by: David Nind <david@davidnind.com> JK: Adjust commit title Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 127060 [details] [review] Bug 28959: Fix other cases
Created attachment 127062 [details] [review] Bug 28959: Fix other cases
Pushed to master for 21.11, thanks to everybody involved!
Created attachment 127142 [details] [review] Bug 28959: (QA follow-up) One more lost 'category' to 'public' change
Created attachment 127206 [details] [review] Bug 28959: Fix occurrence in tools/batch_record_modification.pl No method count found for Koha::Virtualshelves DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'category' in 'where clause' at /kohadevbox/koha/Koha/Objects.pm line 601 at /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/tools/batch_record_modification.tt line 80.
Last two patches pushed to master.
Created attachment 128036 [details] [review] Bug 28959: Fix occurrence in tools/batch_delete_records.pl
(In reply to Jonathan Druart from comment #41) > Created attachment 128036 [details] [review] [review] > Bug 28959: Fix occurrence in tools/batch_delete_records.pl Patch pushed to master.
Did this report create bug 30925, Tomnas ?
(In reply to Marcel de Rooy from comment #43) > Did this report create bug 30925, Tomnas ? Tomnas should be Tomas.
(In reply to Marcel de Rooy from comment #44) > (In reply to Marcel de Rooy from comment #43) > > Did this report create bug 30925, Tomnas ? > > Tomnas should be Tomas. FQA