Discussing the API for request variables, part 2 #1033

jaredbeck · 2017-12-30T06:52:56Z

In #1016, Alex and I discussed a new API for request variables.

Current request variables

.. copied from #1016 for convenience:

key	versions table	api
:request_enabled_for_controller	no	public
:"enabled_for_#{model}"	no	public
:whodunnit	yes	public
:controller_info	yes	public
:transaction_id	yes	private

Some keys are data, to be saved in the versions table, others are configuration. Most are public, but :transaction_id is private. The only thing they have in common is that they affect the current request.

API Goals

Alex would like a convenient way to set a request variable temporarily, for which I propose PaperTrail.request(whodunnit: 'Alex') do .. end.
I have always wanted a better way to communicate which PT settings affect the current request vs. the whole thread, vs. all threads, and deprecating eg. PaperTrail.whodunnit in favor of PaperTrail.request.whodunnit communicates this unambiguously.

Let me know what you think, Alex. And @batter as always I'd appreciate your opinion too.

[ci skip]

jaredbeck · 2018-02-13T17:32:20Z

@alexevanczuk and @batter please review, thanks!

alexevanczuk · 2018-02-13T19:58:16Z

Thanks @jaredbeck! I should be able to look at this this week.

alexevanczuk

Lots of questions and comments.

Also, could you add some more test coverage for the new code? Particularly around making sure that request works when calling with a block, and will properly reset values in different situations to prevent regressions. For example, a context where the original code raises an error, to test ensure is properly called, or that different attribute values work correctly (what if controller info is a nested hash, or passes in objects of different types).

Thanks for doing this, overall looks great and will allow a lot more flexibility using paper trail without sacrificing simplicity or code readability.

alexevanczuk · 2018-02-14T20:35:03Z

lib/paper_trail/request.rb

+      # Returns a deep copy of the internal hash from our RequestStore.
+      # Keys are all symbols. Values are mostly primitives, but
+      # whodunnit can be a Proc.
+      # @api private


This is a private method, but it is called in PaperTrail#request. I am not sure how you think about private methods (is it just for gem clients, or for service classes within the gem)? In either case, I think that def request when called with options should pass the block to Request and the request itself knows how to set config for the duration of the block.

This is a private method, but ..

It's not private, it's # @api private :) The former being a language feature and the latter being a somewhat conventional way of commenting on which methods are subject to change without warning.

In either case, I think that def request when called with options should pass the block to Request and the request itself knows how to set config for the duration of the block.

Sounds good, I'll try that.

alexevanczuk · 2018-02-14T20:37:05Z

lib/paper_trail.rb

@@ -46,7 +48,11 @@ module PaperTrail
  class << self
    # @api private
    def clear_transaction_id


Should this and other # @api private methods, such as transaction_id=, have a deprecation warning? Is your rationale that even if they're private, clients might be using them, and we want to deprecate them in the same way?

Going forward, all transaction id methods should be @api private. I'll fix that in the depr. warnings and comments.

alexevanczuk · 2018-02-14T20:45:13Z

lib/paper_trail.rb

+          "Passing a block to PaperTrail.whodunnit is deprecated, " \
+          'use PaperTrail.request(whodunnit: "John") do .. end'
+        )
+        before = request.whodunnit


Why not use method as suggested in the deprecation warning?

e.g.

request(whodunnit: value) yield end

Good suggestion, thanks.

alexevanczuk · 2018-02-14T20:47:42Z

lib/paper_trail.rb

    # @api public
-    def controller_info
-      paper_trail_store[:controller_info]
+    def controller_info(value = nil, &block)


Why did you add these parameters? They aren't used in Request. The Request method signature is def controller_info. Wouldn't you get a ArgumentError: wrong number of arguments if you call this? If so, can you add test coverage for this?

Seems like a mistake. I didn't mean to change the method signature. Good catch.

alexevanczuk · 2018-02-14T20:48:45Z

lib/paper_trail.rb

@@ -174,21 +204,20 @@ def transaction?
      ::ActiveRecord::Base.connection.open_transactions.positive?
    end

-    # @api public
+    # @api private


Is this intentional? Again I'm not sure how you like to do this maintenance-wise, but I think that maybe we should add a deprecation notice that this is no longer a public method.

Yes, intentional. I'll be changing the deprecation warning to say "deprecated without replacement".

alexevanczuk · 2018-02-14T20:49:55Z

lib/paper_trail/request.rb

+      # whodunnit can be a Proc.
+      # @api private
+      def to_h
+        store.store.deep_dup


I think that store.store is a little confusing. Maybe something should have a different name here.

alexevanczuk · 2018-02-14T20:51:16Z

lib/paper_trail.rb

+      else
+        begin
+          before = Request.to_h
+          Request.set(options)


Do we want to limit what options are settable, and perhaps validate the options? If they set:

PaperTrail.request(some_nonexistant_option: true) do ... end

What should happen?

What if they try to set a private attribute? Such as transaction_id?

Do we want to limit what options are settable, and perhaps validate the options? If they set .. [some_nonexistant_option] .. What should happen?

Good point, we should raise an error.

What if they try to set a private attribute? Such as transaction_id?

We can raise an error for that too.

alexevanczuk · 2018-02-14T20:51:43Z

lib/paper_trail/version_number.rb

@@ -9,7 +9,7 @@ module PaperTrail
  module VERSION
    MAJOR = 8
    MINOR = 1
-    TINY = 1
+    TINY = 0


Shouldn't this be going in the other direction?

jaredbeck · 2018-02-19T03:03:25Z

@alexevanczuk and @batter ready for another review, thanks!

airblade · 2018-02-19T09:49:35Z

Looks great!

alexevanczuk

I had some comments mostly around testing. I re-compared the delegated methods in lib/paper_trail.rb, lib/paper_trail/model_config.rb, and lib/paper_trail/record_trail.rb to the new methods in lib/paper_trail/request.rb, and things look good. However, I'd still feel more comfortable if we added delegation tests for each of the methods moved in the original file, as well as functionality tests for each of the methods moved to lib/paper_trail/request.rb in the spec for the request module.

Not sure what your customs are around proving PRs, but we typically have one person sign off and another click approve, so I'll let @batter officially accept.

alexevanczuk · 2018-02-19T17:37:56Z

README.md

+
+#### Setting `whodunnit` Temporarily
+
+To set whodunnit temporarily, for the duration of a block, use


Maybe consider adding the other supported fields that can be set in the block in the readme?

alexevanczuk · 2018-02-19T18:00:35Z

lib/paper_trail/record_trail.rb

@@ -485,25 +497,25 @@ def without_versioning(method = nil)
        yield @record
      end
    ensure
-      @record.class.paper_trail.enable if paper_trail_was_enabled
+      PaperTrail.request.enable_model(@record.class) if paper_trail_was_enabled
    end

    # Temporarily overwrites the value of whodunnit and then executes the
    # provided block.
    def whodunnit(value)


Why is this method here? Shouldn't it be deprecated in favor of using PaperTrail.request.with(whodunnit: value) do ... end?

Good catch. Yes, I will deprecate it.

alexevanczuk · 2018-02-19T18:02:53Z

spec/paper_trail/request_spec.rb

+    end
+
+    describe ".with" do
+      context "block given" do


Can you add tests for the validator here through the public with test? That is, test that whodunnit, enabled for work (maybe enabled for is a good new test case since it's a regex), transaction id, and an invalid option.

Also can you change the initial value to be non nil so we can test something more useful, e.g. instead of expect(described_class.whodunnit).to be_nil, we can have expect(described_class.whodunnit).to 'previous_whodunnit

alexevanczuk · 2018-02-19T18:08:02Z

spec/paper_trail_spec.rb

@@ -94,37 +94,8 @@
  end

  describe ".version" do
-    it { expect(described_class).to respond_to(:version) }


I'd probably prefer to have some delegation/deprecation tests, e.g. to test deprecation issues such as changing the method signature to controller_info.

This could be done tersely with a shared example if you want to write less code:

shared_example 'it delegates to request' do |method, *args, &block| it do expect(PaperTrail::Request).to receive(:method).with(args, &block).and_return(:some_return_value) expect(described_class.send(:method, args, &block).to_return(:some_return_value) end end

it_behaves_like 'it delegates to request', :controller_info, [], nil it_behaves_like 'it delegates to request', :whodunnit, [:some_whodunnit] { 'some_block' }

Not sure if the above will work exactly as is, but that's the idea. Could be just as straightforward to do it more explicitly for each method, but I think it would be less error-prone this way.

alexevanczuk · 2018-02-19T18:10:53Z

spec/paper_trail/request_spec.rb

+require "spec_helper"
+
+module PaperTrail
+  ::RSpec.describe(Request, versioning: true) do


If you have the time, I'd probably prefer to have the other @api public methods tested as well here, such as controller_info, controller_info=, enabled_for_controller?, enabled_for_controller=, and whodunnit=.

alexevanczuk · 2018-02-19T18:11:02Z

spec/paper_trail/request_spec.rb

+    end
+
+    describe ".whodunnit" do
+      context "when set to a proc" do


Can you add a context when not set to a proc?

batter · 2018-02-21T04:42:41Z

README.md


 ```ruby
-PaperTrail.whodunnit = proc do
-  caller.first{ |c| c.starts_with? Rails.root.to_s }
+PaperTrail.request.with(whodunnit: 'Dorian Marié') do


I like the simplicity of this

batter · 2018-02-21T04:44:54Z

lib/paper_trail.rb

@@ -46,124 +47,158 @@ module PaperTrail
  class << self
    # @api private


Does this comment actually do anything or it's merely a note to help developers remember which methods are public / private?

It specifies whether something is part of our public API, ie. whether we would consider changing it to be a breaking change. Notably, not all public methods are public API, if that makes sense.

batter · 2018-02-21T04:48:17Z

lib/paper_trail/frameworks/cucumber.rb

-  PaperTrail.controller_info = {} if defined? Rails
+  PaperTrail.request.enabled_for_controller = true
+  PaperTrail.request.whodunnit = nil
+  PaperTrail.request.controller_info = {} if defined? Rails


Should this be if defined?(::Rails) to be consistent with the RSpec helper? I don't think there is a Rails module in this library yet but there could be one day

Sure, I'm always happy to use absolute constant paths, it's one less thing to worry about.

batter · 2018-02-21T04:53:08Z

My first impression is that I like it and it seems like an eloquent solution that makes the gem nicely flexible to configure in a thread-safe manner or across all threads depending on what the user desires for various conditions. I'd like to spend some more time unpacking all the changes, will try to check back to do a deeper review later this week.

jaredbeck · 2018-02-22T01:40:38Z

Thanks everyone for the reviews!

Alex, I'd really appreciate your help writing some more tests. Would you like to? You can make a PR into this one, eg. "Alex wants to merge .. commits into request_variables from alex_branch" Thanks!

alexevanczuk · 2018-02-25T01:28:23Z

@jaredbeck

Added specs: #1052

jaredbeck · 2018-02-26T03:58:01Z

I'd like to spend some more time unpacking all the changes, will try to check back to do a deeper review later this week.

Ben, I think I'm ready to merge this. Do you still want to do another review?

Andy, can you un-protect this branch? I want to do a rebase and tidy up my commits, but force-pushing is disabled. I'd prefer if only master and the stable branches (eg. 8-stable, 7-stable) were protected. Thanks.

jaredbeck · 2018-02-27T04:55:39Z

Closing via #1056 which uses a non-protected branch, allowing me to rebase and squash prior to final merge to master.

batter · 2018-03-01T19:50:04Z

LGTM, I'm sure there will be time to tweak functionality if needed before a release. Nice work by @alexevanczuk and @jaredbeck!

jaredbeck · 2018-03-01T22:25:18Z

LGTM, I'm sure there will be time to tweak functionality if needed before a release. Nice work by @alexevanczuk and @jaredbeck!

Thanks Ben! Yeah, I'm planning on releasing 9.0.0 in a few weeks, as soon as ruby 2.2 EoL is announced (https://www.ruby-lang.org/en/news/2017/09/14/ruby-2-2-8-released/)

alexevanczuk and others added 2 commits December 30, 2017 00:14

A new API for request variables

14e00a3

Second draft of the new Request API

32ea02a

jaredbeck added the Not a bug label Dec 30, 2017

jaredbeck added this to the 9.0.0 milestone Dec 30, 2017

jaredbeck added 2 commits February 13, 2018 12:01

Merge branch 'master' into request_variables

b338260

Add changelog entry (squash me)

5c039d4

[ci skip]

alexevanczuk suggested changes Feb 14, 2018

View reviewed changes

jaredbeck and others added 7 commits February 18, 2018 19:45

Grooming the 9.0.0 changelog entry

40a41a0

[CI] Test against Ruby 2.5

3e8004f

Test 2.3 and 2.5, but not 2.4

12ec54c

Working on new API for request vars

6e7df2c

Merge branch 'master' into request_variables

40930ee

Improve stack trace in deprec. warnings

da57563

Use new request API internally, organize tests

446c654

jaredbeck mentioned this pull request Feb 19, 2018

PT 9 will have a new API for whodunnit ankit1910/paper_trail-globalid#7

Closed

Continue working on new request vars API

142cf20

alexevanczuk reviewed Feb 19, 2018

View reviewed changes

batter reviewed Feb 21, 2018

View reviewed changes

Alex Evanczuk and others added 3 commits February 25, 2018 21:40

Add specs to request variables branch (#1052)

8a79be3

gitignore *.sqlite3-journal

b7deb8c

Deprecate RecordTrail#whodunnit

957ca1f

Prefer absolute constant paths

747b936

jaredbeck mentioned this pull request Feb 27, 2018

A new API for request variables, eg. whodunnit #1056

Merged

jaredbeck closed this Feb 27, 2018

jaredbeck mentioned this pull request Mar 14, 2018

MyModel.paper_trail.disable should take a block #916

Closed

jaredbeck deleted the request_variables branch August 27, 2018 01:08


		#### Setting `whodunnit` Temporarily

		To set whodunnit temporarily, for the duration of a block, use

		@@ -46,124 +47,158 @@ module PaperTrail
		class << self
		# @api private

Discussing the API for request variables, part 2 #1033

Discussing the API for request variables, part 2 #1033

Conversation

jaredbeck commented Dec 30, 2017

Current request variables

API Goals

jaredbeck commented Feb 13, 2018

alexevanczuk commented Feb 13, 2018

alexevanczuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredbeck commented Feb 19, 2018

airblade commented Feb 19, 2018

alexevanczuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

batter commented Feb 21, 2018

jaredbeck commented Feb 22, 2018

alexevanczuk commented Feb 25, 2018

jaredbeck commented Feb 26, 2018

jaredbeck commented Feb 27, 2018

batter commented Mar 1, 2018

jaredbeck commented Mar 1, 2018