township-agency / ex_teal

MIT License
1 stars 0 forks source link

Question: Is it necessary to customize the `handle_update` for a resource? #85

Closed loganhenson closed 3 years ago

loganhenson commented 3 years ago

I recently came across a handle_update for a resource in a project that looked like:

defmodule ExampleWeb.ExTeal.ProductResource do
  ...
  def with, do: [:product_type, :allergens, category_tags]

  def fields do
    [
      ...
      AllergenSelect.build(),
      ProductCategorySelect.build(),
      ...
    ]
  end

  def handle_update(_conn, resource, attributes) do
    Product.changeset(
      resource,
      Map.merge(%{"allergens" => [], "category_tags" => []}, attributes)
    )
  end
end

I am unsure why the Map.merge is necessary (As they should be included in the attributes).

staylorwr commented 3 years ago

👋🏼 Thanks for the great question: https://github.com/township-agency/ex_teal/blob/master/assets/src/components/Form/MultiSelect.vue#L42-L48

Now I'll be honest and say that I can't fully remember why we made this distinction, but a multi-select form field only fills values when it has values attached to it. So the default functionality is:

Honestly that reads as a bug report to me. It's probably something we can clean up by simply removing that if (this.value) if statement in the form version of the multiselect. I'll try to grep through history to see if I can remember if there was a reason for it.

So, in this case we customized the handle_update func so that the attributes passed to the changeset always contain a 'allergens' and 'category_tags' empty list, but again this seems like a teal bug.

staylorwr commented 3 years ago

Hmmm here's the git blame:

https://github.com/township-agency/ex_teal/commit/b79596f0a5649d682b67e9ed3253ff6c6a97f39c

I'm remembering a bit more context, because these are form encoded POSTs, it's hard to send back an empty array as a value, which becomes an interpretation problem between phoenix and form-encoded data. We could try sending

this.field.fill = formData => {
  if (this.value) {
    this.value.forEach((option) => {
      formData.append(`${this.field.attribute}[]`, option.value);
    });
  } else {
     formData.append(`${this.field.attributes}[]}, null);
  }
}

but I'm not convinced that will pass sanitized params 🙈

staylorwr commented 3 years ago

For those that read this far down this issue thread, we talked off github about the thing that Logan identified and came to the following conclusion:

Using the handle_update override is the appropriate solution for now, given the unknowns around how the combination of vue, FormData in the browser, and the Phoenix API responses handle the decoding of the form values.

There are a couple of solutions we could consider in the future:

loganhenson commented 3 years ago

Note: The specific thing that this handle_update is used for, is to ensure that upon removal of all items in a relationship on the frontend, that upon update, it will remove them in the backend. (Without the override, it will not sync the relationship, and will be a no-op)