1. Tools > Rotating collections > New collection 2. Add a title and description 3. After adding the new collection notice the breadcrumb isn't right: Tools > Rotating collections > Collection " > Edit collection
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!
Backported to 24.05.x for upcoming 24.05.06