Prepare to release 6.5 #968
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
|
We still have one failing test case with ActiveRecord 6.0. Once this is resolved, is everything ready for a release? |
mkilling
reviewed
Sep 19, 2019
| if ActsAsTaggableOn::Utils.legacy_activerecord? | ||
| set_attribute_was("#{tag_type}_list", #{tag_type}_list) | ||
| else | ||
| attribute_change("#{tag_type}_list") |
There was a problem hiding this comment.
If my current understanding is correct, this line is supposed to set the 'previous' value of the *_list attribute. It seems that attribute_change is not the right method to do this. I’ve quickly looked into the code for AttributeMutationTracker but I’m not quite sure yet what’s the best way to replicate the behavior of set_attribute_was in AR 6.0.
Ideas welcome.
There was a problem hiding this comment.
Perhaps we could use {attribute}_will_change! as seen here in Rails 6 as well: https://api.rubyonrails.org/classes/ActiveModel/Dirty.html
If an attribute is modified in-place then make use of [attribute_name]_will_change! to mark that the attribute is changing. Otherwise Active Model can't track changes to in-place attributes. Note that Active Record can detect in-place modifications automatically. You do not need to call [attribute_name]_will_change! on Active Record models.
person.name_will_change!
person.name_change # => ["Bill", "Bill"]
person.name << 'y'
person.name_change # => ["Bill", "Billy"]
There was a problem hiding this comment.
The problem is a bit different though. It’s that in this test case original value for tag_list is nil and not ["awesome", "epic"] as expected. ActiveRecord picks up that the attribute has been changed, but it does not print the correct previous value.
taggable = TaggableModel.find(@taggable.id)
taggable.tag_list
# => ["awesome", "epic"]
taggable.tag_list_will_change!
taggable.tag_list_change
# => [nil, nil]
taggable.tag_list = 'one'
taggable.tag_list_change
# => [nil, ["one"]]
|
It seems that #955 is somewhat related to this |
|
I changed the core.rb file to: and it works !!! |
|
@edd1312 Thanks for trying that out. If I understand your code correctly, it will still not work on ActiveRecord 6.0. Also, I’m not convinced that saving the record should be a side effect of assigning to the tag_type_list. |
|
@mkilling i tried it with the branch #951 . making this changes it returns this result perhaps this is not the best solutions but it works for me |
|
This works for me: if self.class.preserve_tag_order? || (parsed_new_list.sort != #{tag_type}_list.sort)
if ActsAsTaggableOn::Utils.legacy_activerecord?
set_attribute_was("#{tag_type}_list", #{tag_type}_list)
else
unless #{tag_type}_list_changed?
@attributes["#{tag_type}_list"] = ActiveModel::Attribute.from_user("#{tag_type}_list", #{tag_type}_list, ActsAsTaggableOn::Taggable::TagListType.new)
end
end
write_attribute("#{tag_type}_list", parsed_new_list)
endCan anybody who’s more familiar with ActiveModel tell me if there are any problems with this solution? (Edit: fixed my proposed code for multiple updates of |
|
@seuros Given my proposed change above is acceptable, what are the next steps towards releasing version 6.5? I’m tracking my changes in https://github.com/mkilling/acts-as-taggable-on/tree/support-rails-6. I can raise a pull request. |
|
This LGTM, it'd be great to get Rails 6 support! |
|
for those that want to add the mkilling rails 6 support solution. the line in Gemfile should be: |
|
I will release a prerelease later this week.
Sorry for delay. I'm currently in the himalaya without stable internet to
release.
…On Mon, Oct 21, 2019, 15:43 Marvin Killing ***@***.***> wrote:
@mbleigh <https://github.com/mbleigh> @seuros <https://github.com/seuros>
It would be great if we could merge this and make it a release. How can we
move this forward? Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#968?email_source=notifications&email_token=AASIUTZYRC6DVJAHBUPJHTDQPV4KPA5CNFSM4IUUHSM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBZYMSY#issuecomment-544441931>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AASIUTZPYOONLUPJY7LYGULQPV4KPANCNFSM4IUUHSMQ>
.
|
Co-Authored-By: Easy Bills <[email protected]>
rolandoalvarado
pushed a commit
to rolandoalvarado/acts-as-taggable-on
that referenced
this pull request
Mar 5, 2020
close mbleigh#951 fixes mbleigh#953 files mbleigh#961
This was referenced Mar 12, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support for ActiveRecord 6.