[Koha-bugs] [Bug 14544] Move the list related code to Koha::Virtualshelves

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14544

--- Comment #172 from Jonathan Druart <jonathan.druart at bugs.koha-community.org> ---
(In reply to Marcel de Rooy from comment #171)
> QA Comment:
> Great job !!! Lot of work involved.

Thanks a lot for Qaing this!
Very useful feedback :)

> Still have some points for consideration:
> 
> OPAC -- View contents of a list -- Send shelf - Crash
> Can't call method "can_be_viewed" on an undefined value at
> /home/koha/testclone/opac/opac-sendshelf.pl line 55.
> *BLOCKER*

Fixed!
The shelfnumber param was not passed.
I have also add a test in the pl file to avoid this crash when viewing an
non-existing shelf (now display 'not authorized', which is the current
behavior).

> OPAC - View contents of a list - Sort by callnumber - Bang
> DBIx::Class::ResultSet::next(): Unknown column 'itemcallnumber' in 'order
> clause' at /home/koha/testclone/opac/opac-shelves.pl line 236
> *BLOCKER*

Fixed! But... This is a bad one.
In opac-shelves.pl and shelves.pl, the join was only done on biblio.
But itemcallnumber is in the items table.
I had to add the biblio<=>biblioitems<=>items relationship to the Schema and a
PK on virtualshelfcontents table
(DBIx::Class::Storage::DBIHacks::_adjust_select_args_for_complex_prefetch():
Unable to perform complex limited prefetch off Virtualshelfcontent without
declared primary key).
Fixed in a separate patch. Note that bug 14819 is stuck in the NSO status but
also adds the relationships.

> OPAC - Lists- Open a shared list where you do not have permission to remove
> items of someone else. Try to remove such an item. No success, but no
> warning too! IIRC There was a warning in the past about some items not
> deleted..
> Now try to remove a item you added yourself to that list while having
> permission to remove your own entries. This does not work ! No items are
> deleted, no message.
> I am inclined to see this last point as a *BLOCKER* too.

Fixed!
The "op" was "view" instead of "remove_biblios" on the submit button.
Note that the individual links work (and intranet is ok).

> OPAC - View list - Delete your own item while the perms are ADD or DDD.
> Should not be possible. But the owner is able to now. This is change of
> behavior and not consistent with the wording of those permissions "Do not
> allow anyone to remove his own contributed entries" (including your own!). I
> would view it as a sort of Readonly list
> where you cannot accidentally delete your own items. (Note however that
> adding is still possible.) 
> I am inclined to see this last point as a *BLOCKER* too (but I may not be
> completely neutral here :)
> +                    # FIXME
> +                    # This does not make sense, but it's has been
> backported from DelFromShelf.
> +                    # Why shouldn't we allow to remove his own contribution
> if allow_delete_other is on?

Well, not fixed.
I have asked on #koha and Nicole agreed
(http://irc.koha-community.org/koha/2015-08-18#i_1716925).
So I am not sure what to do here.

> Code Exceptions.pm module comes in now with a dependency Exception::Class.
> The description => "poeut" still needs some attention apparently :) Not sure
> what it means btw..
> Although this looks good to me on itself, this dev should probably better
> have gone on its own report. Note there is another report too (13995).
> It kind of creeps in now.

poeut is a typo for pouet which does not mean anything :)
The exceptions implementation can be modified easily later (the question was:
should we put all exceptions in 1 file, all exceptions of a given module in 1
file or 1 exception per file).

> OPAC - Share a list - Enter email address - Return to your lists: This list
> does not exist ??  op=view && category=1. Adjust the URL? 

Fixed!
Yes, the op is 'list', not 'view".

> OPAC - I shared a list with AAD perms. The other user could add a few
> entries. After that removed the share.
> Now come back as owner of the list and try to remove one item of yourself
> and one of the other user.
> You will get the message: The item has been removed from the list. 
> This is not completely true: You could only remove one item; the other is
> not permitted. 

This is a specific case and I would prefer to manage it in a separate bug
report.
I am not sure this work on master :)

> OPAC - Create a new list - Starts with permissions D D D - Would it not be
> better to default to D A D ?
> Note that if I create a list with save to new list, it does default to DAD.
> So this is a minor inconsistency..

Yes, and it's a regression.
Fixed on both interface (I have c/p OPAC code for the intranet).

> OPAC - View a list: For a list with ADD or DDD perms, the button Remove
> could be hidden or disabled? Might be the case now too btw.

Not easily, and there is no regression here: it's how it works currently.
The problem is that the queries done in remove_biblios should be executed for
each biblio in the list.
Maybe not a big deal, to investigate...

> OPAC Detail - List(s) this item appears in: [nothing filled while expected] 
> I click from view a (private) list on one entry. So go to opac detail.
> Should I expect that list to be mentioned now? It is not.

Fixed!
In the template shelves->count should be check, not only the shelves variable
(which is always defined).

> Staff - View public lists [Related to another report]
> With the delete public lists permission I can delete the public list of
> another user. OK The staff user probably has a good reason to do so. 
> Just noting that it would perhaps be more friendly to switch it to Private
> instead of a rigourous Delete?

Todo later yes.

> Code in opac-addbybiblionumber.pl: HandleSelectedShelf: Empty TODO in an
> else branch. Noting for the record. 

Yes, I preferred to add the TODO marked than nothing, but no regression here.

> Code  t/db_dependent/Utils/Datatables_Virtualshelves.t and
> C4/Utils/DataTables/VirtualShelves.pm
> This is only justified/used by svc/virtualshelves/search
> Isn't this the time to move this svc script to Koha::Virtualshelves too ? Or
> at least on another report ? 

Not ready yet. The rewrite should use the REST api.

> Database design: Do we still need (or use) virtualshelfcontents.flags  ?
> Could a dbrev remove it now?

To investigate, but yes, I am not sure to have found any usefulness to this
column.

> An observation (which could be considered as outside the scope): Add an item
> to the list with a barcode (in staff) is somewhat strange. You cannot add
> items, only biblios. This is current situation already. Just for
> completeness :)

Changing the wording could maybe fix the ambiguity.

-- 
You are receiving this mail because:
You are watching all bug changes.