-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rails 5.2 support, drop Rails 4.2 support. #887
Changes from all commits
17eeacd
04e6758
bc7ec0d
e766f9f
edc6180
04c6658
6747109
33cc9bd
ef3797a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,11 @@ | ||
appraise 'activerecord-5.2' do | ||
gem 'activerecord', '~> 5.2.0' | ||
end | ||
|
||
appraise 'activerecord-5.1' do | ||
gem 'activerecord', "~> 5.1.1" | ||
end | ||
|
||
appraise 'activerecord-5.0' do | ||
gem 'activerecord', "~> 5.0.3" | ||
end | ||
|
||
appraise "activerecord-4.2" do | ||
gem "activerecord", "~> 4.2.8" | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,7 +96,6 @@ def self.taggable? | |
include Cache | ||
include Ownership | ||
include Related | ||
include Dirty | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
module ActsAsTaggableOn::Taggable | ||
module Core | ||
|
||
def self.included(base) | ||
base.extend ActsAsTaggableOn::Taggable::Core::ClassMethods | ||
|
||
|
@@ -28,12 +29,16 @@ def initialize_acts_as_taggable_on_core | |
has_many context_taggings, -> { includes(:tag).order(taggings_order).where(context: tags_type) }, | ||
as: :taggable, | ||
class_name: 'ActsAsTaggableOn::Tagging', | ||
dependent: :destroy | ||
dependent: :destroy, | ||
after_add: :dirtify_tag_list, | ||
after_remove: :dirtify_tag_list | ||
|
||
has_many context_tags, -> { order(taggings_order) }, | ||
class_name: 'ActsAsTaggableOn::Tag', | ||
through: context_taggings, | ||
source: :tag | ||
|
||
attribute "#{tags_type.singularize}_list".to_sym, ActiveModel::Type::Value.new | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line makes I saw you comment this PR will be released as the version 6.0.0 and allow breaking changes, but the impact of this change, if you don't expect, is very big. What do you think about this case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I understand the issue. Could you provide a failing spec? Though I think if it's 6.0.0 release, then breaking changes are totally ok, just repo owner has to update readme before reasling this new version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Failing is request spec with the rspec-json_matcher gem. I copy and modify some from the real codes and output of running rspec: # request spec
it { is_expected.to be_json_as [product: expected_product_response] }
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The case mentioned above is with the rspec-json_matcher gem, but if not using the rspec-json_matcher gem, the test suites testing api response would all fail because of the change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion this change is harmful #1064 |
||
end | ||
|
||
taggable_mixin.class_eval <<-RUBY, __FILE__, __LINE__ + 1 | ||
|
@@ -42,12 +47,24 @@ def #{tag_type}_list | |
end | ||
|
||
def #{tag_type}_list=(new_tags) | ||
parsed_new_list = ActsAsTaggableOn.default_parser.new(new_tags).parse | ||
|
||
if self.class.preserve_tag_order? || parsed_new_list.sort != #{tag_type}_list.sort | ||
set_attribute_was('#{tag_type}_list', #{tag_type}_list) | ||
write_attribute("#{tag_type}_list", parsed_new_list) | ||
end | ||
|
||
set_tag_list_on('#{tags_type}', new_tags) | ||
end | ||
|
||
def all_#{tags_type}_list | ||
all_tags_list_on('#{tags_type}') | ||
end | ||
|
||
private | ||
def dirtify_tag_list(tagging) | ||
attribute_will_change! tagging.context.singularize+"_list" | ||
end | ||
RUBY | ||
end | ||
end | ||
|
@@ -161,7 +178,7 @@ def all_tags_on(context) | |
|
||
if ActsAsTaggableOn::Utils.using_postgresql? | ||
group_columns = grouped_column_names_for(ActsAsTaggableOn::Tag) | ||
scope.order("max(#{tagging_table_name}.created_at)").group(group_columns) | ||
scope.order(Arel.sql("max(#{tagging_table_name}.created_at)")).group(group_columns) | ||
else | ||
scope.group("#{ActsAsTaggableOn::Tag.table_name}.#{ActsAsTaggableOn::Tag.primary_key}") | ||
end.to_a | ||
|
@@ -181,33 +198,16 @@ def set_tag_list_on(context, new_list) | |
add_custom_context(context) | ||
|
||
variable_name = "@#{context.to_s.singularize}_list" | ||
process_dirty_object(context, new_list) unless custom_contexts.include?(context.to_s) | ||
|
||
instance_variable_set(variable_name, ActsAsTaggableOn.default_parser.new(new_list).parse) | ||
parsed_new_list = ActsAsTaggableOn.default_parser.new(new_list).parse | ||
|
||
instance_variable_set(variable_name, parsed_new_list) | ||
end | ||
|
||
def tagging_contexts | ||
self.class.tag_types.map(&:to_s) + custom_contexts | ||
end | ||
|
||
def process_dirty_object(context, new_list) | ||
value = new_list.is_a?(Array) ? ActsAsTaggableOn::TagList.new(new_list) : new_list | ||
attrib = "#{context.to_s.singularize}_list" | ||
|
||
if changed_attributes.include?(attrib) | ||
# The attribute already has an unsaved change. | ||
old = changed_attributes[attrib] | ||
@changed_attributes.delete(attrib) if old.to_s == value.to_s | ||
else | ||
old = tag_list_on(context) | ||
if self.class.preserve_tag_order | ||
@changed_attributes[attrib] = old if old.to_s != value.to_s | ||
else | ||
@changed_attributes[attrib] = old.to_s if old.sort != ActsAsTaggableOn.default_parser.new(value).parse.sort | ||
end | ||
end | ||
end | ||
|
||
def reload(*args) | ||
self.class.tag_types.each do |context| | ||
instance_variable_set("@#{context.to_s.singularize}_list", nil) | ||
|
@@ -315,3 +315,4 @@ def find_or_create_tags_from_list_with_context(tag_list, _context) | |
end | ||
end | ||
end | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one was fine actually, It's good to have a blank line at the end of files :). Sorry, I should have been more specific than a 👍 |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,7 @@ def tags_match_type | |
|
||
def escaped_tag(tag) | ||
tag = tag.downcase unless ActsAsTaggableOn.strict_case_match | ||
ActsAsTaggableOn::Utils.escape_like(tag) | ||
tag.gsub(/[!%_]/) { |x| '!' + x } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one should be crazily spec'ed :). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's just a rollback of cc7a2c0, nothing I wrote for this PR. That commit breaks builds in master branch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can use ActsAsTaggableOn::Utils.escape_like(tag) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not to repeat myself, but this change is unrelated to the scope of this PR, as it barely rollbacks commit that broke the build here and in master branch |
||
end | ||
|
||
def adjust_taggings_alias(taggings_alias) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
# -*- encoding : utf-8 -*- | ||
require 'spec_helper' | ||
|
||
describe ActsAsTaggableOn::Taggable::Dirty do | ||
describe 'Dirty behavior of taggable objects' do | ||
context 'with un-contexted tags' do | ||
before(:each) do | ||
@taggable = TaggableModel.create(tag_list: 'awesome, epic') | ||
|
@@ -14,19 +14,27 @@ | |
end | ||
|
||
it 'should show changes of dirty object' do | ||
expect(@taggable.changes).to eq({'tag_list' => ['awesome, epic', ['one']]}) | ||
expect(@taggable.changes).to eq({'tag_list' => [['awesome', 'epic'], ['one']]}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a breaking change. Either rollback your change or we have to communicate crazy about that. Our own internal code depends on the previous way it was formatted, and I bet a bunch of projects have the same. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Erowlin this PR is tagged for release 6.0.0, which is supposed to have breaking changes, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then I guess we're fine! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a major breaking change that we should avoid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can not promise I will have any time to invest into it in next days/weeks. Even though new behaviour seems much more logical to me, because previously it was a change from string to array, and now it's change from array to array ( no akward change of data type). |
||
end | ||
|
||
it 'flags tag_list as changed' do | ||
expect(@taggable.tag_list_changed?).to be_truthy | ||
it 'should show changes of freshly initialized dirty object' do | ||
taggable = TaggableModel.find(@taggable.id) | ||
taggable.tag_list = 'one' | ||
expect(taggable.changes).to eq({'tag_list' => [['awesome', 'epic'], ['one']]}) | ||
end | ||
|
||
if Rails.version >= "5.1" | ||
it 'flags tag_list as changed' do | ||
expect(@taggable.will_save_change_to_tag_list?).to be_truthy | ||
end | ||
end | ||
|
||
it 'preserves original value' do | ||
expect(@taggable.tag_list_was).to eq('awesome, epic') | ||
expect(@taggable.tag_list_was).to eq(['awesome', 'epic']) | ||
end | ||
|
||
it 'shows what the change was' do | ||
expect(@taggable.tag_list_change).to eq(['awesome, epic', ['one']]) | ||
expect(@taggable.tag_list_change).to eq([['awesome', 'epic'], ['one']]) | ||
end | ||
|
||
context 'without order' do | ||
|
@@ -90,23 +98,19 @@ | |
end | ||
|
||
it 'should show changes of dirty object' do | ||
expect(@taggable.changes).to eq({'language_list' => ['awesome, epic', ['one']]}) | ||
expect(@taggable.changes).to eq({'language_list' => [['awesome', 'epic'], ['one']]}) | ||
end | ||
|
||
it 'flags language_list as changed' do | ||
expect(@taggable.language_list_changed?).to be_truthy | ||
end | ||
|
||
it 'preserves original value' do | ||
expect(@taggable.language_list_was).to eq('awesome, epic') | ||
expect(@taggable.language_list_was).to eq(['awesome', 'epic']) | ||
end | ||
|
||
it 'shows what the change was' do | ||
expect(@taggable.language_list_change).to eq(['awesome, epic', ['one']]) | ||
end | ||
|
||
it 'shows what the changes were' do | ||
expect(@taggable.language_list_changes).to eq(['awesome, epic', ['one']]) | ||
expect(@taggable.language_list_change).to eq([['awesome', 'epic'], ['one']]) | ||
end | ||
end | ||
|
||
|
@@ -123,5 +127,16 @@ | |
expect(@taggable.changes).to be_empty | ||
end | ||
end | ||
|
||
context 'when language_list changed by association' do | ||
let(:tag) { ActsAsTaggableOn::Tag.new(name: 'one') } | ||
|
||
it 'flags language_list as changed' do | ||
expect(@taggable.changes).to be_empty | ||
@taggable.languages << tag | ||
expect(@taggable.language_list_changed?).to be_truthy | ||
end | ||
end | ||
|
||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the whole method can be removed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried - some of tests fail, especially sqlite ones. Let's not risk it this time :)