Adds items in this collection section. #724

justinlittman · 2023-10-23T19:03:19Z

closes #714

jcoyne · 2023-10-24T02:20:32Z

app/models/public_xml.rb

@@ -45,4 +45,8 @@ def relations(predicate)
      node.attribute('resource').text.split('/', 2).last.split(':', 2).last
    end
  end
+
+  def object_type
+    document.root.at_xpath('identityMetadata/objectType').try(:text)


Why try here? There shouldn't be any nodes with out an objectType, right? That's pretty fundamental.

Because every other call to .text in this class is .try(:text). I have no idea why that would be, but following existing conventions.

I don't think that's a very good reason. A lot of this code was written 12 years ago before the model was clear and before there was a safe navigation operator in Ruby.

jcoyne · 2023-10-24T02:22:30Z

app/views/purl/_collection_items.html.erb

@@ -0,0 +1,3 @@
+<% if @purl.object_type == 'collection' && @purl.catalog_key %>


Can you make a method collection? here?

I would have, except PurlResource already has a .collection method which provides the collection that the item is a part of. It seems that a .collection? method that indicates whether this item is a collection risks confusion.

I still feel that we should move as much logic from the views as possible. If we need to rename collection to is_member_of, I think that would be good to and help align with Cocina terminology.

This method name has been refactored, so please move this to the collection? method now.

jcoyne · 2023-10-24T02:22:49Z

app/views/purl/_collection_items.html.erb

@@ -0,0 +1,3 @@
+<% if @purl.object_type == 'collection' && @purl.catalog_key %>
+  <%= link_to 'View items in this collection in SearchWorks', "#{Settings.searchworks.url}/catalog?f[collection][]=#{@purl.catalog_key}" %>


~~Is it possible the collection is not released to searchworks?~~

Nope, because it has a catalog key.

jcoyne · 2023-10-24T02:32:02Z

spec/views/purl/_collection_items.html.erb_spec.rb

+  context 'when a published collection' do
+    let(:purl) { PurlResource.new(id: 'bb631ry3167') }
+
+    it 'displays a View items in this collection link' do


Isn't this same scenario tested in the integration test?

Yes, but I wanted the test cases in this spec to be complete so that a dev could understand the expected behavior from the view from this alone.

The integration test is necessary because it tests that the view is actually called.

jcoyne · 2023-10-24T02:33:30Z

spec/views/purl/_collection_items.html.erb_spec.rb

+
+    it 'does not display View items in this collection link' do
+      render
+      expect(rendered).not_to have_css 'a', text: 'View items in this collection in SearchWorks'


These two negative assertion tests are very fragile because a change to the text in the template that does not also change these tests completely makes these assertions useless. E.g we could show a link, but since the text has changed we won't catch it.

Can we just assert that nothing is rendered in this case?

I couldn't figure out how to assert nothing is rendered, so just removed the text assertion.

jcoyne · 2023-10-24T02:35:33Z

app/views/purl/_mods_metadata_sections.html.erb

@@ -85,6 +86,18 @@
  </div>
 <% end %>

+<% if collection_items.present? %>


Can't you just move all of this into the partial? That is already wrapped in a conditional. Alternatively if you introduce a ViewComponent here, you can make this be the render? condition. I find that much easier to read and test rather than this approach here.

I agree with you, but I am following the existing pattern in this file. I'm not trying to refactor in this PR.

I've changed the existing pattern in #727 so please follow that pattern here.

jcoyne · 2023-10-24T02:36:23Z

spec/model/purl_resource_spec.rb

+
+  describe '#object_type' do
+    it 'pulls the value from the identity metadata' do
+      allow(subject).to receive(:public_xml_body).and_return <<-EOF


Please move this to a before as it's part of setting up the scenario and has no assertions.

I'm just following the pattern in this file.

This codebase is full of bad patterns and needs to be over hauled. Please use practices like https://www.betterspecs.org/#mock and not the existing patterns here.

jcoyne · 2023-10-24T03:04:43Z

spec/views/purl/_collection_items.html.erb_spec.rb

+    end
+  end
+
+  context 'when an unpublished collection' do


How do we know it's "unpublished"? Do you mean "doesn't have a catkey"? I'm concerned that we're overloading what "published" means.

OK, changed to when collection does not have a catalog record id.

closes #714

jcoyne · 2023-10-24T18:49:57Z

There are now conflicts with the DOI feature.

justinlittman · 2023-10-25T13:47:54Z

Superceded by #736

jcoyne reviewed Oct 24, 2023

View reviewed changes

jcoyne requested changes Oct 24, 2023

View reviewed changes

jcoyne reviewed Oct 24, 2023

View reviewed changes

Adds items in this collection section.

38e43f3

closes #714

justinlittman force-pushed the t714-collection branch from e1cd970 to 38e43f3 Compare October 24, 2023 11:48

justinlittman closed this Oct 25, 2023

justinlittman deleted the t714-collection branch October 25, 2023 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds items in this collection section. #724

Adds items in this collection section. #724

justinlittman commented Oct 23, 2023

jcoyne Oct 24, 2023

justinlittman Oct 24, 2023

jcoyne Oct 24, 2023

jcoyne Oct 24, 2023

justinlittman Oct 24, 2023

jcoyne Oct 24, 2023

jcoyne Oct 24, 2023

jcoyne Oct 24, 2023 •

edited

Loading

jcoyne Oct 24, 2023

jcoyne Oct 24, 2023

justinlittman Oct 24, 2023

jcoyne Oct 24, 2023 •

edited

Loading

justinlittman Oct 24, 2023

jcoyne Oct 24, 2023

justinlittman Oct 24, 2023

jcoyne Oct 24, 2023

jcoyne Oct 24, 2023

justinlittman Oct 24, 2023

jcoyne Oct 24, 2023

jcoyne Oct 24, 2023

justinlittman Oct 24, 2023

jcoyne commented Oct 24, 2023

justinlittman commented Oct 25, 2023

		@@ -0,0 +1,3 @@
		<% if @purl.object_type == 'collection' && @purl.catalog_key %>

		@@ -0,0 +1,3 @@
		<% if @purl.object_type == 'collection' && @purl.catalog_key %>
		<%= link_to 'View items in this collection in SearchWorks', "#{Settings.searchworks.url}/catalog?f[collection][]=#{@purl.catalog_key}" %>

Adds items in this collection section. #724

Adds items in this collection section. #724

Conversation

justinlittman commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcoyne Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcoyne Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcoyne commented Oct 24, 2023

justinlittman commented Oct 25, 2023

jcoyne Oct 24, 2023 •

edited

Loading

jcoyne Oct 24, 2023 •

edited

Loading