Bug 6279 - Can't delete lists from the second page of lists in the OPAC
Summary: Can't delete lists from the second page of lists in the OPAC
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: 3.20
Hardware: All All
: P5 - low major (vote)
Assignee: Galen Charlton
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-28 12:09 UTC by Nicole C. Engard
Modified: 2016-06-21 21:37 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed patch (3.49 KB, patch)
2013-02-22 17:12 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 6279: Cannot delete lists from second page of lists in the OPAC (3.20 KB, patch)
2014-09-25 15:28 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6279: Follow-up for pagination and confirm (12.65 KB, patch)
2014-09-26 10:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6279: Cannot delete lists from second page of lists in the OPAC (3.22 KB, patch)
2014-09-26 10:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6279: Follow-up for pagination and confirm (12.67 KB, patch)
2014-09-26 10:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6279: Cannot delete lists from second page of lists in the OPAC (3.28 KB, patch)
2014-09-26 14:38 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 6279: Follow-up for pagination and confirm (12.73 KB, patch)
2014-09-26 14:39 UTC, Fridolin SOMERS
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2011-04-28 12:09:36 UTC
When you're on the second page of lists and try to delete a list by opening the list you're presented with an error that the list cannot be found.

http://screencast.com/t/fn0bg4bipnG
Comment 1 Nicole C. Engard 2011-04-28 12:10:16 UTC

*** This bug has been marked as a duplicate of bug 5524 ***
Comment 2 Fridolin SOMERS 2013-02-22 16:20:32 UTC
This is still existing in actual master : 3.11.00.018
Comment 3 Fridolin SOMERS 2013-02-22 17:12:22 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2013-02-25 17:09:56 UTC
Testing after applying the patch:

- Viewing private lists, navigate to second page of lists
- Click 'delete' link next to a list name for an EMPTY list.
- Deletion successful.

Success.

- Viewing private lists, navigate to second page of lists
- Click 'delete' link next to a list name for an list WITH CONTENTS.
- Deletion successful but WITHOUT WARNING (I guess it's this way in
  master too).

Success.

- Viewing private lists, navigate to second page of lists.
- Click an EMPTY list to view its contents.
- Click the "delete list" button from the list contents view.
- No error, list deleted.

Success.

- Viewing private lists, navigate to second page of lists.
- Click a list WITH CONTENTS to view its contents.
- Click the "delete list" button from the list contents view.
- No error. Redirected to the first page of private lists with 
  no link to the second page of lists. Navigating back to private
  lists and then to the second page of lists shows that the list 
  was not deleted.

Fail.

- Viewing public lists, navigate to second page of lists
- Click 'delete' link next to a list name for an EMPTY list.
- No error. Redirected to the first page of public lists with no 
  link to the second page of lists. Navigating back to public lists
  and then to the second page of lists shows that the list WAS
  deleted.

Success*

- Viewing public lists, navigate to second page of lists
- Click 'delete' link next to a list name for list WITH CONTENTS.
- No error. Redirected to the first page of public lists with no 
  link to the second page of lists. Navigating back to public lists
  and then to the second page of lists shows that the list WAS NOT
  deleted.

Fail.

- Viewing public lists, navigate to second page of lists.
- Click an EMPTY list to view its contents.
- Click the "delete list" button from the list contents view.
- No error. Redirected to the first page of public lists with no 
  link to the second page of lists. Navigating back to public lists
  and then to the second page of lists shows that the list WAS
  deleted.

Success*

- Viewing public lists, navigate to second page of lists.
- Click an list WITH CONTENTS to view its contents.
- Click the "delete list" button from the list contents view.
- No error. Redirected to the first page of public lists with no 
  link to the second page of lists. Navigating back to public lists
  and then to the second page of lists shows that the list WAS NOT
  deleted.

Fail.

* The problem of being redirected to the first page of lists with no pagination link to more lists is one which I thought we had a separate bug report for, but I can't find it. In terms of the list being deleted these tests are successful, but in terms of offering the user a working process they are a failure.
Comment 5 Fridolin SOMERS 2013-03-07 11:14:44 UTC
Seems lists need some revamping.
It is just impossible to correct this bug with current code.
Comment 6 Jonathan Druart 2013-06-27 14:58:56 UTC
Works for me on master.
Comment 7 Jonathan Druart 2014-09-25 10:37:51 UTC
Problem in master and 3.14.x (at least)
Comment 8 Jonathan Druart 2014-09-25 11:06:25 UTC
Marcel,
Could you have a look at this bug please?
I am a little bit lost in the VirtualShelves code.
I don't understand why the DelShelf is done as far in the code.
We shouldn't need to call GetShelves to delete a shelf.

Please tell me if you don't have the time and I will try to provide a patch.
Comment 9 Marcel de Rooy 2014-09-25 12:46:01 UTC
(In reply to Jonathan Druart from comment #8)
> Marcel,
> Could you have a look at this bug please?
> I am a little bit lost in the VirtualShelves code.
> I don't understand why the DelShelf is done as far in the code.
> We shouldn't need to call GetShelves to delete a shelf.
> 
> Please tell me if you don't have the time and I will try to provide a patch.

Will have a look
Comment 10 Marcel de Rooy 2014-09-25 13:12:45 UTC
The (horrible) code in VirtualShelves/Page.pm needs refactoring. I would like to do that when converting it to DBIx in one pass.

I try to reproduce your problem on master: I create 22 private lists. List 21 has one item, list 22 has no items. I delete list 21, have to confirm and it is gone. I delete list 22; no confirm button because it is empty. It is gone too.

OK. Reading back: If I click on list 21 and want to delete there. It says: list number unrecognized. Cool ;) Looking further..
Comment 11 Marcel de Rooy 2014-09-25 15:28:16 UTC Comment hidden (obsolete)
Comment 12 Owen Leonard 2014-09-25 18:35:35 UTC
(In reply to Owen Leonard from comment #4)
> Testing after applying the patch:

I once again followed this test plan.

> - Viewing public lists, navigate to second page of lists.
> - Click an list WITH CONTENTS to view its contents.
> - Click the "delete list" button from the list contents view.
> - No error. Redirected to the first page of public lists with no 
>   link to the second page of lists. Navigating back to public lists
>   and then to the second page of lists shows that the list WAS NOT
>   deleted.
> 
> Fail.

This was the only failure among those tests, other than the continuing problem of the pagination links disappearing.

I'm not sure whether to sign off on this because it is an improvement or ask that this one case be fixed as well.
Comment 13 Marcel de Rooy 2014-09-25 20:14:05 UTC
(In reply to Owen Leonard from comment #12)
> (In reply to Owen Leonard from comment #4)
> > Testing after applying the patch:
> 
> I once again followed this test plan.
> 
> > - Viewing public lists, navigate to second page of lists.
> > - Click an list WITH CONTENTS to view its contents.
> > - Click the "delete list" button from the list contents view.
> > - No error. Redirected to the first page of public lists with no 
> >   link to the second page of lists. Navigating back to public lists
> >   and then to the second page of lists shows that the list WAS NOT
> >   deleted.
> > 
> > Fail.
> 
> This was the only failure among those tests, other than the continuing
> problem of the pagination links disappearing.
> 
> I'm not sure whether to sign off on this because it is an improvement or ask
> that this one case be fixed as well.

Do you mean that the list was not deleted in this case?
Comment 14 Jonathan Druart 2014-09-26 07:28:08 UTC
Marcel,
There are several occurrences of "DEL-" in the template.
Your patch only changes one.


Something else, I didn't try but, if you remove the 4 lines in the pm file, are you sure we won't introduce a regression? I am under the impression that this test was there to verify the user has the right (i.e he is the owner/creator of the list) to delete the list?
Comment 15 Marcel de Rooy 2014-09-26 07:57:48 UTC
(In reply to Jonathan Druart from comment #14)
> Marcel,
> There are several occurrences of "DEL-" in the template.
> Your patch only changes one.
> 
> 
> Something else, I didn't try but, if you remove the 4 lines in the pm file,
> are you sure we won't introduce a regression? I am under the impression that
> this test was there to verify the user has the right (i.e he is the
> owner/creator of the list) to delete the list?

I am working on it. Please wait..
Comment 16 Marcel de Rooy 2014-09-26 10:12:54 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2014-09-26 10:13:44 UTC
(In reply to Jonathan Druart from comment #14)
> Something else, I didn't try but, if you remove the 4 lines in the pm file,
> are you sure we won't introduce a regression? I am under the impression that
> this test was there to verify the user has the right (i.e he is the
> owner/creator of the list) to delete the list?

No, the permission test is still in the code. Thanks.
Comment 18 Marcel de Rooy 2014-09-26 10:38:27 UTC
(In reply to Owen Leonard from comment #12)
> > - Viewing public lists, navigate to second page of lists.
> > - Click an list WITH CONTENTS to view its contents.
> > - Click the "delete list" button from the list contents view.
> > - No error. Redirected to the first page of public lists with no 
> >   link to the second page of lists. Navigating back to public lists
> >   and then to the second page of lists shows that the list WAS NOT
> >   deleted.
> > 
> > Fail.
> 
> This was the only failure among those tests, other than the continuing
> problem of the pagination links disappearing.
> 
> I'm not sure whether to sign off on this because it is an improvement or ask
> that this one case be fixed as well.

Thanks for testing, Owen.

I recall that deleting a list on the second page (from the all shelves view) brought me back to the first page without pagination. So the confirm button never appeared and I could not delete the list. But deleting it from its own shelf page (with the items) was successfull with the first patch.

The second patch should resolve some further issues with pagination. In order to achieve the best result in not too much time, I sacrificed the confirmation buttons. They were at least debatable since you already confirm a delete. Hopefully this can be signed off now.
Comment 19 Jonathan Druart 2014-09-26 10:49:02 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2014-09-26 10:49:09 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2014-09-26 10:50:33 UTC
All looks good to me, but I really would like another sign off on this one.
I am not confident at all in this code and cannot be sure this patch won't introduce regressions :-/
Comment 22 Fridolin SOMERS 2014-09-26 14:38:31 UTC Comment hidden (obsolete)
Comment 23 Fridolin SOMERS 2014-09-26 14:39:53 UTC Comment hidden (obsolete)
Comment 24 Fridolin SOMERS 2014-09-26 14:41:01 UTC
Nice job, works fine
Comment 25 Owen Leonard 2014-09-26 18:41:35 UTC
Testing this step in the OPAC:

> - Viewing private [and private] lists, navigate to second page of lists
> - Click 'delete' link next to a list name for an list WITH CONTENTS.

Clicking 'delete' give me a JavaScript prompt but doesn't redirect back to the list with a confirmation button to delete the contents of the list as it did before. This means that if JavaScript is turned off a list with contents can be deleted without any warning at all. This seems like a regression to me.
Comment 26 Marcel de Rooy 2014-09-29 06:55:07 UTC
(In reply to Owen Leonard from comment #25)
> Testing this step in the OPAC:
> 
> > - Viewing private [and private] lists, navigate to second page of lists
> > - Click 'delete' link next to a list name for an list WITH CONTENTS.
> 
> Clicking 'delete' give me a JavaScript prompt but doesn't redirect back to
> the list with a confirmation button to delete the contents of the list as it
> did before. This means that if JavaScript is turned off a list with contents
> can be deleted without any warning at all. This seems like a regression to
> me.

OK. I am not sure if running Koha without JavaScript these days will be very progressive, but this sounds valid.
At this point, I would go for refactoring instead of spending more time on the current code. So I obsolete both patches.
Comment 27 Marcel de Rooy 2014-09-29 09:49:43 UTC
I just disabled javascript in Firefox (far away in about:config btw).

If I want to go to my lists in the opac and click on the Lists menu, nothing happens. Same for clicking on Cart. Add to cart does not work. Etc. etc.

If we still should develop with disabled javascript in mind, is not the object of this report, but there may already be quite some functionality that does not work without it.
Comment 28 Jonathan Druart 2014-12-11 14:39:52 UTC
(In reply to M. de Rooy from comment #26)
> At this point, I would go for refactoring instead of spending more time on
> the current code. So I obsolete both patches.

It's a major bug and still exists on master.
I would like to see this bug fixed quickly.
And the quicker way is to push these patches, don't you think?
Comment 29 Marcel de Rooy 2014-12-17 14:49:44 UTC
(In reply to Jonathan Druart from comment #28)
> (In reply to M. de Rooy from comment #26)
> > At this point, I would go for refactoring instead of spending more time on
> > the current code. So I obsolete both patches.
> 
> It's a major bug and still exists on master.
> I would like to see this bug fixed quickly.
> And the quicker way is to push these patches, don't you think?

See comment 25 for a reason not to.
Comment 30 Jonathan Druart 2015-08-18 13:45:50 UTC
Will be fixed by 14544.
Comment 31 Jonathan Druart 2015-09-15 10:32:39 UTC
A fix could be submitted on top of 3.20, in hopes that big 14544 will be into 3.22.
Comment 32 Jonathan Druart 2015-11-13 14:47:04 UTC
Fixed in 3.22 by bug 14544.