Some more refactoring and testing of BulletTrain::LoadsAndAuthorizesResource

[aaricpittman]

This PR does the following:

• BulletTrain::LoadsAndAuthorizesResource#regex_to_remove_controller_namespace to a class method to match expectations in BulletTrain::LoadsAndAuthorizesResource.account_load_and_authorize_resource.
• Simplifies BulletTrain::LoadsAndAuthorizesResource.model_namespace_from_controller_namespace.
• Simplifies and adds some checks against undefined dependencies BulletTrain::LoadsAndAuthorizesResource#load_team.
• Adds some tests around these methods.

[jagthedrummer]

Hey @aaricpittman, this seems to break the starter repo tests that we run in CI. Looks like it breaks things very early in the init process and we don't ever get around to actually running tests. https://app.circleci.com/pipelines/github/bullet-train-co/bullet_train-core/1588/workflows/dcd633a8-a835-4e84-922a-4fe46a30867a

[aaricpittman]

@jagthedrummer Sorry, I saw the failures but hadn't had a chance to look into them. I was wondering if this would happen. It is failing because the abstract regex_to_remove_controller_namespace method is getting invoked now because it is defined at the appropriate level.

My guess is that it is an include order issue. I'll look into why and get it fixed.

[kaspth]

Hey thanks for working on this! I had some questions and suggestions for the changes here.

[kaspth]

Since we've inserted the return unless @team above, we can remove the try here:

Suggested change

	if @team.try(:persisted?)
	if @team.persisted?

[aaricpittman]

I'm not calling try for null safety. I'm calling it because there is nothing in this gem to guarantee that the value of @team responds to persisted?

[kaspth]

Seems like it'd be better to raise the NoMethodError exception with our expected interface to ease debugging, but this code path was already wrapped in try so we can keep it.

[kaspth]

Will this override an actual Current constant in apps? I wonder if this is related to why starter repo tests fail.

I wonder if we'd be better off with something like this in test_helper.rb:

# Define a fallback `Current` when we're running our tests isolated from the starter repo's `Current`.
unless defined?(::Current)
  class Current < ActiveSupport::CurrentAttributes
    attribute :team
  end
end

[jagthedrummer]

This may be one of those things where writing tests directly in the gem is somewhat lacking compared to writing tests in the starter repo where we have a more robust BT implementation that we can rely on.

[aaricpittman]

I've updated the test as @kaspth suggested.

I think having to write tests in another application or library because they won't work in the library where the code under-test is written is an indication of one or more software design problems.

bullet_train-super_load_and_authorize_resource has some dependency issues.

[aaricpittman]

For example, the code of this gem has an undeclared dependency on cancancan or, at a minimum, that the class calling account_load_and_authorize_resource defines a method load_and_authorize_resource.

[jagthedrummer]

Yeah there are definitely some undeclared dependencies and circular dependencies through a few of the BT packages. I have it on my list to try to sort that out soon.

[jagthedrummer]

@aaricpittman, if possible it might be nice to split this into some smaller PRs that each do one specific bit of refactoring. That way we could merge the bits that we can, and then deal with the tricky bits separately.

[aaricpittman]

@jagthedrummer / @kaspth made all the suggested changes and fixed the code so the tests pass.