Skip to content
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

Marks object as dirty even if nothing was changed in RichTextTranslation fields #597

Closed
csalmeida opened this issue Dec 12, 2022 · 5 comments

Comments

@csalmeida
Copy link

csalmeida commented Dec 12, 2022

Context

Hi all, I am not sure here but I think I might have found an issue with Mobility's dirty option and using action_text fields.

I have a model which includes two text areas with Trix. Content and Description. When submitting the edit form without changing any values the @song.saved_changes after @song.update shows that Mobility::Backends::ActionText::RichTextTranslation were saved even if the contents did not change.

Is this expected behaviour or it shouldn't happen? I assume those fields shouldn't save either since they haven't changed?

The mobility plugin configuration:

Mobility.configure do
  # PLUGINS
  plugins do
    # Backend
    #
    # Sets the default backend to use in models. This can be overridden in models
    # by passing +backend: ...+ to +translates+.
    #
    # To default to a different backend globally, replace +:key_value+ by another
    # backend name.
    #
    backend :key_value

    # Dirty
    #
    # Uncomment this line to include and enable globally:
    dirty
  end

The songs controller action:

  def update
    @song = Song.find_by(id: params[:id])

    # Allows all tags are removed on submission.
    # Otherwise it will the last one.
    if params.dig(:song, :tag_ids).nil?
      params[:song][:tag_ids] = []
    end

    if @song.update(song_params)
      puts @song.saved_changes.inspect # Mobility::Backends::ActionText::RichTextTranslation objects present.
      redirect_to songs_path
    else
      render :edit
    end
  end

The output in the terminal is the following when submitting the form without changing values, it only includes rich text fields for some reason:

{"updated_at"=>
  [Fri, 09 Dec 2022 04:11:06.660466000 UTC +00:00, Fri, 09 Dec 2022 04:12:23.104814000 UTC +00:00],
 "description_pt"=>
  [#<Mobility::Backends::ActionText::RichTextTranslation:0x000000011331df30
    id: nil,
    name: "description",
    body: nil,
    record_type: "Song",
    record_id: 4,
    created_at: nil,
    updated_at: nil,
    locale: "pt">,
   #<Mobility::Backends::ActionText::RichTextTranslation:0x000000011331df30
    id: nil,
    name: "description",
    body: nil,
    record_type: "Song",
    record_id: 4,
    created_at: nil,
    updated_at: nil,
    locale: "pt">],
 "content_pt"=>
  [#<Mobility::Backends::ActionText::RichTextTranslation:0x000000011331e250
    id: 18,
    name: "content",
    body: #<ActionText::Content "<!-- BEGIN app/views/l...">,
    record_type: "Song",
    record_id: 4,
    created_at: Fri, 09 Dec 2022 00:20:25.021014000 UTC +00:00,
    updated_at: Fri, 09 Dec 2022 00:20:25.021014000 UTC +00:00,
    locale: "pt">,
   #<Mobility::Backends::ActionText::RichTextTranslation:0x000000011331e250
    id: 18,
    name: "content",
    body: #<ActionText::Content "<!-- BEGIN app/views/l...">,
    record_type: "Song",
    record_id: 4,
    created_at: Fri, 09 Dec 2022 00:20:25.021014000 UTC +00:00,
    updated_at: Fri, 09 Dec 2022 00:20:25.021014000 UTC +00:00,
    locale: "pt">],
 "description_en"=>
  [#<Mobility::Backends::ActionText::RichTextTranslation:0x00000001133960c0
    id: nil,
    name: "description",
    body: nil,
    record_type: "Song",
    record_id: 4,
    created_at: nil,
    updated_at: nil,
    locale: "en">,
   #<Mobility::Backends::ActionText::RichTextTranslation:0x00000001133960c0
    id: nil,
    name: "description",
    body: nil,
    record_type: "Song",
    record_id: 4,
    updated_at: nil,
    locale: "en">],
 "content_en"=>
  [#<Mobility::Backends::ActionText::RichTextTranslation:0x000000011331e4f8
    id: 19,
    name: "content",
    body: #<ActionText::Content "<!-- BEGIN app/views/l...">,
    record_type: "Song",
    record_id: 4,
    created_at: Fri, 09 Dec 2022 00:20:25.022686000 UTC +00:00,
    updated_at: Fri, 09 Dec 2022 00:20:25.022686000 UTC +00:00,
    locale: "en">,
   #<Mobility::Backends::ActionText::RichTextTranslation:0x000000011331e4f8
    id: 19,
    name: "content",
    body: #<ActionText::Content "<!-- BEGIN app/views/l...">,
    record_type: "Song",
    record_id: 4,
    created_at: Fri, 09 Dec 2022 00:20:25.022686000 UTC +00:00,
    updated_at: Fri, 09 Dec 2022 00:20:25.022686000 UTC +00:00,
    locale: "en">]}

If the form is submitted and the title property is changed, it does include it (this works as it should):

{"updated_at"=>
  [Fri, 09 Dec 2022 04:25:58.481859000 UTC +00:00, Fri, 09 Dec 2022 04:26:29.673314000 UTC +00:00],
 "description_pt"=>
  [#<Mobility::Backends::ActionText::RichTextTranslation:0x0000000116a9dd48
    id: nil,
    name: "description",
    body: nil,
    record_type: "Song",
    record_id: 4,
    created_at: nil,
    updated_at: nil,
    locale: "pt">,
   #<Mobility::Backends::ActionText::RichTextTranslation:0x0000000116a9dd48
    id: nil,
    name: "description",
    body: nil,
    record_type: "Song",
    record_id: 4,
    created_at: nil,
    updated_at: nil,
    locale: "pt">],
 "content_pt"=>
  [#<Mobility::Backends::ActionText::RichTextTranslation:0x0000000116a9e040
    id: 18,
    name: "content",
    body: #<ActionText::Content "<!-- BEGIN app/views/l...">,
    record_type: "Song",
    record_id: 4,
    created_at: Fri, 09 Dec 2022 00:20:25.021014000 UTC +00:00,
    updated_at: Fri, 09 Dec 2022 00:20:25.021014000 UTC +00:00,
    locale: "pt">,
   #<Mobility::Backends::ActionText::RichTextTranslation:0x0000000116a9e040
    id: 18,
    name: "content",
    body: #<ActionText::Content "<!-- BEGIN app/views/l...">,
    record_type: "Song",
    record_id: 4,
    created_at: Fri, 09 Dec 2022 00:20:25.021014000 UTC +00:00,
    updated_at: Fri, 09 Dec 2022 00:20:25.021014000 UTC +00:00,
    locale: "pt">],
 "title_en"=>["Butter got spilt", "Butter got spilt edited"],
 "description_en"=>
  [#<Mobility::Backends::ActionText::RichTextTranslation:0x0000000116ade4b0
    id: nil,
    name: "description",
    body: nil,
    record_type: "Song",
    record_id: 4,
    created_at: nil,
    updated_at: nil,
    locale: "en">,
   #<Mobility::Backends::ActionText::RichTextTranslation:0x0000000116ade4b0
    id: nil,
    name: "description",
    body: nil,
    record_type: "Song",
    record_id: 4,
    created_at: nil,
    updated_at: nil,
    locale: "en">],
 "content_en"=>
  [#<Mobility::Backends::ActionText::RichTextTranslation:0x0000000116a9e2e8
    id: 19,
    name: "content",
    body: #<ActionText::Content "<!-- BEGIN app/views/l...">,
    record_type: "Song",
    record_id: 4,
    created_at: Fri, 09 Dec 2022 00:20:25.022686000 UTC +00:00,
    updated_at: Fri, 09 Dec 2022 00:20:25.022686000 UTC +00:00,
    locale: "en">,
   #<Mobility::Backends::ActionText::RichTextTranslation:0x0000000116a9e2e8
    id: 19,
    name: "content",
    body: #<ActionText::Content "<!-- BEGIN app/views/l...">,
    record_type: "Song",
    record_id: 4,
    created_at: Fri, 09 Dec 2022 00:20:25.022686000 UTC +00:00,
    updated_at: Fri, 09 Dec 2022 00:20:25.022686000 UTC +00:00,
    locale: "en">]}

Expected behaviour

Enabling the dirty plugin and running @song.saved_changes should be empty if none of the values were changed.

For example, when no changes are made to the form saved_changes would be {} and when a change is made it would be:

{
 "title_en"=>["Butter got spilt", "Butter got spilt edited"]
}

Actual behaviour

RichText fields are added saved_changes even if they're not modified in the form.

Possible Fix

It might be that I'm missing something but that's the behaviour I'm getting that happens when a field is set to use the action text backend at the moment.

I have a repository here where this could be replicated, sorry I could not get it deployed: https://github.com/csalmeida/rails-mobility

Creating a song and then editing it to check for the changed attribute should replicate the results I'm having.

I'm thinking that maybe because it's markup coming from Trix it might get modified in the client for some reason perhaps?

I have the following model using just Mobility's field definitions and submit a form without changing any fields I get:

Model definition:

class Song < ApplicationRecord
	attr_accessor :language
	
	extend Mobility
	
	translates :title, type: :string, locale_accessors: [:en, :pt]
	translates :description, type: :text, locale_accessors: [:en, :pt]
	translates :content, type: :text, locale_accessors: [:en, :pt]
end

Inspecting after @song.update it works as expected no values changed and therefore previous_changes is empty:

(ruby) @song.previous_changes
{}

When submitting the form again but this time with a value changed it reflects that change in previous_changes, so far so good!

(ruby) @song.previous_changes
{"updated_at"=>[Sat, 10 Dec 2022 16:09:21.395697000 UTC +00:00, Sat, 10 Dec 2022 16:16:55.869677000 UTC +00:00], "title_en"=>["The wood of Maçaranduba", "The wood of Maçaranduba 2"]}

Now I'm introducing a Mobility Action Text field, and submitting the form without changing any values:

class Song < ApplicationRecord
	attr_accessor :language
	
	extend Mobility
	
	translates :title, type: :string, locale_accessors: [:en, :pt]
	translates :description, type: :text, locale_accessors: [:en, :pt]
	translates :content, backend: :action_text, locale_accessors: [:en, :pt]
end

The input now becomes:

(Duration: 0.7ms | Allocations: 1072)
{"updated_at"=>[Sat, 10 Dec 2022 16:19:40.699785000 UTC +00:00, Sat, 10 Dec 2022 16:20:33.864672000 UTC +00:00],
 "content_en"=>
  [#<Mobility::Backends::ActionText::RichTextTranslation:0x0000000113f44bd8
    id: 2,
    name: "content",
    body: #<ActionText::Content "<div class=\"trix-conte...">,
    record_type: "Song",
    record_id: 1,
    created_at: Sun, 03 Apr 2022 22:25:54.450739000 UTC +00:00,
    updated_at: Sun, 03 Apr 2022 23:27:54.407177000 UTC +00:00,
    locale: "en">,
   #<Mobility::Backends::ActionText::RichTextTranslation:0x0000000113f44bd8
    id: 2,
    name: "content",
    body: #<ActionText::Content "<div class=\"trix-conte...">,
    record_type: "Song",
    record_id: 1,
    created_at: Sun, 03 Apr 2022 22:25:54.450739000 UTC +00:00,
    updated_at: Sun, 03 Apr 2022 23:27:54.407177000 UTC +00:00,
    locale: "en">],
 "content_pt"=>
  [#<Mobility::Backends::ActionText::RichTextTranslation:0x0000000113f44a20
    id: 3,
    name: "content",
    body: #<ActionText::Content "<div class=\"trix-conte...">,
    record_type: "Song",
    record_id: 1,
    created_at: Sun, 03 Apr 2022 23:20:59.179068000 UTC +00:00,
    updated_at: Sun, 03 Apr 2022 23:20:59.179068000 UTC +00:00,
    locale: "pt">,
   #<Mobility::Backends::ActionText::RichTextTranslation:0x0000000113f44a20
    id: 3,
    name: "content",
    body: #<ActionText::Content "<div class=\"trix-conte...">,
    record_type: "Song",
    record_id: 1,
    created_at: Sun, 03 Apr 2022 23:20:59.179068000 UTC +00:00,
    updated_at: Sun, 03 Apr 2022 23:20:59.179068000 UTC +00:00,
    locale: "pt">]}

If the title is changed that change is added to the changes array as well:

{"updated_at"=>[Sat, 10 Dec 2022 16:16:55.871974000 UTC +00:00, Sat, 10 Dec 2022 16:19:40.697910000 UTC +00:00],
 "title_en"=>["The wood of Maçaranduba 2", "The wood of Maçaranduba"],
 "content_en"=>
  [#<Mobility::Backends::ActionText::RichTextTranslation:0x0000000113d97128
    id: 2,
    name: "content",
    body: #<ActionText::Content "<div class=\"trix-conte...">,
    record_type: "Song",
    record_id: 1,
    created_at: Sun, 03 Apr 2022 22:25:54.450739000 UTC +00:00,
    updated_at: Sun, 03 Apr 2022 23:27:54.407177000 UTC +00:00,
    locale: "en">,
   #<Mobility::Backends::ActionText::RichTextTranslation:0x0000000113d97128
    id: 2,
    name: "content",
    body: #<ActionText::Content "<div class=\"trix-conte...">,
    record_type: "Song",
    record_id: 1,
    created_at: Sun, 03 Apr 2022 22:25:54.450739000 UTC +00:00,
    updated_at: Sun, 03 Apr 2022 23:27:54.407177000 UTC +00:00,
    locale: "en">],
 "content_pt"=>
  [#<Mobility::Backends::ActionText::RichTextTranslation:0x0000000113d96f20
    id: 3,
    name: "content",
    body: #<ActionText::Content "<div class=\"trix-conte...">,
    record_type: "Song",
    record_id: 1,
    created_at: Sun, 03 Apr 2022 23:20:59.179068000 UTC +00:00,
    updated_at: Sun, 03 Apr 2022 23:20:59.179068000 UTC +00:00,
    locale: "pt">,
   #<Mobility::Backends::ActionText::RichTextTranslation:0x0000000113d96f20
    id: 3,
    name: "content",
    body: #<ActionText::Content "<div class=\"trix-conte...">,
    record_type: "Song",
    record_id: 1,
    created_at: Sun, 03 Apr 2022 23:20:59.179068000 UTC +00:00,
    updated_at: Sun, 03 Apr 2022 23:20:59.179068000 UTC +00:00,
    locale: "pt">]}

Could I be missing something here? It could well be that I'm assuming that previous changes should be empty in this particular case but maybe that's the intended behaviour.

Thank you very much, I'm curious to read what you think. 🙏

@csalmeida
Copy link
Author

Found another important aspect, after the update the object has a different ID than the one it had before saving the record:

Before @song.update:

#<Mobility::Backends::ActionText::RichTextTranslation:0x00000001192de460
 id: nil,
 name: "description",
 body: nil,
 record_type: "Song",
 record_id: 4,
 created_at: nil,
 updated_at: nil,
 locale: "en">

After @song.update:

#<Mobility::Backends::ActionText::RichTextTranslation:0x000000011a3e6168
 id: nil,
 name: "description",
 body: nil,
 record_type: "Song",
 record_id: 4,
 created_at: nil,
 updated_at: nil,
 locale: "en">

Could it be that this is what ends up adding the value to the @song.previous_changes hash?

@csalmeida
Copy link
Author

I have also opened an issue at mobility-actiontext: sedubois/mobility-actiontext#37

I noticed that comparing the previous Mobility::Backends::ActionText body value with the new one can be used to determine if something is changed.

I'm trying to build some logic to filter out attributes that use Mobility::Backends::ActionText out of previous_changes if no changes were made to them but it might be a hacky way. I'll share it here if I get it working! 🙏

@csalmeida
Copy link
Author

For now, I've solved it like this, sharing in case it helps anyone in the future:

  def record_changed?(previous_record, current_record, params)
    record_changes = current_record.previous_changes
    mobility_action_text = "Mobility::Backends::ActionText::RichTextTranslation"

    # Loop through each array of changes:
    record_changes&.each do |key, value|
      
      # Previous value is used to compare the class name but it can't be compared with changed_value because they're both storing updated values.
      previous_value = value[0]
      changed_value = value[1]

      # Check if the items for each changes array are part of Mobility Action Text
      if previous_value&.class&.name == mobility_action_text
        # Compares a duplicate of the original object because previous_value and changed value cannot be trusted.
        # Removes key entirely if the value is still the same:
        if previous_record&.public_send(key)&.to_plain_text == current_record&.public_send(key)&.to_plain_text 
          record_changes.delete(key)
        end
      end
    end

    # If both content and description were not changed, then remove update from the hash. 
    if record_changes.size == 1 && record_changes["updated_at"]&.present?
      record_changes.delete("updated_at")
    end

    # Whether or not the record has changed.
    return record_changes.empty? ? false : true
  end

Please feel free to close if this is out of scope of the library, thanks a lot! 🙏

@shioyama
Copy link
Owner

Thanks for the details! Glad you found a fix. For reference, Mobility doesn't support compatibility with other gems, that's something you need to figure out yourself (which it seems like you have).

@csalmeida
Copy link
Author

Thanks @shioyama! It's reassuring to read that extending is a good way of doing things. That's correct, I have adapted it for now and it seems to work fine. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants