Open tjchambers opened 5 days ago
I am going to leave this question open, but IMHO the key should represent the Ecto schema's field name.
Currently afaict the current paper_trail implementation refers to the database column name, which is not traditionally how I would refer to the changes if I was to try and re-apply them. Regardless of the intent of what the key means there is the issue of changes in column names or field names in Ecto referring to the same column re: backwards compatibility of data (not sure that is solvable). Making this refer to the internal Ecto field name insulates column name changes for the future similar to the use of the schema module name insulating the database table name from being in the Versions content.
Since making the item_changes refer to field names as the key is a breaking change I am going to fork the repo and make that change for my app.
I am still of the opinion that the definition of the intent of the key in the item changes JSON content to be a database column name should be documented in the README for clarity.
In reading the README I read the statement which I strongly support:
Data integrity is an important aim of this project, ...
Paper_trail is an important addition to my app and ensuring that the item_changes capture all of the related DB changes is key to reconstructing events.
I appreciate all the work that goes into projects like this (I was a user of the Paper_trail ruby gem) and want to thank again all the contributors.
IMO this is all the more reason to alert users of this OSS that if the use Ecto field source options that those changes to those fields are not captured due to the difference between field and column names.
Thanks to all for contributing!
here are the changes I made for supporting Ecto field sourcing for posterity:
@doc """
Dumps changes using Ecto fields
"""
@spec serialize_changes(Ecto.Changeset.t()) :: map()
def serialize_changes(%Ecto.Changeset{changes: changes, data: %schema{}} = changeset) do
changed_fields = changes
|> Map.keys()
|> Enum.map(fn key -> {key, schema.__schema__(:field_source, key)} end)
columns = Enum.map(changed_fields, fn {_, column} -> column end)
changeset
|> serialize_model_changes()
|> serialize()
|> Map.take(columns)
|> Map.new(fn {key, value} ->
case Enum.find(changed_fields, fn {_field, column} -> column == key end) do
{field, _column} -> {field, value}
end
end)
end
At first that might seem like a silly question, but it is an important one.
It is a list of "changes". But represented as a key -> value map, what is the meaning of the key portion?
Is it an Ecto Field name? Is it a database column name?
It would be helpful to know the answer to that.