elixir-cldr / cldr_trans

Cldr-based fork of the most excellent "trans" library
Other
10 stars 3 forks source link

How to handle optional fields? #7

Closed ArthurClemens closed 8 months ago

ArthurClemens commented 8 months ago

Perhaps this is more of an Ecto question, but I haven't found the answer.

I have an Article with fields title and body, of which only title is required. The Translations struct lists both field keys - if the translation for body is empty, the changeset is marked invalid.

What I would like to achieve is that this struct is accepted:

translations: %MyApp.Article.Translations{
  en: %MyApp.Article.Translations.Fields{
    title: "Hello world!",
    body: ""
  }
}
kipcole9 commented 8 months ago

You're right, the definition of the embedded schema that is automatically generated also generates a changeset the has validate_required/2 for all translatable fields.

I think I can modify the api to allow a list of translatable fields and some way to flag if required or not required. I need to keep the API consistent so it's not a breaking change.

How about something like:

use MyApp.Cldr.Trans, translates: [:title, :body], required: [:title]

If the :required option isn't specified, all translations are required (current behaviour).

An additional idea might be to allow a validation callback to be configured so it might be:

use MyApp.Cldr.Trans, translates: [:title, :body], required: [:title], validate_change: &MyModule.validate/1

Thoughts?

BTW, if this is urgent you can handcraft the embedded schema and not use the translations/1 macro - then you can do whatever you want. The readme shows you what the structure would look like.

kipcole9 commented 8 months ago

I don't know what the ramifications are in the rest of the library if a translation is missing - that will need some extensive testing to ensure no unexpected behaviour too.

kipcole9 commented 8 months ago

Another option is to not change the current behaviour at all. This would mean that if you want different behaviour you would write your own changeset for the translations embedded schema and then cast_embedded(:translations, MyTranslation) in the main schema changeset.

ArthurClemens commented 8 months ago

Thanks for your swift reply.

Some thoughts:

kipcole9 commented 8 months ago

I prefer the direction of writing a custom changeset

Then apart from updating the documentation to note that the generated changeset includes :required on all translatable fields, there is nothing to be done?

I agree with you, I think suggesting that library users write their own changesets is the better approach.

ArthurClemens commented 8 months ago

It's not obvious to me how to override the default changeset.

kipcole9 commented 8 months ago

And I'm exposing my lack of knowledge in how Ecto changesets work with embedded schemas. 😀

I though embedded schema changesets are invoked with the 'cast_embed/2' call and is there explicit. Does it happen implicitly as well?

ArthurClemens commented 8 months ago

After a bit of playing around, it appears to be quite easy to override the default changeset. So no need to implement custom embeds.

Using a custom translation_field_changeset function I can define the validation behaviour:

defp changeset(article, attrs) do
  article
  |> cast(attrs, [
    :title,
    :body,
  ])
  |> cast_embed(:translations, with: &translations_changeset/2)
  |> validate_required([:title])
end

defp translations_changeset(translations, params) do
  translations
  |> cast(params, [])
  |> cast_embed(:en, with: &translation_field_changeset/2)
end

defp translation_field_changeset(schema, params) do
  schema
  |> cast(params, [:title, :body])
  |> validate_required([:title])
end
kipcole9 commented 8 months ago

You make a good point, the documentation needs improving and I'll work on that. I think for your example, it can be slightly simpler to do the following:

defmodule MyApp.Article do
  use Ecto.Schema
  use MyApp.Cldr.Trans, translates: [:title, :body]

  schema "articles" do
    field :title, :string
    field :body, :string
    translations :translations
  end
end

Then use your changeset functions in your repo as you are already doing:

defmodule MyApp.Repo do
  defp changeset(article, attrs) do
    article
    |> cast(attrs, [
      :title,
      :body,
    ])
    |> cast_embed(:translations, with: &translations_changeset/2)
    |> validate_required([:title])
  end

  defp translations_changeset(translations, params) do
    translations
    |> cast(params, [])
    |> cast_embed(:en, with: &translation_field_changeset/2)
  end

  defp translation_field_changeset(schema, params) do
    schema
    |> cast(params, [:title, :body])
    |> validate_required([:title])
  end
end

Does that work as expected?

ArthurClemens commented 8 months ago

Yes, I already updated my comment above.

kipcole9 commented 8 months ago

Thanks for the feedback. I'll leave the issue open until I update the docs in the next 48 hours.

kipcole9 commented 8 months ago

I believe the updated docs (now published to hexdocs improve the discussion around this area. I will close for now - but I would warmly welcome further feedback and PRs to make the docs even better.