revert controllers to inherit from including app ApplicationController

[elrayle]

Also…

• set up 2.1.1 bugfix release
• fix Error in fixture lexvo_snippet

[jrochkind]

I might write < ::ApplicationController to avoid confusion -- this is the top-level application-provided ApplicationController, which is what the code in this PR is actually doing (I'm pretty sure!), but might be clearer to write it explicitly.

After this change, I don't believe the Qa::ApplicationController will actually be used at all. With the exception of the method you turned into a class-method so it could be called externally. Should Qa::ApplicationController be removed from the gem entirely, if it's not going to be used? Leaving "dead" code in can be confusing for future code readers.

At first it was a mystery why this change was required to keep working with hyrax. But after some investigation, I see that in 2.0.0/2.0.1, these controllers originally inherited from app-provided top-level ApplicationController. https://github.com/samvera/questioning_authority/blob/v2.0.1/app/controllers/qa/linked_data_terms_controller.rb

The Qa::ApplicationController was entirely blank in 2.0.0/2.0.1, and my guess is not actually used by any code in QA, it could have been deleted and nothing would have broken. https://github.com/samvera/questioning_authority/blob/v2.0.1/app/controllers/qa/application_controller.rb

In 2.1.0, the controllers were changed to inherit from Qa::ApplicationController. In this commit, as part of CORS implementation. https://github.com/samvera/questioning_authority/commit/7dbf2cb084c24bbed75d8334ae609c0c6859e447

So we now realize the change in that commit introduced a bug -- changing between inheriting from ::ApplicationController and Qa::ApplicationController is potentially a backwards incompat -- but it already happened in 2.1.0, and this PR is reversing it, to fix the backwards breaking change. OK, so by that argument it's not a backwards breaking change to change it again now, instead this is fixing a mistaken change in 2.1.0, presumbly to be quickly released as 2.1.1. Great!

With this PR reversing that change, I would also reverse the changes to Qa::ApplicationController. I believe that post this PR Qa::ApplicationController goes back to not actually being used by any code in the app. Except for the method this PR makes a class method so it can be called by other controllers -- it can easily be relocated anywhere. I would suggest returning Qa::ApplicationController to the empty state it was in prior to https://github.com/samvera/questioning_authority/commit/7dbf2cb084c24bbed75d8334ae609c0c6859e447 -- or even deleting the class entirely. The other methods in there that are not the class methods are probably not being used, and dead code is confusing for future code readers.

[elrayle]

@jrochkind I agree with almost all of your analysis. Except... 'this PR Qa::ApplicationController goes back to not actually being used by any code in the app'. The two methods in Qa::ApplicationController have been confirmed to be used for the cors headers in QA tests and in an application that includes QA.

[jrochkind]

I don't see how it's possible any the options instance method in Qa::ApplicationController can possibly be used by controllers that don't inherit from it. I am pretty certain that if you delete that method, all your tests will still pass. It's dead code. Unless there is some controller somewhere inheriting Qa::ApplicationController base class we haven't yet mentioned?

The self.cors_allow_origin_header class method can be and is used... but that class method can be defined anywhere, it doesn't need to be in Qa::ApplicationController. I suppose if you think that's the best place for it, it can be left there. But I'm pretty sure the options method is dead code.

[elrayle]

Tweaks based on feedback...

Made a few tweaks based on @jrochkind feedback.

• changed inheritance on controllers (except ApplicationController) to ::ApplicationController to make it clearer that these controllers will inherit from the application that includes the gem
• removed require_dependency 'qa/application_controller' as it didn't appear to be doing anything

Why Qa::ApplicationController is needed...

Qa::ApplicationController is used in two ways

• delegate :cors_allow_origin_header, to: Qa::ApplicationController in each controller (new in this commit)
• routes.rb holds route definitions for OPTIONS method on each path. They are all sent to Qa::ApplicationController #options method (defined in 2.1.0)

[elrayle]

Ran all Hyrax tests against this branch. 0 failures.

[jrochkind]

Thanks for clarifying. I think there could potentially be other preferable places to put that code than Qa::ApplicationController, as it's not the normal use of an engine::ApplicationController. But it's not totally unreasonable, I don't consider it a blocker. Copying that langauge you wrote here with the two ways QA::ApplicationController is used into a comment at top of the QA::ApplicationController file might be helpful.

I don't consider any of these blockers, but would be more comfortable with additional review from someone more experienced with community code practices, rather than supplying the approval endorsement myself. But if you can't get anyone else to, I guess I could!

[elrayle]

Added yarddoc to Qa::ApplicationController methods to document their usage.