Hotfix for support of Rails 7.2.0 #1485

kalashnikovisme · 2024-08-10T13:39:49Z

This is just a hotfix. I didn't make big changes. So, the core team of the gem should make these updates:

Change using of ActiveSupport::Deprecation._instance.warn. Rails contributors suggest creating own deprecation class. More here
Review the use of schema_migration in tests. Looks like, I made the needed change, but maybe I didn't understand the purpose of the test.
Decide what to do with Ruby 3.0 tests. Rails 7.2.0 does not support this version, so you should completely change test workflow. I temporarily removed it.

Check the following boxes:

Wrote good commit messages.
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry to the Changelog if the new
code introduces user-observable changes.
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.
Decreased line coverage by 0.07% - sorry 🙂

fatkodima · 2024-08-11T22:33:40Z

Duplicate of #1477.

But either version should be merged, since we already have rails 7.2.

olivierbuffon · 2024-08-14T15:20:59Z

This is the last blocker for our app to be upgraded to 7.2, can wait to have this merged 🙏🏻 ! 😃

fatkodima · 2024-08-14T15:29:59Z

Same for us, the only blocker thats left.

arikarim · 2024-08-15T07:20:24Z

any update on this please?

ronnqvistandreas · 2024-08-15T11:38:34Z

lib/paper_trail/compatibility.rb

@@ -18,7 +18,7 @@ module PaperTrail
  # versions.
  module Compatibility
    ACTIVERECORD_GTE = ">= 6.1" # enforced in gemspec
-    ACTIVERECORD_LT = "< 7.2" # not enforced in gemspec
+    ACTIVERECORD_LT = "< 8" # not enforced in gemspec


This should most likely be 7.3, as we're cautious with Rails introducing breaking changes in minor versions.

There won't be 7.3, only 8 as a next.

Nice catch!

I decided to use 8 because looks like there won't be Rails 7.3.

We can see it here and here

Anyway, changed it 🙂

So many people wait for the update 🙂

denysivanov · 2024-08-19T12:37:18Z

Any update on this?

ryansdwilson · 2024-08-19T14:19:13Z

Stoked to see this merge. Let us know if we can help!

Halphas · 2024-08-19T17:36:36Z

please merge

ronnqvistandreas

Looks good to me.

kalashnikovisme · 2024-08-19T21:15:18Z

@jaredbeck hey! Sorry for the mention. Could you please help us and merge it or add comments 🙂

chrisjeon · 2024-08-20T12:44:53Z

Can we get this in?

benkruger · 2024-08-20T15:51:43Z

Why is it taking so long to merge this?

denysivanov · 2024-08-21T14:00:15Z

@jaredbeck could you help?

cisolarix · 2024-08-22T03:27:42Z

Looking forward to seeing this PR being merged.

dip00dip · 2024-08-22T07:27:06Z

Are there any blockers to merge this?

denysivanov · 2024-08-23T14:01:09Z

Any update here?

veganstraightedge · 2024-08-28T19:39:45Z

Hi paper trail gem folks 👋🏻 i'm adding my voice to the chorus of encouragement to merging this PR or the other one #1477

zhandao · 2024-08-29T01:55:04Z

We've been waiting for this PR for a long time. 😭

capripot · 2024-08-29T09:55:28Z

Could we release a beta to test it out in larger scale than using this branch directly?

joshRpowell · 2024-08-29T16:33:53Z

.github/workflows/test.yml


        # To keep matrix size down, only test highest and lowest rubies.
        # See "Lowest supported ruby version" in CONTRIBUTING.md
-        ruby: [ '3.0', '3.2' ]
+        ruby: [ '3.2', '3.3' ]


should this be updated?

ruby : [ '3.1', '3.3']

see https://endoflife.date/ruby
per https://github.com/paper-trail-gem/paper_trail/blob/master/.github/CONTRIBUTING.md#lowest-supported-ruby-version

Nice catch! Thanks!

@kalashnikovisme looks like the following need updating as well according to CONTRIBUTING:

paper_trail/paper_trail.gemspec

Line 50 in be31c66

s.required_ruby_version = ">= 3.0.0"

paper_trail/.rubocop.yml

Line 26 in be31c66

TargetRubyVersion: 3.0

paper_trail/.github/workflows/test.yml

Line 20 in be31c66

ruby-version: '3.0'

@joshRpowell is right. This 3 files should be updated, @kalashnikovisme . And why have you removed 3.2?

joshRpowell · 2024-08-29T16:38:59Z

lib/paper_trail/compatibility.rb

@@ -18,7 +18,7 @@ module PaperTrail
  # versions.
  module Compatibility
    ACTIVERECORD_GTE = ">= 6.1" # enforced in gemspec
-    ACTIVERECORD_LT = "< 7.2" # not enforced in gemspec
+    ACTIVERECORD_LT = "< 7.3" # not enforced in gemspec


8.0 ?

ACTIVERECORD_LT = "< 8.0"

see https://github.com/rails/rails/milestones

It was set to < 8 originally but there was a request to set to < 7.3 here.

joshRpowell · 2024-08-29T22:44:02Z

README.md

@@ -92,7 +92,7 @@ Choose version:

 | paper_trail | ruby     | activerecord  |
 |-------------|----------|---------------|
-| unreleased  | >= 3.0.0 | >= 6.1, < 7.2 |
+| unreleased  | >= 3.0.0 | >= 6.1, <= 7.2 |


@kalashnikovisme and >= 3.1.0

Got it 🙂 Thanks!

bvogel · 2024-09-02T08:01:10Z

Now that #1477 has been closed, I'd really prefer their definition for the deprecation object in the PR rather than relying on some internal method that isn't part of the official API AFAIK. could we recover that piece of code to this PR?

@bvogel

As per the comment from @bvogel here: paper-trail-gem#1485 (comment)

tbrammar · 2024-09-03T12:29:37Z

@kalashnikovisme I've created a PR on your fork to adjust the deprecator definition as per @bvogel request above.

If you're good with it can we merge and then hopefully the Paper Trail Gem folks can then get this PR merged? 🙏👍

Adjust deprecator definition to be more inline with the official API

kalashnikovisme · 2024-09-03T21:11:33Z

Merged it!

thanks @tbrammar 🙂

samward · 2024-09-04T08:54:43Z

spec/paper_trail/compatibility_spec.rb

@@ -14,7 +14,7 @@ module PaperTrail

      context "when incompatible" do
        it "writes a warning to stderr" do
-          ar_version = ::Gem::Version.new("7.2.0")
+          ar_version = ::Gem::Version.new("8.0.0")


Should this be 7.3 in line with the other changes?

Suggested change

ar_version = ::Gem::Version.new("8.0.0")

ar_version = ::Gem::Version.new("7.3.0")

It's a pretty interesting question.

I think no because there is > method in other places to compare versions. But in this place we have exact version to test with.

At the first version of this pull request, I used 7.3 because of these commits in the rails repository here and here. But I changed it to 8.0 because of this comment.

So, I think that we should use 7.3 in cases with >.

kalashnikovisme · 2024-09-04T23:54:50Z

I tested it with Rails 7.2.1. Everything works fine.

Migoo · 2024-09-05T07:49:15Z

Hello everyone,
Thanks for the job.

I try this PR and I have some issue in tests related to Papertrail.

we have the following helper:

 module Versioning
  def with_versioning
    was_enabled = PaperTrail.enabled?
    was_enabled_for_request = PaperTrail.request.enabled?
    PaperTrail.enabled = true
    PaperTrail.request.enabled = true
    begin
      yield
    ensure
      PaperTrail.enabled = was_enabled
      PaperTrail.request.enabled = was_enabled_for_request
    end
  end
end

And when we use in test with the following pattern:

# Model
class MyModel < ApplicationRecord
  include Discard::Model
  self.discard_column = :deleted_at
  
  has_paper_trail ignore: [:updated_at, :data_fetched_at],
                  versions: { class_name: "MyModelVersion" },
                  on: [:update]
end  

# Test

require 'test_helper'

class MyModelTest < ActiveSupport::TestCase
  include ActiveJob::TestHelper
  include Versioning
  test 'should have versioning' do
    with_versioning do
      my_model = create(:my_model)

      assert_equal 0, MyModelVersion.count

      my_model.update!(price: 1000)

      assert_equal 1, MyModelVersion.count

      my_model.update!(data_fetched_at: Time.zone.now)

      assert_equal 1, MyModelVersion.count
    end
  end
end

we have the following error:

Failure:
test_should_have_versioning(Minitest::Result) [/usr/local/bundle/gems/activerecord-7.2.1/lib/active_record/connection_adapters/postgresql/database_statements.rb:19]:
ActiveRecord::StatementInvalid: PG::UndefinedTable: ERROR:  relation "versions" does not exist
LINE 10:  WHERE a.attrelid = '"versions"'::regclass

For us Papertrail is not well working with RoR 7.2 and it's blocking our upgrade.
Any help is welcoming

Regards

spickermann · 2024-09-05T08:06:12Z

@Migoo The issue is probably caused by how theMyModelVersion model inherits from PaperTrail::Version or configures the table name. Please see my comment above for how to fix that.

Migoo · 2024-09-05T09:24:00Z

@spickermann Thanks for the quick answer ! I didn't see you reply above. I juste update the code and it's working 🙌

schinery · 2024-09-05T09:40:51Z

I tested it with Rails 7.2.1. Everything works fine.

Same, all good for me too. Ship it?

arikarim · 2024-09-05T13:52:03Z

this one takes too long, i bet Rails 8 comes ahead of this merge 😑

tbrammar · 2024-09-07T16:08:03Z

Pretty please @seanlinsley @gurgelrenan @aried3r @jaredbeck - can we get this PR merged and released? 🙏🫠

jaredbeck

Looks good, thanks everyone who contributed.

jaredbeck · 2024-09-08T03:58:59Z

Released as 15.2.0

kalashnikovisme force-pushed the rails_7.2.0 branch 2 times, most recently from a6c0107 to 2a5da16 Compare August 10, 2024 13:41

kalashnikovisme changed the title ~~Hotfix for support of Rails 7.2.0~~ WIP: Hotfix for support of Rails 7.2.0 Aug 10, 2024

kalashnikovisme force-pushed the rails_7.2.0 branch 5 times, most recently from 29b952b to 12fa951 Compare August 10, 2024 14:17

Hotfix for support of Rails 7.2.0

bb656a4

kalashnikovisme force-pushed the rails_7.2.0 branch from 12fa951 to bb656a4 Compare August 10, 2024 14:19

kalashnikovisme changed the title ~~WIP: Hotfix for support of Rails 7.2.0~~ Hotfix for support of Rails 7.2.0 Aug 10, 2024

ronnqvistandreas reviewed Aug 15, 2024

View reviewed changes

Change ActiveRecord compatibility versions

5343ce6

kalashnikovisme requested a review from ronnqvistandreas August 19, 2024 18:30

ronnqvistandreas reviewed Aug 19, 2024

View reviewed changes

gurgelrenan mentioned this pull request Aug 22, 2024

Add support for Rails 7.2 #1488

Closed

veganstraightedge mentioned this pull request Aug 28, 2024

Rails 7.2 compatibility #1477

Closed

6 tasks

joshRpowell reviewed Aug 29, 2024

View reviewed changes

Change ruby versions in test workflow

50b53b3

joshRpowell reviewed Aug 29, 2024

View reviewed changes

Update README

ada7f38

Adjust deprecator definition to be more inline with the official API

c1b17d5

As per the comment from @bvogel here: paper-trail-gem#1485 (comment)

tbrammar mentioned this pull request Sep 3, 2024

Adjust deprecator definition to be more inline with the official API kalashnikovisme/paper_trail#1

Merged

tbrammar and others added 2 commits September 3, 2024 14:39

Update railtie.rb

238c87c

Merge pull request #1 from premiacapital/rails_7.2.0

face459

Adjust deprecator definition to be more inline with the official API

Decreased test line-coverage by 0.01%

32221c5

samward reviewed Sep 4, 2024

View reviewed changes

modosc mentioned this pull request Sep 5, 2024

please consider taking on new maintainers #1492

Open

jaredbeck approved these changes Sep 8, 2024

View reviewed changes

jaredbeck merged commit d304bd5 into paper-trail-gem:master Sep 8, 2024
9 checks passed

kalashnikovisme deleted the rails_7.2.0 branch September 8, 2024 22:33

	ar_version = ::Gem::Version.new("8.0.0")
	ar_version = ::Gem::Version.new("7.3.0")

Hotfix for support of Rails 7.2.0 #1485

Hotfix for support of Rails 7.2.0 #1485

Conversation

kalashnikovisme commented Aug 10, 2024 • edited Loading

fatkodima commented Aug 11, 2024

olivierbuffon commented Aug 14, 2024

fatkodima commented Aug 14, 2024

arikarim commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalashnikovisme Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

kalashnikovisme Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

denysivanov commented Aug 19, 2024

ryansdwilson commented Aug 19, 2024

Halphas commented Aug 19, 2024

ronnqvistandreas left a comment

Choose a reason for hiding this comment

kalashnikovisme commented Aug 19, 2024 • edited Loading

chrisjeon commented Aug 20, 2024

benkruger commented Aug 20, 2024

denysivanov commented Aug 21, 2024

cisolarix commented Aug 22, 2024

dip00dip commented Aug 22, 2024

denysivanov commented Aug 23, 2024

veganstraightedge commented Aug 28, 2024

zhandao commented Aug 29, 2024

capripot commented Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gurgelrenan Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

joshRpowell Aug 29, 2024 • edited Loading

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

bvogel commented Sep 2, 2024

tbrammar commented Sep 3, 2024 • edited Loading

kalashnikovisme commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalashnikovisme commented Sep 4, 2024

Migoo commented Sep 5, 2024

spickermann commented Sep 5, 2024

Migoo commented Sep 5, 2024

schinery commented Sep 5, 2024

arikarim commented Sep 5, 2024

tbrammar commented Sep 7, 2024

jaredbeck left a comment

Choose a reason for hiding this comment

jaredbeck commented Sep 8, 2024

kalashnikovisme commented Aug 10, 2024 •

edited

Loading

kalashnikovisme Aug 16, 2024 •

edited

Loading

kalashnikovisme Aug 16, 2024 •

edited

Loading

kalashnikovisme commented Aug 19, 2024 •

edited

Loading

capripot commented Aug 29, 2024 •

edited

Loading

gurgelrenan Sep 1, 2024 •

edited

Loading

joshRpowell Aug 29, 2024 •

edited

Loading

tbrammar commented Sep 3, 2024 •

edited

Loading