AO3-6944 500 error when sorting by prompter with anon prompts

[ReverM]

Issue

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

Purpose

This fixes a 500 error bug when sorting by prompter with anonymous prompts.

Credit

Danaël / Rever ( they / he )

[brianjaustin]

Could you add a test for the bug behaviour (one that fails without your fix and passes with it)? Thanks!

Caveat for the 2 suggestions below: I have not had a chance to fully test them both, so it's possible they don't work or need some tweaks

[ReverM]

Could you add a test for the bug behaviour (one that fails without your fix and passes with it)? Thanks!

Caveat for the 2 suggestions below: I have not had a chance to fully test them both, so it's possible they don't work or need some tweaks

I wasn't sure how to implement test for this given the replication method. Is there a good way to emulate multiple users or force parameters?

Update: a test has been added. I added 2 new step definitions

[brianjaustin]

Thanks for the initial work on the test! We try to avoid "leaking" implementation details like the URL parameter in cucumber, though, so this is a better fit for a controller test, where it's easy to specify arbitrary parameters. (Aside: Rails does not encourage controller tests anymore, but we still use them, and for targetted scenarios like this, we still find them to be the best option.)

A couple example tests that specify parameters: https://github.com/otwcode/otwarchive/blob/a5c1f042d83f9a1caacbe8824d9e3d0cef58cec3/spec/controllers/works/works_controller_spec.rb

As there is no spec for this controller, please feel free to create one at spec/controllers/challenge_requests_controller_spec.rb and let us know if you have further questions!

[ReverM]

Thanks for the initial work on the test! We try to avoid "leaking" implementation details like the URL parameter in cucumber, though, so this is a better fit for a controller test, where it's easy to specify arbitrary parameters. (Aside: Rails does not encourage controller tests anymore, but we still use them, and for targetted scenarios like this, we still find them to be the best option.)

A couple example tests that specify parameters: a5c1f04/spec/controllers/works/works_controller_spec.rb

As there is no spec for this controller, please feel free to create one at spec/controllers/challenge_requests_controller_spec.rb and let us know if you have further questions!

I tried doing it via spec at first but got confused since the create is not defined in challenge_request_controller.rb. It seems like I'd need to pass through FactoryBot.create in order to populate the prompts to get the requested setting. That's the main reason I went towards cucumber, but I will change it to a spec test.

My only question would be if FactoryBot.create is the correct way to populate the prompts in order to test the behaviour?

[brianjaustin]

Yes, factorybot would be the correct way. Important note, however: Factorybot is automatically included in spec files, so you can just use create without the whole FactoryBot. at the beginning

[ReverM]

I am unsure if this replicate the situation I want to test (i.e I'm unsure if the request is getting correctly created), but other than that, I have added the test in spec form.

[brianjaustin]

Thank you for the test! For this sort of thing, the benefit of it being a bug is you can run the test against master and see that it doesn't pass. Unfortunately, in this case, the test does pass, so I think a bit more will be needed. To help with that, you can put binding.pry in any line of the controller or test to create a breakpoint, which will make iterating to set up exactly the right state a bit easier

[brianjaustin]

Based on https://github.com/otwcode/otwarchive/wiki/Automated-Testing#user-content-Our_RSpec_Standards, we would prefer something like

Suggested change

	it "should not throw a 500 error if sorting by prompter with an anonymous prompt" do
	it "does not throw a 500 error if sorting by prompter with an anonymous prompt" do

It might also be nice to use a context block for the background information like so

    context "when there are anonymous prompts" do
        it "does not throw an error when sorting by prompter" do
            # ...
        end
    end

[ReverM]

Thank you for the test! For this sort of thing, the benefit of it being a bug is you can run the test against master and see that it doesn't pass. Unfortunately, in this case, the test does pass, so I think a bit more will be needed. To help with that, you can put binding.pry in any line of the controller or test to create a breakpoint, which will make iterating to set up exactly the right state a bit easier

I have successfully changed the test so the correct state is reached. However, I cannot get it to fail even on master (I have confirmed that there is an anonymous prompt in the collection, and that the sort column is prompter). It seems that on master @requests is a WillPaginate::ActiveRecord instance, while on my fix it's the actual request. Could it be possible that spec tests do not go all the way through with respect to pagination?

[brianjaustin]

Ah yes, there's a bit extra to force rendering in controller tests. You can use the render_views declaration within the test: https://rspec.info/features/7-0/rspec-rails/controller-specs/render-views/. I tried running the test against master with that, and it fails like I would expect

[ReverM]

Ah yes, there's a bit extra to force rendering in controller tests. You can use the render_views declaration within the test: rspec.info/features/7-0/rspec-rails/controller-specs/render-views. I tried running the test against master with that, and it fails like I would expect

That was it! Thank you!

Test has been updated as requested (oops there might be a run indexer job needed)

[brianjaustin]

The first suggestion I've added should be enough to run the test fully; we have a hook to make rspec tests that require Elasticsearch easier

[brianjaustin]

It looks like more indexes are going to be needed, sorry 😞 You can see a list of the available hooks in https://github.com/otwcode/otwarchive/blob/master/spec/spec_helper.rb#L89 to tinker with -- I believe they should be stackable/combinable

[ReverM]

It looks like more indexes are going to be needed, sorry 😞 You can see a list of the available hooks in master/spec/spec_helper.rb#L89 to tinker with -- I believe they should be stackable/combinable

I do find it somewhat curious how the test work just fine when ran locally. What should be the best way for me to test that the test is going to work on master? I usually always have a terminal open to run elasticsearch. should I not do that?

[brianjaustin]

Locally, Elasticsearch is already configured, so you would need to delete the indexes to replicate how the environment looks in CI. I can't test this at the moment myself, but you should be able do to something like

Indexer.all.each(&:delete)

to force that locally. (If not, calling delete for each indexer -- like CollectionIndexer.delete -- should work)

[ReverM]

I can't seem to be able to replicate the test configuration, will undraft once I find the correct hook to add to make the test work

[ReverM]

Locally, Elasticsearch is already configured, so you would need to delete the indexes to replicate how the environment looks in CI. I can't test this at the moment myself, but you should be able do to something like

Indexer.all.each(&:delete)

to force that locally. (If not, calling delete for each indexer -- like CollectionIndexer.delete -- should work)

This seem to not be sufficient to make the test fail locally 😔

[brianjaustin]

Thank you, and nicely done!