Belongs_to ..., touch: true nie działa


#1

Rails 5.1.4
Ruby 2.4.1

Cześć,

class CreateEvents < ActiveRecord::Migration[5.1]
  def change
    create_table :events do |t|
      t.string :title, index: true
      ...

      t.timestamps
    end
  end
end


class CreateAccessorizations < ActiveRecord::Migration[5.1]
  def change
    create_table :accessorizations do |t|
      t.integer :event_id, index: true
      ...

      t.timestamps      
    end
  end
end

class Event < ApplicationRecord
  has_many :accessorizations, dependent: :destroy, index_errors: true
  accepts_nested_attributes_for :accessorizations, reject_if: :all_blank, allow_destroy: true
  ...

end

class Accessorization < ApplicationRecord
  belongs_to :event, touch: true
  ...

end


class EventsController < ApplicationController
  ...

  def update
    @event = Event.find(params[:id])
    respond_to do |format|
      if @event.update(event_params)
        flash[:success] = t('activerecord.successfull.messages.updated', data: @event.fullname) unless @event.previous_changes.empty?
        format.html { redirect_to @event }
        format.json { render :show, status: :ok, location: @event }
      else
        format.html { render :edit }
        format.json { render json: @event.errors, status: :unprocessable_entity }
      end
    end
  end

  private
    def event_params
      params.require(:event).permit(:title, accessorizations_attributes: [:id, :event_id, ...., :_destroy])
    end
end

Wszystko się pięknie dopisuje, usuwa itd zarówno w tabeli Event jak i Accessorization ale dlaczego @event.previous_changes.empty? działa tylko gdy zmieniam atrybuty modelu Event, a nie działa, gdy usuwam/dopisuję/zmieniam wpisy w tabeli podrzędnej accessorizations?


#2

moduł ActiveModel::Dirty widzi tylko AttributeMethods nie śledzi tego co delegowane


#3

Oki…
To jak działa:
belongs_to parent_resource touch: true
?

Nie jest to instrukcja, która “dotyka” parent_resource i wprowadza zmianę w “updated_at” w chwili, gdy dokonujemy zmian na obiektach “nested”?

…albo inaczej :slight_smile:
To jak najlepiej “wychwycić zmiany” w obiektach “nested” dla akcji :update?


#4

Jak się nie mylę to touch pomija callbacki i jest ograniczony do stemplowania update (tu już nie sprawdzam)

nie wiem czy “najlepiej” to najlepsze określenie dla mojej propozycji ale do pilnowania “bękartów” i “wdów” stosuję triggery


#5

Najlepiej to nie stosować nested attributes i pójść w FormObject


#6

Hahaha…
Pisałem przez wieeeeele lat q SQL i też wiem, że to najlepszy pomysł, aczkolwiek w railsowym kontrolerze może się nie sprawdzić :slight_smile:

Wymyśliłem takie coś:

def update
  ...
  flash[:success] = t('activerecord.successfull.messages.updated', data: @event.fullname) unless @event.previous_changes.empty? && (@event.accessorizations.map {|a| a.previous_changes}).empty?

Co sądzisz o tym pomyśle?


#7

…poczytam o tym


#8

lol … chcesz tylko zablokować komunikat o zmianach gdy nie było zmian

+1 dla @wafcio


#9

Dwie rzeczy:

  1. Zablokować komunikat,

  2. Zablokować akcję modelu, która obecnie wygląda (prawie )tak:

    after_update_commit { self.log_work('update') }

    def log_work(type)
    return if previous_changes.empty?

     self.works.create!(trackable_url: "#{url_helpers.event_path(self)}", action: "#{type}", user: self.user, 
       parameters: self.to_json( 
         include: { accessorizations}
       )
     )
    

    end

…czyli zapisuje do logu operację


#10

Masz może jakiś link do dobrego, wyczerpującego artykułu?


#11

podstawowy to ten: http://blog.codeclimate.com/blog/2012/10/17/7-ways-to-decompose-fat-activerecord-models/

Dawno temu na wroclove.rb Danny Olson miał prezentacje o FormObjectach - https://www.youtube.com/watch?v=NENkGg-wzx8

Jak dla każdego wzorca nie railsway ciężko znaleźć jedno źródło wiedzy które opisze wszystko wyczerpująco.


#12

Może się komuś przyda - rozwiązałem to następująco:

  def update
    @event = Event.find(params[:id])
    accessorizations_marked_any = is_any_accessorizations_marked_destroy?(params[:event][:accessorizations_attributes])
    authorize @event, :update?
    respond_to do |format|
      if @event.update(event_params)
        if @event.saved_changes? || @event.accessorizations.map {|a| a.saved_changes?}.flatten.include?(true) || accessorizations_marked_any
          flash[:success] = t('activerecord.successfull.messages.updated', data: @event.fullname)
          @event.log_work('update')
        end
        format.html { redirect_to @event }
        format.json { render :show, status: :ok, location: @event }
      else
        format.html { render :edit }
        format.json { render json: @event.errors, status: :unprocessable_entity }
      end
    end
  end

  private
   def event_params
    params.require(:event).permit(:title, :all_day, :start_date, :end_date, :note, :project_id, :event_status_id, :event_type_id, accessorizations_attributes: [:id, :event_id, :user_id, :role_id, :_destroy])
  end

  def is_any_accessorizations_marked_destroy?(accessorizations_params)
    is_marked = false
    accessorizations_params.keys.each do |k|
      is_marked = true if accessorizations_params["#{k}"][:"_destroy"] == "1" || accessorizations_params["#{k}"][:"_destroy"] == "true"
    end
    is_marked
  end

end

gdzie:
@event.saved_changes?

  • sprawdza, czy główny obiekt został zmieniony

@event.accessorizations.map {|a| a.saved_changes?}.flatten.include?(true)

  • sprawdza, czy jakikolwiek obiekt nested został zmieniony

funkcja is_any_accessorizations_marked_destroy?(accessorizations_params)

  • sprawdza, czy jakikolwiek rekord z tabeli nested został oznaczony do usunięcia

#13

Słabiutko to wygląda, ja bym nie przepuścił w CR :wink:


#14

Jakieś konstruktywne propozycje omijające FormObject? :slight_smile: :wink:

…Pamiętam, że ostatnio wbiłeś wszystkich w podłogę swoim rozwiązaniem Strong Parameters :slight_smile:

/ poprawiłem troszeczkę funkcję dodając if accessorizations_params.present?

def is_any_accessorizations_marked_destroy?(accessorizations_params)
  is_marked = false
  accessorizations_params.keys.each do |k|
    is_marked = true if accessorizations_params["#{k}"][:"_destroy"] == "1" || accessorizations_params["#{k}"][:"_destroy"] == "true"
  end if accessorizations_params.present?
  is_marked
end

#15

Czy taki kod już byś przepuścił? :slight_smile: :wink:

  def update
    @event = Event.find(params[:id])
    respond_to do |format|
      if @event.update(event_params)
        if event_or_any_accessorization_changed? 
          flash[:success] = t('activerecord.successfull.messages.updated', data: @event.fullname)
          @event.log_work_without_check_changed('update')
        end
        format.html { redirect_to @event }
        format.json { render :show, status: :ok, location: @event }
      else
        format.html { render :edit }
        format.json { render json: @event.errors, status: :unprocessable_entity }
      end
    end
  end

  private
    def accessorization_changed_any?
      @event.accessorizations.map {|a| a.saved_changes?}.flatten.include?(true)
    end

    def accessorization_marked_destroy_any?
      event_params.to_h[:accessorizations_attributes].map {|a| a[1][:_destroy] }.include?("1")
    end

    def event_or_any_accessorization_changed?
      @event.saved_changes? || accessorization_changed_any? || accessorization_marked_destroy_any?
    end

    def event_params
      params.require(:event).permit(:title, ...., accessorizations_attributes: [:id, :event_id, :user_id, :role_id, :_destroy])
    end

end

#16
> @event.accessorizations.map {|a| a.saved_changes?}.flatten.include?(true)

@event.accessorizations.any?(&:saved_changes?)

Architektura tego rozwiązania pozostawia wiele do życzenia. Metody które dodajesz do kontrolera nie mają nic wspólnego z pozostałymi akcjami, dlatego najlepiej byłoby przenieść całą logikę do osobnego obiektu, do którego po prostu przekażesz całe params. Polecam ogólnie używanie narzędzi typu rubocop czy reek (najlepiej jako linter w edytorze).

Być może lepszym rozwiązaniem będzie faktycznie użycie touch: true i porównanie np. samego updated_at w oryginalnym modelu (przed i po zapisie)?

Abstrahując od rozwiązania, czy na pewno potrzebujesz takiego warunku? W typowych CURDach raczej nikt się nie przejmuje tym czy faktyczny update cokolwiek zmienił i po prostu pokazywany jest flash. Rozróżnienie tych przypadków nie poprawia specjalnie UX. W tym konkretnym przypadku warto by na pewno dodać odpowiedni komunikat, również wtedy, kiedy nie będzie zmian. Inaczej działanie będzie niespójne.


#17

Zgadzam się z Tobą, że najlepiej byłoby to wynieść poza kontroler, jednakże gdy zobaczymy, że wszystko służy temu, by wyświetlić odpowiedni komunikat i zapisać dane do historii, to nie wiem, czy gra jest warta świeczki.

użycie touch: true odpada z prostego powodu - nie działa!
Mam zdefiniowane accepts_nested_attributes_for i obiekt @event nie wykrywa zmian w @event.accessorizations.

Pytasz się, czy potrzebuję takiego rozwiązania.
Hmm…
A lubisz w “historii wpisów” oglądać 10 razy te same dane, bo Pani “Jadzia” zawsze lubi wchodzić w edycję i zawsze naciska przycisk “Aktualizuj”? :smile: :wink:

“W tym konkretnym przypadku warto by na pewno dodać odpowiedni komunikat, również wtedy, kiedy nie będzie zmian. Inaczej działanie będzie niespójne.”

  • Wiesz przecież, że Rails 5.1.4 nie dokonuje operacji na bazie danych, gdy nie wprowadziłeś żadnych zmian w formularzu.
  • Zwyczajnie nie wykonuje “update” na rekordzie, gdy nie zmieniłeś danych.
    W jakim celu zatem mam wyświetlać Pani “Jadzi” jakikolwiek komunikat w przypadku, gdy otworzyła formularz do edycji, nie dokonała żadnej zmiany i nacisnęła przycisk “Aktualizuj”?

P.S. Jeżeli inny użytkownik w tym czasie zmienił dane tego rekordu, to Pani Jadzi dostanie komunikat, że zapisała dane - a w zasadzie -> nadpisała.


#18

Założenie nowego pliku na dysku to przecież nie jest inwestycja, którą trzeba starannie rozważać. Za chwilę zrobisz podobnie ciężką logikę w akcji create, do tego pojawią się pozostałe akcje. Kontroler będzie miał 15 prywatnych metod i ponad 1000 linii. Jak wrócisz do tego pliku po 2 tygodniowej przerwie, to sam nie będziesz już pamiętał co było do czego. Zrobienie drobnej poprawki będzie Ciebie kosztowało 2 godziny ponownego wdrożenia a i tak skończy się tym, że zepsuje się jakaś funkcja, o której przeznaczeniu zupełnie zapomniałeś.

Obiekt, którego wejściem będzie params a wyjściem true/false dyktujące kontrolerowi czy zrobić render czy redirect zamknie całą logikę, a do tego będzie go można bardzo ładnie i prosto testować.

Wyjaśnienie co do tego pojawiło się już wcześniej. Metoda touch, która jest wykorzystywana do tego celu pomija większość callbacków i nie wykryjesz zmian korzystając z ActiveModel::Dirty (changes/previous_changes). Zamiast tego porównaj sobie po prostu datę aktualizacji obiektu (lub #cache_key) przed i po wykonaniu update.

To jest kwestia tego, jak i w jakim celu zaimplementowane jest tworzenie tej historii. Być może wystarczy Tobie https://github.com/airblade/paper_trail w miejsce implementacji własnego mechanizmu? Czy tu chodzi tylko o odnotowani faktu aktualizacji danych, bez faktycznego zapisania co zostało zmienione?

Pomijając kwestię zapisywania historii osobiście nie bardzo rozumiem problem. :slight_smile: Czy gdyby faktycznie update następował zawsze, niezależnie czy dane zostały zapisane czy nie - wówczas taki komunikat byłby w porządku? Myślę, że niepotrzebne przeceniasz percepcję pewnych rzeczy przez użytkowników i trochę na siłę starasz się pokazywać komunikaty odzwierciedlające w 100% rzeczywistość. O ile powiedzmy rozumiem kwestię logowania w celu audytu, o tyle problemu z komunikatem już nie. Pewne konwencje narzucone przez środowisko/framework sprawiają, że niektóre rzeczy są banalnie proste i szybko się je pisze, inne z kolei wymagają dużego nakładu.

W tym konkretnym przypadku moim zdaniem nie ma sensu się specjalnie męczyć i bardziej opłaca się po prostu zrobić rozwiązanie szablonowe. Co więcej, czasem więcej sensu może mieć stosowanie najprostszych rozwiązań i logowanie 10x update przy każdym kliku. Może się okazać że w praktyce nie będzie to problemem. Wszystko zależy od tego jaka faktycznie jest jest logika biznesowa, który stoi za tym wymaganiem. Efektywnie po jednej stronie masz 5 minut pracy nad kontrolerem i zamknięcie tematu, a po drugiej długie godziny i w efekcie rozwiązanie, które i tak będzie średnie. :slight_smile: Wyobraź sobie, że dodasz do tego modelu kolejne relacje (co jest prawdopodobne) - za każdym razem będziesz musiał dorabiać spory kawał logiki do tego kontrolera).

O ile faktycznie w praktyce długofalowo lepiej sprawdzi się FormObject zamiast accepts_nested_attributes_for o tyle w tym przypadku nie jest to żadne rozwiązanie Twojego problemu. I tak finalnie skończy się na tym samym - czyli będziesz musiał iterować sobie po metodach changed? i dłubać w przekazanych parametrach. Po prostu zniknie to z kontrolera i pojawi się gdzie indziej.

Podsumowując to Twoja aplikacja, więc decyzje musisz podjąć sam. Ja tylko napisałem swoje uwagi i nie chce Cie mocno namawiać. :slight_smile: Patrząc jedynie na ten kawałek bez znajomości kontekstu - touch: true i porównanie daty wydaje mi się sensowne. Uwoli Cię od konieczności modyfikacji tego kodu kiedy zmienią się / dojdą nowe relacje.


#19

Dzięki za wszystkie Wasze uwagi.

P.S.
Przemyślałem co napisałeś i stwierdziłem, że może faktycznie niepotrzebnie się spinam z tym badaniem zmian na obiekcie i obiektach skojarzonych przez nested, więc …wywaliłem to wszystko :slight_smile:
Został standardowy komunikat i zapisywanie logów w akcji after_commit


#20

Skoro i tak robisz to globalnie, to zdecydowanie ponownie polecam: https://github.com/airblade/paper_trail