Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

☄️ Epic: Failing Specs #324

Closed
29 tasks done
jillpe opened this issue Sep 20, 2023 · 2 comments
Closed
29 tasks done

☄️ Epic: Failing Specs #324

jillpe opened this issue Sep 20, 2023 · 2 comments
Assignees

Comments

@jillpe
Copy link

jillpe commented Sep 20, 2023

Summary

Specs will be likely failing after #325 and we would like to get them passing again to avoid missing issues in the app.

Accepted Criteria

  • Follow up tickets are created for failing specs

Testing Instructions

run specs from the knapsack directory, not hyrax-webapp

Per Rob

"I did check on this and got the specs to run. The way things are set up it does run the Hyku specs too but it allows you to override any spec file by making one the same path / name. So for example if the generic work specs fail because you changed the properties on it. You would make a generic work spec in the knapsack and that would override the Hyku version."

Failures

Ran specs off of:

CROSS OUT = SPECS REMOVED

  • rspec ./spec/forms/hyrax/generic_work_form_spec.rb:27 # Hyrax::GenericWorkForm.terms returns an array of inherited and custom terms
  • rspec ./spec/models/image_spec.rb:10 # Image indexer
  • rspec ./spec/features/labels_spec.rb:54 # Site labels configuration as an administrator institution name full updates the full institution name in the agreement text
  • rspec ./spec/features/labels_spec.rb:12 # Site labels configuration as an administrator application name updates the application name in the brand bar
  • rspec ./spec/features/labels_spec.rb:19 # Site labels configuration as an administrator application name updates the application name in the <title>
  • rspec ./spec/features/labels_spec.rb:36 # Site labels configuration as an administrator institution name updates the institution name in the agreement text
  • rspec ./spec/features/accounts_spec.rb:27 # Accounts administration as an superadmin changes the associated cname (Removed because it's not testing specific to adventist behavior)
  • rspec ./spec/features/accounts_spec.rb:39 # Accounts administration as an superadmin changes the account service endpoints (Removed because it's not testing specific to adventist behavior)
  • rspec ./spec/search_builders/adv_search_builder_spec.rb:55 # AdvSearchBuilder.default_processor_chain is expected to eq [:default_solr_parameters, :add_query_to_solr, :add_facet_fq_to_solr, :add_facetting_to_solr, :add_so...ccess, :exclude_models, :highlight_search_params, :show_parents_only, :include_allinson_flex_fields]
  • rspec ./spec/features/roles_spec.rb:12 # Site Roles as an administrator lists user roles
  • rspec ./spec/features/roles_spec.rb:19 # Site Roles as an administrator updates user roles
  • rspec ./spec/forms/hyrax/image_form_spec.rb:28 # Hyrax::ImageForm.terms returns an array of inherited and custom terms
  • rspec ./spec/features/contact_spec.rb:12 # Site contact configuration as an administrator application name updates the application name in the brand bar
  • rspec ./spec/presenters/hyku/admin/group/navigation_presenter_spec.rb:35 # Hyku::Admin::Group::NavigationPresenter with members page has 3 tabs
  • rspec ./spec/presenters/hyku/admin/group/navigation_presenter_spec.rb:51 # Hyku::Admin::Group::NavigationPresenter with remve page has 3 tabs
  • rspec ./spec/presenters/hyku/admin/group/navigation_presenter_spec.rb:19 # Hyku::Admin::Group::NavigationPresenter with edit page has 3 tabs
  • rspec ./spec/controllers/hyrax/hyrax/generic_works_controller_spec.rb:23 # Hyrax::GenericWorksController#presenter initializes a presenter
  • rspec ./spec/controllers/hyrax/hyrax/admin/appearances_controller_spec.rb:30 # Hyrax::Admin::AppearancesController with an administrator GET #show assigns the requested site as @site
  • rspec ./spec/services/adventist/text_file_text_extraction_service_spec.rb:24 # Adventist::TextFileTextExtractionService position in the array of Hyrax::DerivativeService.services is in the first position
  • rspec ./spec/models/user_spec.rb:11 # User the first created user in global tenant does not get the admin role
  • rspec ./spec/models/user_spec.rb:47 # User#site_roles= assigns global roles to the user
  • rspec ./spec/models/user_spec.rb:39 # User#site_roles fetches the global roles assigned to the user
  • rspec ./spec/models/user_spec.rb:30 # User a subsequent user does not get the admin role
  • rspec ./spec/models/user_spec.rb:20 # User the first created user on a tenant is given the admin role
  • rspec ./spec/models/hyrax/contact_form_spec.rb:30 # Hyrax::ContactForm headers site email set uses the Site email
  • rspec ./spec/models/hyrax/contact_form_spec.rb:21 # Hyrax::ContactForm headers no email set uses the hyrax setting
  • rspec ./spec/models/bulkrax/csv_entry_spec.rb:79 # Bulkrax::CsvEntry#build_metadata assigns factory_class and parsed_metadata
  • rspec ./spec/controllers/hyrax/hyrax/images_controller_spec.rb:28 # Hyrax::ImagesController#presenter initializes a presenter
@jillpe jillpe changed the title Get specs back to passing Epic: Failing Specs Sep 20, 2023
@jillpe
Copy link
Author

jillpe commented Sep 20, 2023

Please link follow up tickets to this ticket

@kirkkwang kirkkwang changed the title Epic: Failing Specs ☄️ Epic: Failing Specs Sep 29, 2023
@ShanaLMoore
Copy link
Contributor

ShanaLMoore commented Oct 2, 2023

There are a lot of errors that say a path doesn't exist, when they do.

ie: replacing visit edit_site_labels_path => '/site/labels/edit' will make the spec pass. These paths are defined in the hyrax-webapp. Why isn't it recognizing the helper methods?

rails routes | grep labels:

image

@ShanaLMoore ShanaLMoore assigned jeremyf and unassigned laritakr Oct 4, 2023
jeremyf referenced this issue Oct 4, 2023
Prior to this commit the Knapsace generic_work_form_spec had a test for terms
that was not part of the Hyku.  We introduce these differences through our
decoration in ./app/forms/hyrax/generic_work_form_decorator.rb

To reflect the existence of the decorator and not have a duplicate file
that future us would need to synchronize with Hyku, I've moved the file
and winnowed it down to reflect the specific changes.

I also needed to add three new properties to the spec as it reflects
upstream changes in Hyku (e.g. `access_right` `alternative_title`
`rights_notes`)

<details>
<summary>Diff of filein Hyku and Knapsack</summary>

```
❯ diff spec/forms/hyrax/generic_work_form_spec.rb hyrax-webapp/spec/forms/hyrax/generic_work_form_spec.rb
26,43d25
<   describe '.terms' do
<     it 'returns an array of inherited and custom terms' do
<       expect(described_class.terms.sort).to eq(
<         %i[
<           title creator contributor description keyword abstract
<           license rights_statement publisher date_created
<           subject language identifier based_near related_url
<           representative_id thumbnail_id rendering_ids files
<           visibility_during_embargo embargo_release_date visibility_after_embargo
<           visibility_during_lease lease_expiration_date visibility_after_lease
<           visibility ordered_member_ids source in_works_ids member_of_collection_ids
<           admin_set_id resource_type aark_id part_of place_of_publication
<           date_issued alt bibliographic_citation remote_url video_embed
<         ].sort
<       )
<     end
<   end
<
```
</details>

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
Prior to this commit the Knapsace image_form_spec had a test for terms
that was not part of the Hyku.  We introduce these differences through our
decoration in ./app/forms/hyrax/image_form_decorator.rb

To reflect the existence of the decorator and not have a duplicate file
that future us would need to synchronize with Hyku, I've moved the file
and winnowed it down to reflect the specific changes.

I also needed to add three new properties to the spec as it reflects
upstream changes in Hyku (e.g. `access_right` `alternative_title`
`rights_notes`)

<details>
<summary>Diff of filein Hyku and Knapsack</summary>

```
❯ diff spec/forms/hyrax/image_form_spec.rb hyrax-webapp/spec/forms/hyrax/image_form_spec.rb
27,44d26
<   describe '.terms' do
<     it 'returns an array of inherited and custom terms' do
<       expect(described_class.terms.sort).to eq(
<         %i[
<           title creator contributor description keyword abstract
<           license rights_statement publisher date_created
<           subject language identifier based_near related_url
<           representative_id thumbnail_id rendering_ids files
<           visibility_during_embargo embargo_release_date visibility_after_embargo
<           visibility_during_lease lease_expiration_date visibility_after_lease
<           visibility ordered_member_ids source in_works_ids member_of_collection_ids
<           admin_set_id resource_type extent aark_id part_of place_of_publication
<           date_issued alt bibliographic_citation remote_url
<         ].sort
<       )
<     end
<   end
<
```
</details>

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
Prior to this commit the Knapsace image_form_spec had a test for terms
that was not part of the Hyku.  We introduce these differences through our
decoration in ./app/forms/hyrax/image_form_decorator.rb

To reflect the existence of the decorator and not have a duplicate file
that future us would need to synchronize with Hyku, I've moved the file
and winnowed it down to reflect the specific changes.

I also needed to add three new properties to the spec as it reflects
upstream changes in Hyku (e.g. `access_right` `alternative_title`
`rights_notes`)

<details>
<summary>Diff of filein Hyku and Knapsack</summary>

```
❯ diff spec/forms/hyrax/image_form_spec.rb hyrax-webapp/spec/forms/hyrax/image_form_spec.rb
27,44d26
<   describe '.terms' do
<     it 'returns an array of inherited and custom terms' do
<       expect(described_class.terms.sort).to eq(
<         %i[
<           title creator contributor description keyword abstract
<           license rights_statement publisher date_created
<           subject language identifier based_near related_url
<           representative_id thumbnail_id rendering_ids files
<           visibility_during_embargo embargo_release_date visibility_after_embargo
<           visibility_during_lease lease_expiration_date visibility_after_lease
<           visibility ordered_member_ids source in_works_ids member_of_collection_ids
<           admin_set_id resource_type extent aark_id part_of place_of_publication
<           date_issued alt bibliographic_citation remote_url
<         ].sort
<       )
<     end
<   end
<
```
</details>

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
Prior to this commit, we had a copy of the ability_spec.rb.  That
Knapsack copy looks to have less coverage than the Hyku version.  And I
found the *diff of ability spec before commit* to be a bit confusing.

To see what would change I copied Hyku's spec over which is present in
the diff *Comparing full changes when copying Hyku's ability spec to Knapsack*

My preference would be do have an ability_decorator_spec to highlight if
and how the abilities of this are different.

Regardless, I'm removing the spec for now.

<details>
<summary>diff of ability spec before commit</summary>

```
❯ diff spec/models/ability_spec.rb hyrax-webapp/spec/models/ability_spec.rb
56,57c56,61
<   describe 'an administrative user' do
<     let(:user) { FactoryBot.create(:admin) }
---
>   describe 'an ordinary user with a role on this tenant' do
>     let(:user) do
>       u = FactoryBot.create(:user)
>       u.add_role(:depositor)
>       u
>     end
61c65
<     it { is_expected.to be_able_to(:manage, Site) }
---
>     it { is_expected.not_to be_able_to(:manage, Site) }
66,67c70,71
<       it "has the admin group" do
<         expect(subject).to include 'admin'
---
>       it "does have the registered group" do
>         expect(subject).to include 'registered'
68a73,76
>
>       it "does not have the admin group" do
>         expect(subject).not_to include 'admin'
>       end
71a80,87
>   describe 'an administrative user' do
>     let(:user) { FactoryBot.create(:admin) }
>
>     it { is_expected.not_to be_able_to(:manage, :all) }
>     it { is_expected.not_to be_able_to(:manage, Account) }
>     it { is_expected.to be_able_to(:manage, Site) }
>   end
>
76a93,191
>
>   # Brought over from blacklight-access_controls v0.6.2
>   describe '#user_groups' do
>     subject { ability.user_groups }
>
>     context 'an admin user' do
>       let(:user) { FactoryBot.create(:admin) }
>
>       it { is_expected.to contain_exactly('admin', 'registered', 'public') }
>     end
>
>     # NOTE(bkiahstroud): Override to test guest users instead of
>     # "unregistered" (User.new) users; see User#add_default_group_membership!
>     context 'a guest user' do
>       let(:user) { create(:guest_user) }
>
>       it { is_expected.to contain_exactly('public') }
>     end
>
>     context 'a registered user' do
>       let(:user) { create(:user) }
>
>       it { is_expected.to contain_exactly('registered', 'public') }
>     end
>
>     # NOTE(bkiahstroud): Override test to create Hyrax::Groups
>     # that the user is a member of.
>     context 'a user with groups' do
>       let(:user)    { create(:user) }
>
>       before do
>         create(:group, name: 'group1', member_users: [user])
>         create(:group, name: 'group2', member_users: [user])
>       end
>
>       it { is_expected.to include('group1', 'group2') }
>     end
>   end
>
>   describe '#admin?' do
>     subject { ability.admin? }
>
>     context 'a user with the admin role' do
>       let(:user) { create(:admin) }
>
>       it { is_expected.to eq(true) }
>     end
>
>     context 'a user in the admin Hyrax::Group' do
>       let(:user) { create(:user) }
>
>       before do
>         create(:admin_group, member_users: [user])
>       end
>
>       it { is_expected.to eq(true) }
>     end
>
>     context 'a user without the admin role' do
>       let(:user) { create(:user) }
>
>       it { is_expected.to eq(false) }
>     end
>
>     context 'a user not in the admin Hyrax::Group' do
>       let(:user) { create(:user) }
>
>       before do
>         create(:group, name: 'non-admin', member_users: [user])
>       end
>
>       it { is_expected.to eq(false) }
>     end
>   end
>
>   describe '#all_user_and_group_roles' do
>     let(:user) { create(:user) }
>     let(:user_reader_role) { create(:role, :user_reader) }
>     let(:collection_editor_role) { create(:role, :collection_editor) }
>     let(:work_depositor_role) { create(:role, :work_depositor) }
>
>     before do
>       user.add_role(user_reader_role.name, Site.instance)
>       create(
>         :group,
>         name: 'test_group',
>         member_users: [user],
>         roles: [collection_editor_role.name, work_depositor_role.name]
>       )
>     end
>
>     it 'lists all role names that apply to the user' do
>       expect(subject.all_user_and_group_roles).to contain_exactly(
>         user_reader_role.name,
>         collection_editor_role.name,
>         work_depositor_role.name
>       )
>     end
>   end
```

</details>

<details>
<summary>Comparing full changes when copying Hyku's ability spec to
Knapsack</summary>

```
diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb
index 424f6bc..ca8a439 100644
--- a/spec/models/ability_spec.rb
+++ b/spec/models/ability_spec.rb
@@ -53,20 +53,36 @@
     end
   end

+  describe 'an ordinary user with a role on this tenant' do
+    let(:user) do
+      u = FactoryBot.create(:user)
+      u.add_role(:depositor)
+      u
+    end
+
+    it { is_expected.not_to be_able_to(:manage, :all) }
+    it { is_expected.not_to be_able_to(:manage, Account) }
+    it { is_expected.not_to be_able_to(:manage, Site) }
+
+    describe "#user_groups" do
+      subject { ability.user_groups }
+
+      it "does have the registered group" do
+        expect(subject).to include 'registered'
+      end
+
+      it "does not have the admin group" do
+        expect(subject).not_to include 'admin'
+      end
+    end
+  end
+
   describe 'an administrative user' do
     let(:user) { FactoryBot.create(:admin) }

     it { is_expected.not_to be_able_to(:manage, :all) }
     it { is_expected.not_to be_able_to(:manage, Account) }
     it { is_expected.to be_able_to(:manage, Site) }
-
-    describe "#user_groups" do
-      subject { ability.user_groups }
-
-      it "has the admin group" do
-        expect(subject).to include 'admin'
-      end
-    end
   end

   describe 'a superadmin user' do
@@ -74,4 +90,103 @@

     it { is_expected.to be_able_to(:manage, :all) }
   end
+
+  # Brought over from blacklight-access_controls v0.6.2
+  describe '#user_groups' do
+    subject { ability.user_groups }
+
+    context 'an admin user' do
+      let(:user) { FactoryBot.create(:admin) }
+
+      it { is_expected.to contain_exactly('admin', 'registered', 'public') }
+    end
+
+    # NOTE(bkiahstroud): Override to test guest users instead of
+    # "unregistered" (User.new) users; see User#add_default_group_membership!
+    context 'a guest user' do
+      let(:user) { create(:guest_user) }
+
+      it { is_expected.to contain_exactly('public') }
+    end
+
+    context 'a registered user' do
+      let(:user) { create(:user) }
+
+      it { is_expected.to contain_exactly('registered', 'public') }
+    end
+
+    # NOTE(bkiahstroud): Override test to create Hyrax::Groups
+    # that the user is a member of.
+    context 'a user with groups' do
+      let(:user)    { create(:user) }
+
+      before do
+        create(:group, name: 'group1', member_users: [user])
+        create(:group, name: 'group2', member_users: [user])
+      end
+
+      it { is_expected.to include('group1', 'group2') }
+    end
+  end
+
+  describe '#admin?' do
+    subject { ability.admin? }
+
+    context 'a user with the admin role' do
+      let(:user) { create(:admin) }
+
+      it { is_expected.to eq(true) }
+    end
+
+    context 'a user in the admin Hyrax::Group' do
+      let(:user) { create(:user) }
+
+      before do
+        create(:admin_group, member_users: [user])
+      end
+
+      it { is_expected.to eq(true) }
+    end
+
+    context 'a user without the admin role' do
+      let(:user) { create(:user) }
+
+      it { is_expected.to eq(false) }
+    end
+
+    context 'a user not in the admin Hyrax::Group' do
+      let(:user) { create(:user) }
+
+      before do
+        create(:group, name: 'non-admin', member_users: [user])
+      end
+
+      it { is_expected.to eq(false) }
+    end
+  end
+
+  describe '#all_user_and_group_roles' do
+    let(:user) { create(:user) }
+    let(:user_reader_role) { create(:role, :user_reader) }
+    let(:collection_editor_role) { create(:role, :collection_editor) }
+    let(:work_depositor_role) { create(:role, :work_depositor) }
+
+    before do
+      user.add_role(user_reader_role.name, Site.instance)
+      create(
+        :group,
+        name: 'test_group',
+        member_users: [user],
+        roles: [collection_editor_role.name, work_depositor_role.name]
+      )
+    end
+
+    it 'lists all role names that apply to the user' do
+      expect(subject.all_user_and_group_roles).to contain_exactly(
+        user_reader_role.name,
+        collection_editor_role.name,
+        work_depositor_role.name
+      )
+    end
+  end
 end
```

</details>

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
In reviewing spec/models/collection_spec.rb and
hyrax-webapp/spec/models/collection_spec.rb; the specs in the Knapsack
are a subset of the specs in the collection_spec.rb

<details>
<summary>diff between knapsack and Hyku</summary>

```
❯ diff spec/models/collection_spec.rb hyrax-webapp/spec/models/collection_spec.rb
5a6,13
>   let(:user) { create(:user).tap { |u| u.add_role(:admin, Site.instance) } }
>   let(:account) { create(:account) }
>   let(:collection) { create(:collection, user: user) }
>
>   before do
>     Site.update(account: account)
>   end
>
14a23,73
>
>   describe "Featured Collections" do
>     before do
>       FeaturedCollection.create(collection_id: collection.id)
>     end
>
>     describe "#update" do
>       context "when collection is public" do
>         let(:collection) { create(:public_collection) }
>
>         it "does not remove the collection from featured collections" do
>           expect(collection).not_to receive(:remove_featured)
>           collection.update(title: ['New collection title'])
>         end
>       end
>
>       context "when collection is private" do
>         let(:collection) { create(:private_collection) }
>
>         it "removes collection from featured collections" do
>           expect(collection).to receive(:remove_featured)
>           collection.update(title: ['New collection title'])
>         end
>       end
>
>       context "when collection is changed from public to private" do
>         let(:collection) { create(:public_collection) }
>
>         it "removes collection from featured collections" do
>           expect(collection).to receive(:remove_featured)
>           collection.visibility = Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PRIVATE
>           collection.save!
>         end
>       end
>     end
>
>     describe "#destroy" do
>       it 'deletes the featured collection after destroying the collection' do
>         expect(collection).to receive(:remove_featured)
>         collection.destroy
>       end
>     end
>
>     describe "#remove_featured" do
>       it 'removes collection from featured collections' do
>         expect(FeaturedCollection.where(collection_id: collection.id).count).to eq(1)
>         collection.remove_featured
>         expect(FeaturedCollection.where(collection_id: collection.id).count).to eq(0)
>       end
>     end
>   end
```

</details>

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
Prior to this commit the diff was as follows:

```
❯ diff spec/models/image_spec.rb hyrax-webapp/spec/models/image_spec.rb
10c10
<     it { is_expected.to eq WorkIndexer }
---
>     it { is_expected.to eq ImageIndexer }
```

When I ran the specs I got the following:

```
Failures:

  1) Image indexer
     Failure/Error: it { is_expected.to eq WorkIndexer }

     NameError:
       uninitialized constant WorkIndexer
     # ./spec/models/image_spec.rb:10:in `block (3 levels) in <top (required)>'
     # /usr/local/bundle/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'
     # ./hyrax-webapp/spec/support/multitenancy_metadata.rb:50:in `block (2 levels) in <top (required)>'
     # /usr/local/bundle/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:124:in `block in run'
     # /usr/local/bundle/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:110:in `loop'
     # /usr/local/bundle/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:110:in `run'
     # /usr/local/bundle/gems/rspec-retry-0.6.2/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # /usr/local/bundle/gems/rspec-retry-0.6.2/lib/rspec/retry.rb:37:in `block (2 levels) in setup'
     # ./spec/spec_helper.rb:10:in `block (2 levels) in <top (required)>'

Top 1 slowest examples (0.01817 seconds, 5.4% of total time):
  Image indexer
    0.01817 seconds ./spec/models/image_spec.rb:10

Finished in 0.33691 seconds (files took 13.55 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/models/image_spec.rb:10 # Image indexer
```

I updated the spec to say `it { is_expected.to eq ImageIndexer }` and
the spec passed.

This resulted in the knapsack version being a duplicate of the hyku
version.  So fairwell duplicate!

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
The knapsack tests of `spec/models/role_spec.rb` is a subset of the
tests in `hyrax-webapp/spec/models/role_spec.rb`

As such, we can fully remove those tests.  To further check, I copied
over the Hyku version ran the spec; they all passed.  So we can now
defer to Hyku on these tests.

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
Below is the diff showing that the knapsack is a subset of the hyku
version.  As such, we can delete the knapsack version.

```
❯ diff spec/models/site_spec.rb hyrax-webapp/spec/models/site_spec.rb
74a75,92
>
>     context "valid attributes" do
>       subject { described_class.new }
>
>       it "is valid without theme attributes" do
>         expect(subject).to be_valid
>       end
>
>       it "is valid with home page theme attributes" do
>         subject.home_theme = "Catchy Theme"
>         subject.show_theme = "Images Show Page"
>         subject.search_theme = "Grid View"
>         expect(subject).to be_valid
>         expect(subject.home_theme).to eq "Catchy Theme"
>         expect(subject.show_theme).to eq "Images Show Page"
>         expect(subject.search_theme).to eq "Grid View"
>       end
>     end
```

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
Looking at the diff between knapsack and Hyku we have the following:

```
❯ diff spec/models/uploaded_file_spec.rb hyrax-webapp/spec/models/uploaded_file_spec.rb
34c34
<     before { expect(Hyrax::UploadedFileUploader).to receive(:storage).and_return(described_class) }
---
>     before { expect(Hyrax::UploadedFileUploader).to receive(:storage).and_return(described_class).at_least(:once) }
```

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
The user spec had significant changes between knapsack and hyku.
However, since we don't have a user model nor decorator in the knapsack,
I think those differences reflect changes between the knapsacks's
originating hyku version and the updated hyku version.

To verify, I copied over hyku's updated user spec and ran the specs.
Everything passed.  Below is the original diff.

<details>
<summary>Diff between knapsack and hyku for user</summary>
```
3a4,8
>   it 'validates email and password' do
>     is_expected.to validate_presence_of(:email)
>     is_expected.to validate_presence_of(:password)
>   end
>
5c10
<     subject { FactoryBot.create(:base_user) }
---
>     subject { FactoryBot.create(:user) }
13a19
>       expect(subject).not_to have_role :admin, Site.instance
18c24
<     subject { FactoryBot.create(:base_user) }
---
>     subject { FactoryBot.create(:user) }
20,22c26,28
<     it 'is given the admin role' do
<       expect(subject.persisted?).to eq true
<       expect(subject).to have_role :admin, Site.instance
---
>     it 'is not given the admin role' do
>       expect(subject).not_to have_role :admin
>       expect(subject).not_to have_role :admin, Site.instance
27,28c33,34
<     let!(:first_user) { FactoryBot.create(:base_user) }
<     let!(:next_user) { FactoryBot.create(:base_user) }
---
>     let!(:first_user) { FactoryBot.create(:user) }
>     let!(:next_user) { FactoryBot.create(:user) }
30,31c36
<     it 'does not get the admin role' do
<       expect(next_user.persisted?).to eq true
---
>     it 'is not given the admin role' do
32a38
>       expect(next_user).not_to have_role :admin, Site.instance
40c46
<       expect(subject.site_roles.pluck(:name)).to match_array ['admin', 'registered']
---
>       expect(subject.site_roles.pluck(:name)).to match_array ['admin']
48c54
<       expect(subject.site_roles.pluck(:name)).to match_array ['registered']
---
>       expect(subject.site_roles.pluck(:name)).to be_empty
50c56
<       subject.update(site_roles: ['admin', 'registered'])
---
>       subject.update(site_roles: ['admin'])
52c58
<       expect(subject.site_roles.pluck(:name)).to match_array ['admin', 'registered']
---
>       expect(subject.site_roles.pluck(:name)).to match_array ['admin']
60a67,138
>
>   describe '#hyrax_groups' do
>     subject { FactoryBot.create(:user) }
>
>     it 'returns an array of Hyrax::Groups' do
>       expect(subject.hyrax_groups).to be_an_instance_of(Array)
>       expect(subject.hyrax_groups.first).to be_an_instance_of(Hyrax::Group)
>     end
>   end
>
>   describe '#groups' do
>     subject { FactoryBot.create(:user) }
>
>     before do
>       FactoryBot.create(:group, name: 'group1', member_users: [subject])
>     end
>
>     it 'returns the names of the Hyrax::Groups the user is a member of' do
>       expect(subject.groups).to include('group1')
>     end
>   end
>
>   describe '#hyrax_group_names' do
>     subject { FactoryBot.create(:user) }
>
>     before do
>       FactoryBot.create(:group, name: 'group1', member_users: [subject])
>     end
>
>     it 'returns the names of the Hyrax::Groups the user is a member of' do
>       expect(subject.hyrax_group_names).to include('group1')
>     end
>   end
>
>   describe '#add_default_group_membership!' do
>     context 'when the user is a new user' do
>       subject { FactoryBot.build(:user) }
>
>       it 'is called after a user is created' do
>         expect(subject).to receive(:add_default_group_membership!) # rubocop:disable RSpec/SubjectStub
>
>         subject.save!
>       end
>     end
>
>     # #add_default_group_membership! does nothing for guest users;
>     # 'public' is the default group for all users (including guests).
>     # See Ability#default_user_groups in blacklight-access_controls-0.6.2
>     context 'when the user is a guest user' do
>       subject { FactoryBot.build(:guest_user) }
>
>       it 'does not get any Hyrax::Group memberships' do
>         expect(subject.hyrax_group_names).to eq([])
>
>         subject.save!
>
>         expect(subject.hyrax_group_names).to eq([])
>       end
>     end
>
>     context 'when the user is a registered user' do
>       subject { FactoryBot.build(:user) }
>
>       it 'adds the user as a member of the registered Hyrax::Group' do
>         expect(subject.hyrax_group_names).to eq([])
>
>         subject.save!
>
>         expect(subject.hyrax_group_names).to contain_exactly('registered')
>       end
>     end
>   end
```
</details>

And here is the diff of changes when I copied things over:

<details>
<summary>Copied Hyku spec over and used git diff for what would change in Knapsack</summary>

```
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index dd9d03f..f20dccc 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1,8 +1,13 @@
 # frozen_string_literal: true

 RSpec.describe User, type: :model do
+  it 'validates email and password' do
+    is_expected.to validate_presence_of(:email)
+    is_expected.to validate_presence_of(:password)
+  end
+
   context 'the first created user in global tenant' do
-    subject { FactoryBot.create(:base_user) }
+    subject { FactoryBot.create(:user) }

     before do
       allow(Account).to receive(:global_tenant?).and_return true
@@ -11,25 +16,26 @@
     it 'does not get the admin role' do
       expect(subject.persisted?).to eq true
       expect(subject).not_to have_role :admin
+      expect(subject).not_to have_role :admin, Site.instance
     end
   end

   context 'the first created user on a tenant' do
-    subject { FactoryBot.create(:base_user) }
+    subject { FactoryBot.create(:user) }

-    it 'is given the admin role' do
-      expect(subject.persisted?).to eq true
-      expect(subject).to have_role :admin, Site.instance
+    it 'is not given the admin role' do
+      expect(subject).not_to have_role :admin
+      expect(subject).not_to have_role :admin, Site.instance
     end
   end

   context 'a subsequent user' do
-    let!(:first_user) { FactoryBot.create(:base_user) }
-    let!(:next_user) { FactoryBot.create(:base_user) }
+    let!(:first_user) { FactoryBot.create(:user) }
+    let!(:next_user) { FactoryBot.create(:user) }

-    it 'does not get the admin role' do
-      expect(next_user.persisted?).to eq true
+    it 'is not given the admin role' do
       expect(next_user).not_to have_role :admin
+      expect(next_user).not_to have_role :admin, Site.instance
     end
   end

@@ -37,7 +43,7 @@
     subject { FactoryBot.create(:admin) }

     it 'fetches the global roles assigned to the user' do
-      expect(subject.site_roles.pluck(:name)).to match_array ['admin', 'registered']
+      expect(subject.site_roles.pluck(:name)).to match_array ['admin']
     end
   end

@@ -45,11 +51,11 @@
     subject { FactoryBot.create(:user) }

     it 'assigns global roles to the user' do
-      expect(subject.site_roles.pluck(:name)).to match_array ['registered']
+      expect(subject.site_roles.pluck(:name)).to be_empty

-      subject.update(site_roles: ['admin', 'registered'])
+      subject.update(site_roles: ['admin'])

-      expect(subject.site_roles.pluck(:name)).to match_array ['admin', 'registered']
+      expect(subject.site_roles.pluck(:name)).to match_array ['admin']
     end

     it 'removes roles' do
@@ -58,4 +64,76 @@
       expect(subject.site_roles.pluck(:name)).to be_empty
     end
   end
+
+  describe '#hyrax_groups' do
+    subject { FactoryBot.create(:user) }
+
+    it 'returns an array of Hyrax::Groups' do
+      expect(subject.hyrax_groups).to be_an_instance_of(Array)
+      expect(subject.hyrax_groups.first).to be_an_instance_of(Hyrax::Group)
+    end
+  end
+
+  describe '#groups' do
+    subject { FactoryBot.create(:user) }
+
+    before do
+      FactoryBot.create(:group, name: 'group1', member_users: [subject])
+    end
+
+    it 'returns the names of the Hyrax::Groups the user is a member of' do
+      expect(subject.groups).to include('group1')
+    end
+  end
+
+  describe '#hyrax_group_names' do
+    subject { FactoryBot.create(:user) }
+
+    before do
+      FactoryBot.create(:group, name: 'group1', member_users: [subject])
+    end
+
+
+    it 'returns the names of the Hyrax::Groups the user is a member of' do
+      expect(subject.hyrax_group_names).to include('group1')
+    end
+  end
+
+  describe '#add_default_group_membership!' do
+    context 'when the user is a new user' do
+      subject { FactoryBot.build(:user) }
+
+      it 'is called after a user is created' do
+        expect(subject).to receive(:add_default_group_membership!) # rubocop:disable RSpec/SubjectStub
+
+        subject.save!
+      end
+    end
+
+    # #add_default_group_membership! does nothing for guest users;
+    # 'public' is the default group for all users (including guests).
+    # See Ability#default_user_groups in blacklight-access_controls-0.6.2
+    context 'when the user is a guest user' do
+      subject { FactoryBot.build(:guest_user) }
+
+      it 'does not get any Hyrax::Group memberships' do
+        expect(subject.hyrax_group_names).to eq([])
+
+        subject.save!
+
+        expect(subject.hyrax_group_names).to eq([])
+      end
+    end
+
+    context 'when the user is a registered user' do
+      subject { FactoryBot.build(:user) }
+
+      it 'adds the user as a member of the registered Hyrax::Group' do
+        expect(subject.hyrax_group_names).to eq([])
+
+        subject.save!
+
+        expect(subject.hyrax_group_names).to contain_exactly('registered')
+      end
+    end
+  end
```
</details>

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
Prior to this commit, it appears that knapsack's spec version was a copy
of a prior Hyku state's spec version.  When I ran the now deleted
Knapsack spec, it failed.  I copied over the updated Hyku spec ran it
and all things passed.

As such, the NavigationPresenter spec appears to be a duplicate and can
go away.

Related:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
In reviewing the diff, the specs for knapsack were a subset of the hyku
spec fo MenuPresenter.

I copied over the Hyku spec and ran them; all things passed so I feel
confident deleting the MenuPresenter spec.

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
In reviewing the WorkShowPresenter spec, Knapsack's was a subset of
the Hyku spec.  As such we don't need to keep this around as the specs
are vetted in Hyku.

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
This commit includes two things:

1. Favor Bulkrax::EntrySpecHelper provided by Bulkrax (for which the
   removed code was a prototype).
2. Use the existing spec to determine what the default work type should
   be (instead of relying on a random assignment based on the first
   element of the registered curation concerns)

And from this commit, we have a passing test!

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
- samvera/bulkrax#726
- samvera/bulkrax#719
- samvera/bulkrax#742
- samvera/hyku#1811
jeremyf referenced this issue Oct 4, 2023
I encourage you to read the code comments, reproduced here:

> Yes there's a duplicate for add_access_controls_to_solr_params; but
> that does not appear to be causing a problem like the duplication and
> order of the now removed additional :add_advanced_parse_q_to_solr,
> :add_advanced_search_to_solr filters.  Those existed in their current
> position and at the end of the array.
>
> When we had those duplicates, the :add_advanced_parse_q_to_solr
> obliterated the join logic for files.
>
> <2023-10-04 Wed> Oh how I wish I wrote a bit more back in the day.
> But alas, I didn't.  So I must rebuild that context.  Looking at the
> ":add_advanced_parse_q_to_solr obliterated the join logic" fragment,
> I'm assuming that what I meant was "I think that the
> 'show_works_or_works_that_contain_files' processor chain should come
> after the 'add_advanced_parse_q_to_solr' and
> 'add_advanced_search_to_solr' processor chains.
>
> This assumption is bolstered by the implementation of
> `Hyrax::CollectionMemberSearchBuilderDecorator` which introduces the
> `show_works_or_works_that_contain_files` method.  See
> https://github.com/samvera/hyku/blob/07fde572f9152d513b13f71cae90dd4fdfbfba6c/app/search_builders/hyrax/collection_member_search_builder_decorator.rb#L16-L34

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
jeremyf referenced this issue Oct 4, 2023
I encourage you to read the code comments, reproduced here:

> Yes there's a duplicate for add_access_controls_to_solr_params; but
> that does not appear to be causing a problem like the duplication and
> order of the now removed additional :add_advanced_parse_q_to_solr,
> :add_advanced_search_to_solr filters.  Those existed in their current
> position and at the end of the array.
>
> When we had those duplicates, the :add_advanced_parse_q_to_solr
> obliterated the join logic for files.
>
> <2023-10-04 Wed> Oh how I wish I wrote a bit more back in the day.
> But alas, I didn't.  So I must rebuild that context.  Looking at the
> ":add_advanced_parse_q_to_solr obliterated the join logic" fragment,
> I'm assuming that what I meant was "I think that the
> 'show_works_or_works_that_contain_files' processor chain should come
> after the 'add_advanced_parse_q_to_solr' and
> 'add_advanced_search_to_solr' processor chains.
>
> This assumption is bolstered by the implementation of
> `Hyrax::CollectionMemberSearchBuilderDecorator` which introduces the
> `show_works_or_works_that_contain_files` method.  See
> https://github.com/samvera/hyku/blob/07fde572f9152d513b13f71cae90dd4fdfbfba6c/app/search_builders/hyrax/collection_member_search_builder_decorator.rb#L16-L34

Related to:

- https://github.com/scientist-softserv/adventist-dl/issues/538
@jeremyf jeremyf closed this as completed Oct 5, 2023
@kirkkwang kirkkwang transferred this issue from scientist-softserv/adventist-dl May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

5 participants