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?
…Pamiętam, że ostatnio wbiłeś wszystkich w podłogę swoim rozwiązaniem Strong Parameters
/ 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
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.
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”?
“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.
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. 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. 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ć. 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.
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
Został standardowy komunikat i zapisywanie logów w akcji after_commit
