|
Description
Nicole C. Engard
2011-04-28 12:09:36 UTC
*** This bug has been marked as a duplicate of bug 5524 *** This is still existing in actual master : 3.11.00.018 Created attachment 15627 [details] [review] Proposed patch See commit message 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. Seems lists need some revamping. It is just impossible to correct this bug with current code. Works for me on master. Problem in master and 3.14.x (at least) 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. (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 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.. Created attachment 31883 [details] [review] Bug 6279: Cannot delete lists from second page of lists in the OPAC The code in Page.pm still needs more attention, but this may serve as a quick fix for the problem; it will at least improve the situation. If you delete a list from its individual page (viewshelf), you already see the items (if any). So we will assume that after you said Yes to the message box, you want to delete it. There will not be a confirmation button anymore. Note that after you deleted the list, the intended message is not shown and the pagination bar is gone. I leave those for the future refactoring. Test plan: Delete a list from its own viewshelf page in OPAC and staff. Note that OPAC will delete it rightaway now. Note that staff still has the confirmation button. (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. (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? 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? (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.. Created attachment 31892 [details] [review] Bug 6279: Follow-up for pagination and confirm This follow-up patch restores the pagination and removes the Confirm buttons. This is not really needed since you already confirmed a javascript messagebox. The code needs refactoring. This is a quick fix. Test plan: Delete lists from the second page (from list view and detail view) in both OPAC and staff. (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. (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. Created attachment 31893 [details] [review] Bug 6279: Cannot delete lists from second page of lists in the OPAC The code in Page.pm still needs more attention, but this may serve as a quick fix for the problem; it will at least improve the situation. If you delete a list from its individual page (viewshelf), you already see the items (if any). So we will assume that after you said Yes to the message box, you want to delete it. There will not be a confirmation button anymore. Note that after you deleted the list, the intended message is not shown and the pagination bar is gone. I leave those for the future refactoring. Test plan: Delete a list from its own viewshelf page in OPAC and staff. Note that OPAC will delete it rightaway now. Note that staff still has the confirmation button. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 31894 [details] [review] Bug 6279: Follow-up for pagination and confirm This follow-up patch restores the pagination and removes the Confirm buttons. This is not really needed since you already confirmed a javascript messagebox. The code needs refactoring. This is a quick fix. Test plan: Delete lists from the second page (from list view and detail view) in both OPAC and staff. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> 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 :-/ Created attachment 31898 [details] [review] Bug 6279: Cannot delete lists from second page of lists in the OPAC The code in Page.pm still needs more attention, but this may serve as a quick fix for the problem; it will at least improve the situation. If you delete a list from its individual page (viewshelf), you already see the items (if any). So we will assume that after you said Yes to the message box, you want to delete it. There will not be a confirmation button anymore. Note that after you deleted the list, the intended message is not shown and the pagination bar is gone. I leave those for the future refactoring. Test plan: Delete a list from its own viewshelf page in OPAC and staff. Note that OPAC will delete it rightaway now. Note that staff still has the confirmation button. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Created attachment 31899 [details] [review] Bug 6279: Follow-up for pagination and confirm This follow-up patch restores the pagination and removes the Confirm buttons. This is not really needed since you already confirmed a javascript messagebox. The code needs refactoring. This is a quick fix. Test plan: Delete lists from the second page (from list view and detail view) in both OPAC and staff. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Nice job, works fine 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.
(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. 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. (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? (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. Will be fixed by 14544. A fix could be submitted on top of 3.20, in hopes that big 14544 will be into 3.22. Fixed in 3.22 by bug 14544. |