AO3-4862 Extending work controller test coverage by cesy · Pull Request #3888 · otwcode/otwarchive

cesy · 2020-08-19T07:36:58Z

Issue

https://otwarchive.atlassian.net/browse/AO3-4862

Purpose

Updated version of #3165

Please make sure Tal Hayon gets correct credit.

Also implemented show multiple for pseuds

- Added edit multiple works for pseud - Removed several dead code paths

cesy · 2020-08-19T08:31:10Z

        context "with restricted works" do
          before do
-            @work2 = create(:work, fandom_string: @fandom.name, restricted: true)
+            work2 = create(:work, fandom_string: @fandom.name, restricted: true)


It's not useless, we're checking it doesn't come up in the search results.

cesy · 2020-08-19T08:31:33Z

-          @fandom2 = create(:canonical_fandom)
-          @work2 = create(:work, fandom_string: @fandom2.name)
+          fandom2 = create(:canonical_fandom)
+          work2 = create(:work, fandom_string: fandom2.name)


Not useless, we're checking it's not in the results.

sarken

Thanks for updating this! I've left some comments, and I think this might need to be updated with master before Travis will successfully run. No rush, though!

tickinginstant · 2020-11-21T00:08:40Z

-          @fandom2 = create(:canonical_fandom)
-          @work2 = create(:work, fandom_string: @fandom2.name)
+          fandom2 = create(:canonical_fandom)
+          work2 = create(:work, fandom_string: fandom2.name)


The results run in a different scope, so these variables are inaccessible in the results. (Hence Hound's complaint.) I think let! would be good here.

tickinginstant · 2020-11-21T00:09:11Z

+        put :update_multiple, params: params
+      end
+
+      xit "redirects to the user multiple work path with an error" do


Why is this test disabled?

Also doesn't seem to have a reason so I've enabled it

Did you mean to reenable this? It's still xit.

It should probably also say "the edit user multiple works path" -- there are other user multiple works paths, e.g. show_multiple_user_works_path.

Yes, oops, should now be properly enabled, and I've fixed the name

cesy · 2021-06-16T13:38:57Z

I've fixed all the previous comments, can't currently figure out if the failing test is related to the changes.

sarken · 2021-06-16T23:28:46Z

        include "Invalid creator: Could not find a pseud *impossible*."
    end

+    it "renders new if edit is pressed" do


it "renders new if edit_button params" do, since that describes the code rather than the behavior and corresponds more closely with the phrasing below?

sarken · 2021-06-16T23:30:42Z


+    it "renders new if edit is pressed" do
+      work_attributes = attributes_for(:work).except(:posted)
+      # returns a failure unpermitted param :posted, so removed it following same method as above


I think you can leave this off -- if we didn't need an explanation in the other places we've used it, we don't need it here.

If you think it's necessary, the best move would probably be to either put a comment above the first use of .except(:posted) or all uses.

sarken · 2021-06-16T23:57:57Z

+    end
+
+    it "redirects to tags works page for noncanonical merged tags page" do
+      noncanonical_fandom = create(:fandom, canonical: false)


For both this example and the next, create(:fandom) makes noncanonical tags by default, so you should be able to do without canonical: false.

Also, can you set up the canonical/syn relationship right out of the gate by doing create(:fandom, merger_id: fandom.id) and eliminating the next two lines?

sarken · 2021-06-17T00:01:15Z

-    let(:update_work) {
-      work = create(:work, authors: [update_user.default_pseud])
+    let!(:update_work) do
+      work = create(:work, authors: [update_user.default_pseud], posted: true)


posted: true shouldn't be necessary here. The work factory should make posted works by default:

otwarchive/factories/works.rb

Lines 4 to 12 in c200136

factory :work do

title { "My title is long enough" }

fandom_string { "Testing" }

rating_string { ArchiveConfig.RATING_DEFAULT_TAG_NAME }

archive_warning_string { ArchiveConfig.WARNING_NONE_TAG_NAME }

language_id { Language.default.id }

chapter_info = { content: "This is some chapter content for my work." }

chapter_attributes { chapter_info }

posted { true }

sarken · 2021-06-17T00:08:29Z

@@ -0,0 +1,20 @@
+# frozen_string_literal: true


Let's call this file tags_spec.rb, so it could cover all the work actions with tags in the name, much like we have drafts_spec.rb for drafts and post_draft.

sarken · 2021-06-17T00:12:08Z

+        put :update_multiple, params: params
+      end
+
+      xit "redirects to the user multiple work path with an error" do


Did you mean to reenable this? It's still xit.

It should probably also say "the edit user multiple works path" -- there are other user multiple works paths, e.g. show_multiple_user_works_path.

sarken · 2021-06-17T00:34:48Z

+    When "AO3-3455" is fixed
+      # And I should see "classic"


This isn't right -- the tag shows up on the edit page, just not in the meta due to caching. (Also, fwiw, the next thing we're merging is #4014, which contains the fix for AO3-3455.)

The failure might be because you need to check the contents of the Additional Tags field, which is done with a different step than checking text outside a form field. You could also try enabling JavaScript for this test instead, which might drag the tag out of the form field.

sarken · 2021-06-17T00:56:07Z

      end
    end

+    context "when work parameters are invalid" do


I'm kind of thinking we should set this up in a way that'll make it easier to add coverage of other invalid work params in the future, e.g.

context "when work params are invalid" do context "when summary is invalid" do context "when some other param is invalid" do context "when yet another param is invalid" do

... but now that I'm looking at the edit multiple work page, summary isn't one of the parameters you're meant to edit, so I'm a little confused what we're actually try to test here.

Is this meant to test passing an unpermitted work parameter (e.g., a summary), or a permitted parameter with an invalid value (e.g., a visibility setting that doesn't exist)?

I'm not sure what the original developer was trying to test, but checking both of those sounds like a good idea to me. I'll have a go.

It looks like an unpermitted work parameter actually works fine, which is a minor loophole if people want to be editing their form submissions manually, but not a big security risk since it still goes through the normal work controller checks. And a permitted parameter with an invalid value gives an error in the controller rather than a friendly error page, but that's okay because the form doesn't let you do that unless you edit the form submission manually anyway. So I'm going to scrap this test as it requires coding skills to do either rather than being something a user normally could do.

sarken · 2021-06-17T01:13:13Z

+      expect(response).to redirect_to(tag_path(noncanonical_tag))
+    end
+
+    it "redirects to tags works page for noncanonical merged tags page" do


Should this example and the one on line 357 be tested under the without caching section, since we already have the same tests in the with caching section?

otwarchive/spec/controllers/works/default_rails_actions_spec.rb

Lines 456 to 474 in 584c4da

context "when tag is a synonym" do

let(:fandom_synonym) { create(:fandom, merger: fandom) }

it "redirects to the merger's work index" do

params = { tag_id: fandom_synonym.name }

get :index, params: params

it_redirects_to tag_works_path(fandom)

end

context "when collection is specified" do

let(:collection) { create(:collection) }

it "redirects to the merger's collection works index" do

params = { tag_id: fandom_synonym.name, collection_id: collection.name }

get :index, params: params

it_redirects_to collection_tag_works_path(collection, fandom)

end

end

end

It would also makes sense to use the same descriptions for the new tests that we used for the existing tests. That will make it clearer the examples just cover the same thing in different contexts. (If you could change the existing describe "with caching" and describe "without caching" to context instead, that would be great.)

sarken · 2021-06-17T01:27:51Z

+            allow(controller).to receive(:fetch_admin_settings).and_call_original
+          end
+
+          it "shows results when filters are disabled" do


Let's describe this as it "returns the work" do. That will

match the description of the related example on line 332

get rid of the incorrect part about filters being disabled (we're testing when filter counts are suspended, which is made clear by the fact this is inside context "when suspend_filter_counts is on" do)

I've tweaked slightly further to "shows the work in the index", and also clarified the description a couple of lines down.

cesy · 2021-06-18T16:02:06Z

When I come back to this, I have one cucumber test still to fix, the rest of the comments have been addressed.

sarken · 2023-07-14T00:18:50Z

Hi, Cesy! Someone pointed out this has been in draft for two years, so I wanted to check if you'd be picking this back up soon. If not, no worries, we can close it until you're ready.

cesy · 2023-07-14T17:27:02Z

I'd still be up for it if you think it would be useful? I imagine better automated tests are still a good thing.

sarken · 2023-07-14T22:40:01Z

Yeah, more test coverage is always good!

Bilka2 · 2026-08-01T13:49:02Z

Adopted at #5959 based on the policy for stalled pull requests

hatal175 and others added 8 commits August 6, 2020 14:51

AO3-4855 Started working on extending works controller tests

4fd3d51

Also implemented show multiple for pseuds

AO3-4862 Extended work controller test coverage

956eaed

- Added edit multiple works for pseud - Removed several dead code paths

AO3-4862 Fix rspec tests

6e99707

AO3-4862 Remove fixes to be done later in other pull requests

c0e42ab

AO3-4862 Merging latest master

948c8b8

AO3-4862 Fixing test failures

d8fda5d

AO3-4862 Removing bits that don't work

dd73d02

AO3-4862 Making Hound happier

bb3cace

cesy commented Aug 19, 2020

View reviewed changes

cesy added 3 commits August 19, 2020 09:33

AO3-4862 Making Hound happier

3042982

AO3-4862 Making Hound happier

32f7739

AO3-4862 Fixing my mistakes from Hound

6ce011e

sarken added the Awaiting Review label Aug 19, 2020

sarken requested changes Nov 10, 2020

View reviewed changes

sarken added Reviewed: Action Needed Scope: Tests Only Only changes automated tests or test configuration and removed Awaiting Review labels Nov 10, 2020

tickinginstant reviewed Nov 21, 2020

View reviewed changes

cesy added 6 commits June 16, 2021 10:40

AO3-4862 review comments

6f61328

AO3-4862 for Hound

1b294be

Make Hound happier

707d827

AO3-4862 fixing test failures

09a49ba

AO3-4862 test failures

d3615dd

One more for Hound

584c4da

cesy requested a review from sarken June 16, 2021 13:38

sarken added Coder Has Actioned Review and removed Reviewed: Action Needed labels Jun 16, 2021

sarken requested changes Jun 17, 2021

View reviewed changes

sarken added Reviewed: Action Needed and removed Coder Has Actioned Review labels Jun 17, 2021

cesy added 3 commits June 17, 2021 11:02

AO3-4862 review comments

691e704

AO3-4862 review comments and expanding update multiple spec

055121a

AO3-4862 removing invalid test

77e8516

cesy marked this pull request as draft June 18, 2021 16:02

Bilka2 mentioned this pull request Aug 1, 2026

AO3-4862 Extend works controller test coverage #5959

Open

Bilka2 closed this Aug 1, 2026

	factory :work do
	title { "My title is long enough" }
	fandom_string { "Testing" }
	rating_string { ArchiveConfig.RATING_DEFAULT_TAG_NAME }
	archive_warning_string { ArchiveConfig.WARNING_NONE_TAG_NAME }
	language_id { Language.default.id }
	chapter_info = { content: "This is some chapter content for my work." }
	chapter_attributes { chapter_info }
	posted { true }

	context "when tag is a synonym" do
	let(:fandom_synonym) { create(:fandom, merger: fandom) }

	it "redirects to the merger's work index" do
	params = { tag_id: fandom_synonym.name }
	get :index, params: params
	it_redirects_to tag_works_path(fandom)
	end

	context "when collection is specified" do
	let(:collection) { create(:collection) }

	it "redirects to the merger's collection works index" do
	params = { tag_id: fandom_synonym.name, collection_id: collection.name }
	get :index, params: params
	it_redirects_to collection_tag_works_path(collection, fandom)
	end
	end
	end

Uh oh!

Conversation

cesy commented Aug 19, 2020

Issue

Purpose

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cesy commented Jun 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cesy commented Jun 18, 2021

Uh oh!

sarken commented Jul 14, 2023