Skip to content

Handle deleted collection items#1110

Merged
fbacall merged 4 commits intomasterfrom
handle-deleted-collection-items
Jun 3, 2025
Merged

Handle deleted collection items#1110
fbacall merged 4 commits intomasterfrom
handle-deleted-collection-items

Conversation

@fbacall
Copy link
Copy Markdown
Member

@fbacall fbacall commented May 23, 2025

Summary of changes

  • Displays a "tombstone" where an item in a collection or learning path topic has been deleted.

Motivation and context

#1108

Opted not to delete the CollectionItem/LearningPathTopicItem because there could be useful context in the comment.

Screenshots

image

Checklist

  • I have read and followed the CONTRIBUTING guide.
  • I confirm that I have the authority necessary to make this contribution on behalf of its copyright owner and agree
    to license it to the TeSS codebase under the
    BSD license.

@fbacall fbacall requested a review from Copilot May 27, 2025 12:47
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds support for displaying a “tombstone” overlay when a referenced resource has been deleted in collections or learning paths.

  • Introduces fixtures and controller tests for learning paths, topics, and collections containing deleted items
  • Updates views (_learning_path_topic_item, _collection_item, new _deleted_resource partial) to conditionally render a tombstone instead of missing resources
  • Adds localization, CSS styles, Safe Navigation in models, and template changes for autocompleter

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
test/fixtures/*.yml Added fixtures for paths, topics, collections with deleted items
test/controllers/*_controller_test.rb New tests for show/edit/update when resource is deleted
config/locales/en.yml Added deleted_resource localization key
app/views/common/_deleted_resource.html.erb New partial for deleted-item overlay
app/views/learning_path_topic_items/_*.html.erb Render deleted-item partial when resource is missing
app/views/collection_items/_*.html.erb Ditto for collections
app/views/public_activity/common/_add_item.html.erb Fallback title for deleted resource
app/views/collections/_collection_items_form.erb JSON form: handle missing resource title and URL
app/assets/stylesheets/masonry.scss Styles for .deleted-item-overlay
app/assets/javascripts/templates/*.hbs Autocompleter: show muted span if no URL
app/models/*_item.rb Use &. for resource title and guard secondary activity
Comments suppressed due to low confidence (1)

test/fixtures/collection_items.yml:14

  • [nitpick] Using a hardcoded large ID assumes the record doesn't exist, which can make tests brittle; consider using a fixture with no resource association or an explicit 'missing' fixture.
  resource_id: 9999999 # Lets hope this doesn't exist


get :show, params: { id: learning_path }
assert_response :success
assert_response :success
Copy link

Copilot AI May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion is duplicated; you can remove the extra assert_response :success for clarity.

Suggested change
assert_response :success

Copilot uses AI. Check for mistakes.
Comment on lines +4 to 6
<%= t('activity.actions.add') %> <%= type.model_name.human.downcase %> <%= link_to activity.parameters[:resource_title] || t('deleted_resource'),
polymorphic_path(type.model_name.singular.to_sym,
id: activity.parameters[:resource_id]) %>
Copy link

Copilot AI May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Deleted resources are still rendered as links, which may lead to broken paths; consider rendering a non-clickable element (e.g. <span>) when the resource is missing.

Suggested change
<%= t('activity.actions.add') %> <%= type.model_name.human.downcase %> <%= link_to activity.parameters[:resource_title] || t('deleted_resource'),
polymorphic_path(type.model_name.singular.to_sym,
id: activity.parameters[:resource_id]) %>
<%= t('activity.actions.add') %> <%= type.model_name.human.downcase %>
<% if activity.parameters[:resource_id].present? %>
<%= link_to activity.parameters[:resource_title] || t('deleted_resource'),
polymorphic_path(type.model_name.singular.to_sym, id: activity.parameters[:resource_id]) %>
<% else %>
<span><%= t('deleted_resource') %></span>
<% end %>

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +2
<% learning_path_topic_item ||= nil %>
<% collection_item ||= learning_path_topic_item %>
Copy link

Copilot AI May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The partial mixes learning_path_topic_item and collection_item, which can be confusing; consider passing a generic item local to simplify and clarify variable usage.

Suggested change
<% learning_path_topic_item ||= nil %>
<% collection_item ||= learning_path_topic_item %>
<% item ||= nil %>

Copilot uses AI. Check for mistakes.
@fbacall fbacall merged commit 8c1f29a into master Jun 3, 2025
11 checks passed
@fbacall fbacall deleted the handle-deleted-collection-items branch June 3, 2025 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants