|
Description
Lucas Gass (lukeg)
2024-06-27 14:59:50 UTC
Created attachment 168214 [details] [review] Bug 37213: Improved breadcrumbs in rotating collections To test: 1. Tools -> Rotating collections -> New collections 2. Add a title and description 3. After adding the new collection notice the breadcrumb isn't right 4. Apply patch 5. Repeat steps 1 and 2 6. After adding the new collection notice the breadcrumb is improved. Created attachment 168216 [details] [review] Bug 37213: Improved breadcrumbs in rotating collections To test: 1. Tools -> Rotating collections -> New collections 2. Add a title and description 3. After adding the new collection notice the breadcrumb isn't right 4. Apply patch 5. Repeat steps 1 and 2 6. After adding the new collection notice the breadcrumb is improved. Signed-off-by: Sam Lau <samalau@gmail.com> Added Assignee now. Please fill yourself next time :) This looks a bit odd. I would not expect created or deleted to pop up in the breadcrumbs. If something failed, we should have a dialog alert somewhere else.
Furthermore there seems to be an issue with:
[% WRAPPER breadcrumb_item %]
<a href="/cgi-bin/koha/rotating_collections/addItems.pl?colId=[% editColId | uri %]">[% tx("Collection '{collection_title}'", { collection_title = editColTitle }) | html %]</em></a>
[% END %]
The quoted empty string comes from there. Should the script pass a correct op and record id here instead? If you delete a collection, probably the op should change to new. If you added or updated, we need the record id.
Note also that I see an </em> without an opening <em> ?
Created attachment 168232 [details] [review] Bug 37213: (follow-up) Remove stray </em>, make title logic consistent with breadcrumbs (In reply to Marcel de Rooy from comment #4) > This looks a bit odd. I would not expect created or deleted to pop up in the > breadcrumbs. If something failed, we should have a dialog alert somewhere > else. We do have a dialog alert. The thing which is inconsistent about this page is that there is a "creation success/failure" stage at all. Usually we would redirect to the list of collections in either case, adding a dialog if there was an error. I think these changes are logical given the unusual workflow of having a "success" page. > The quoted empty string comes from there. Should the script pass a correct > op and record id here instead? If you delete a collection, probably the op > should change to new. If you added or updated, we need the record id. > > Note also that I see an </em> without an opening <em> ? These issues are all unrelated to the changes in the original patc. I've submitted a follow-up to correct them. Created attachment 168575 [details] [review] Bug 37213: Improved breadcrumbs in rotating collections To test: 1. Tools -> Rotating collections -> New collections 2. Add a title and description 3. After adding the new collection notice the breadcrumb isn't right 4. Apply patch 5. Repeat steps 1 and 2 6. After adding the new collection notice the breadcrumb is improved. Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 168576 [details] [review] Bug 37213: (follow-up) Remove stray </em>, make title logic consistent with breadcrumbs Signed-off-by: David Nind <david@davidnind.com> Not related to this bug, but I'm not sure that I understand the difference between Tools > Rotating collections and Cataloging > Automation > Stock rotation (when the StockRotation system preference is enabled). Having similar names is a bit confusing. Created attachment 171269 [details] [review] Bug 37213: Improved breadcrumbs in rotating collections To test: 1. Tools -> Rotating collections -> New collections 2. Add a title and description 3. After adding the new collection notice the breadcrumb isn't right 4. Apply patch 5. Repeat steps 1 and 2 6. After adding the new collection notice the breadcrumb is improved. Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Created attachment 171270 [details] [review] Bug 37213: (follow-up) Remove stray </em>, make title logic consistent with breadcrumbs Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> I passed QA on this because the reported bug was fixed. If there's an issue with the unusual pattern I think this should be addressed in a follow-up bug to this one. I hope that's fine. Pushed for 24.11! Well done everyone, thank you! |