Bug 28959 - virtualshelves.category is really a boolean
Summary: virtualshelves.category is really a boolean
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on:
Blocks: 28965 30925
  Show dependency treegraph
 
Reported: 2021-09-07 11:00 UTC by Tomás Cohen Arazi
Modified: 2022-06-20 16:14 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00


Attachments
Bug 28959: Move category => public (2.59 KB, patch)
2021-09-07 15:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28959: DBIC update (1.84 KB, patch)
2021-09-07 15:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28959: Add virtualshelves.public as a boolean (50.71 KB, patch)
2021-09-07 15:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28959: Move category => public (2.74 KB, patch)
2021-09-07 16:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28959: DBIC update (1.84 KB, patch)
2021-09-07 16:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28959: Add virtualshelves.public as a boolean (50.71 KB, patch)
2021-09-07 16:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
WIP (2.10 KB, patch)
2021-09-07 16:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28959: (follow-up) Adjust all places in which 'category' was used (9.70 KB, patch)
2021-09-08 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28959: (follow-up 2) Adjust more places in which 'category' was used (9.19 KB, patch)
2021-09-11 06:57 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 28959: Move category => public (2.78 KB, patch)
2021-09-11 20:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 28959: DBIC update (1.89 KB, patch)
2021-09-11 20:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 28959: Add virtualshelves.public as a boolean (50.76 KB, patch)
2021-09-11 20:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 28959: (follow-up) Adjust all places in which 'category' was used (9.74 KB, patch)
2021-09-11 20:38 UTC, David Nind
Details | Diff | Splinter Review
Bug 28959: (follow-up 2) Adjust more places in which 'category' was used (9.24 KB, patch)
2021-09-11 20:38 UTC, David Nind
Details | Diff | Splinter Review
Bug 28959: Move category => public (2.93 KB, patch)
2021-09-12 12:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28959: DBIC update (2.03 KB, patch)
2021-09-12 12:01 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28959: Add virtualshelves.public as a boolean (50.91 KB, patch)
2021-09-12 12:01 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28959: (follow-up) Adjust all places in which 'category' was used (9.89 KB, patch)
2021-09-12 12:01 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28959: (follow-up) Adjust more places in which 'category' was used (9.40 KB, patch)
2021-09-12 12:01 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28959: Move category => public (2.93 KB, patch)
2021-10-28 12:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28959: DBIC update (2.03 KB, patch)
2021-10-28 12:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28959: Add virtualshelves.public as a boolean (50.83 KB, patch)
2021-10-28 12:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28959: (follow-up) Adjust all places in which 'category' was used (10.02 KB, patch)
2021-10-28 12:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28959: (follow-up) Adjust more places in which 'category' was used (9.39 KB, patch)
2021-10-28 12:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28959: (follow-up) Adjust more places in which 'category' was used (9.44 KB, patch)
2021-10-28 14:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28959: Fix other cases (6.83 KB, patch)
2021-10-28 14:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28959: Fix other cases (7.26 KB, patch)
2021-10-28 15:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28959: (QA follow-up) One more lost 'category' to 'public' change (765 bytes, patch)
2021-11-01 07:08 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 28959: Fix occurrence in tools/batch_record_modification.pl (1.39 KB, patch)
2021-11-02 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28959: Fix occurrence in tools/batch_delete_records.pl (893 bytes, patch)
2021-11-26 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2021-09-07 11:00:06 UTC
Somehow it got more complicated than required.
Comment 1 Tomás Cohen Arazi 2021-09-07 15:24:02 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2021-09-07 15:24:06 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2021-09-07 15:24:10 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2021-09-07 16:39:41 UTC
Created attachment 124622 [details] [review]
Bug 28959: Move category => public

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2021-09-07 16:39:46 UTC
Created attachment 124623 [details] [review]
Bug 28959: DBIC update
Comment 6 Tomás Cohen Arazi 2021-09-07 16:39:52 UTC
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
Comment 7 Tomás Cohen Arazi 2021-09-07 16:39:58 UTC Comment hidden (obsolete)
Comment 8 Tomás Cohen Arazi 2021-09-08 19:45:38 UTC
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>
Comment 9 David Nind 2021-09-08 20:26:30 UTC
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.
Comment 10 Fridolin Somers 2021-09-11 06:57:46 UTC
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.
Comment 11 Fridolin Somers 2021-09-11 07:00:37 UTC
I've added a followup 2, otherwise seems to work fine.

Maybe we could change in form :
"Category: Private/Public" with "Public: No/Yes"
Comment 12 Marcel de Rooy 2021-09-11 13:51:53 UTC
(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.
Comment 13 David Nind 2021-09-11 20:37:47 UTC
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>
Comment 14 David Nind 2021-09-11 20:37:52 UTC
Created attachment 124800 [details] [review]
Bug 28959: DBIC update

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 David Nind 2021-09-11 20:37:58 UTC
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>
Comment 16 David Nind 2021-09-11 20:38:02 UTC
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>
Comment 17 David Nind 2021-09-11 20:38:08 UTC
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>
Comment 18 Joonas Kylmälä 2021-09-12 11:23:43 UTC
(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.
Comment 19 Joonas Kylmälä 2021-09-12 12:00:59 UTC
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>
Comment 20 Joonas Kylmälä 2021-09-12 12:01:04 UTC
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>
Comment 21 Joonas Kylmälä 2021-09-12 12:01:08 UTC
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>
Comment 22 Joonas Kylmälä 2021-09-12 12:01:14 UTC
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>
Comment 23 Joonas Kylmälä 2021-09-12 12:01:21 UTC
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>
Comment 24 Marcel de Rooy 2021-09-12 14:08:06 UTC
(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 :)
Comment 25 Fridolin Somers 2021-09-14 00:15:15 UTC
(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.
Comment 26 Tomás Cohen Arazi 2021-09-14 00:25:11 UTC
(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.
Comment 27 Jonathan Druart 2021-10-28 12:23:28 UTC
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>
Comment 28 Jonathan Druart 2021-10-28 12:23:33 UTC
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>
Comment 29 Jonathan Druart 2021-10-28 12:23:38 UTC
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>
Comment 30 Jonathan Druart 2021-10-28 12:23:42 UTC
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>
Comment 31 Jonathan Druart 2021-10-28 12:23:47 UTC
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>
Comment 32 Jonathan Druart 2021-10-28 12:25:16 UTC
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
Comment 33 Jonathan Druart 2021-10-28 14:40:25 UTC
(In reply to Jonathan Druart from comment #32)
> Remaining occurrences of category in opac-shelves.pl

I meant opac/opac-shareshelf.pl
Comment 34 Tomás Cohen Arazi 2021-10-28 14:59:23 UTC
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>
Comment 35 Tomás Cohen Arazi 2021-10-28 14:59:28 UTC
Created attachment 127060 [details] [review]
Bug 28959: Fix other cases
Comment 36 Tomás Cohen Arazi 2021-10-28 15:06:19 UTC
Created attachment 127062 [details] [review]
Bug 28959: Fix other cases
Comment 37 Jonathan Druart 2021-10-28 15:45:48 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 38 Andrew Nugged 2021-11-01 07:08:23 UTC
Created attachment 127142 [details] [review]
Bug 28959: (QA follow-up) One more lost 'category' to 'public' change
Comment 39 Jonathan Druart 2021-11-02 10:41:17 UTC
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.
Comment 40 Jonathan Druart 2021-11-02 15:54:38 UTC
Last two patches pushed to master.
Comment 41 Jonathan Druart 2021-11-26 11:15:35 UTC
Created attachment 128036 [details] [review]
Bug 28959: Fix occurrence in tools/batch_delete_records.pl
Comment 42 Jonathan Druart 2021-11-26 11:18:38 UTC
(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.
Comment 43 Marcel de Rooy 2022-06-08 12:08:58 UTC
Did this report create bug 30925, Tomnas ?
Comment 44 Marcel de Rooy 2022-06-20 07:09:26 UTC
(In reply to Marcel de Rooy from comment #43)
> Did this report create bug 30925, Tomnas ?

Tomnas should be Tomas.
Comment 45 Tomás Cohen Arazi 2022-06-20 16:14:31 UTC
(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