Fix 'undefined constant RED' error + add tests

[cbliard]

I just got this error while using safer_rails_console today in development:

NameError: uninitialized constant SaferRailsConsole::Patches::Sandbox::AutoRollback::RED
from /home/cbliard/.rbenv/versions/2.5.3/lib/ruby/gems/2.5.0/gems/safer_rails_console-0.3.0/lib/safer_rails_console/patches/sandbox/auto_rollback.rb:15:in `handle_and_reraise_exception'

This PR should fix the error.

Tests added:

::SaferRailsConsole::Patches::Sandbox::AutoRollback
  .handle_and_reraise_exception
    when raising a PG::ReadOnlySqlTransaction exception
      outputs a message on stdout and forwards the exception
    when raising a classic exception
      rollbacks, begins a new transaction and forwards the exception

Please note that the first test in spec/safer_rails_console/console_spec.rb only passes because it is run before the tests I just added: I needed to load ::SaferRailsConsole::Patches::Sandbox::AutoRollback to test handle_and_reraise_exception method, but it makes the test "SaferRailsConsole::Console.initialize_sandbox loads sandbox patches" fail if it is run afterwards (Because it checks that the patch is not loaded before calling initialize_sandbox). That makes the test order-dependent and that's bad. Happy to fix it if you have some guidance here.

[timothysu]

Thanks so much for the PR! It's much appreciated. Let me know if you have any questions!

[timothysu]

I'd use the entire namespace from the root to be safe - ::SaferRailsConsole::Colors::RED. The fact that RED resolved correctly was serendipitous.

A better way to do this would be to include SaferRailsConsole::Colors instead of extend ..., ensuring that the constants are included into the metaclass. That'd involve a little bit of shuffling within Colors though, so no pressure and the first solution is totally adequate!

[cbliard]

I thought about including, but then the color_text won't be available.

[timothysu]

Yup, the shuffling I mentioned would involve putting that into a ClassMethods or similar module and using the popular self.included; extend ClassMethods pattern.

[timothysu]

Currently all the patches are integration tested in a real rails setup and console. We don't require anything within patches outside of that. See this context https://github.com/salsify/safer_rails_console/blob/master/spec/integration/patches/sandbox_spec.rb#L8 for more information. Feel free to help improve that by including the case where a normal exception is forwarded!

That being said, I'm totally for unit testing the patches. I'd probably update the test for SaferRailsConsole::Console.initialize_sandbox to the following:

  context ".initialize_sandbox" do
    before do
      allow(SaferRailsConsole::Console).to receive(:require)
    end

    it "loads sandbox patches" do
      described_class.initialize_sandbox

      expect(SaferRailsConsole::Console).to have_received(:require).with('safer_rails_console/patches/sandbox')
    end
  end

[cbliard]

I will do the proposed changes. Expect it on next Friday. Thanks for the feedback.

[cbliard]

I dug a little deeper about this issue, trying to implement it as an integration test. In fact it passes: the constant RED is defined in this case.

It's because the SaferRailsConsole::Colors is included in lib/safer_rails_console/consoles/irb.rb and makes RED available as a constant in the global scope.

I think I got the undefined constant RED because we are using pry for rails console in development and test (and irb in prod). With pry the file is not included. It's indicated in the README but I did not notice.

So I am not sure if my fix is relevant anymore. For me it looks like using safer_rails_console gem should not add color_text, RED, GREEN, and others to the global namespace. So the include SaferRailsConsole::Colors should be removed from lib/safer_rails_console/irb.rb and it should use the entire namespace, like SaferRailsConsole::Colors.color_text(text, SaferRailsConsole::Colors::RED).

What are your thoughts about it?

[timothysu]

Happy new year - apologies for the delay in response.

Thanks for taking a deeper look into it. I agree with not polluting the global namespace. The followup is to also generate a pry.rb, that would change the prompt similarly per _pry_.config.command_prefix = "%". The console configuration loading will also need to get updated so it loads the correct file (irb.rb vs pry.rb) given the REPL used - https://github.com/salsify/safer_rails_console/blob/master/lib/safer_rails_console/console.rb#L14 . I can add that in when I get a chance in the next week or two, unless you want to take a stab at it.

[cbliard]

Happy new year to you too!

I'll try to get a look this week. It should be easier for me as I am already using pry and can reproduce the issue at will. I'll try to add an integration test using pry too.

[OpakAlex]

can we merge it?