Correct error for missing delegate field

[jinjagit]

closes #118

TLDR: This 'fixes' the problem, but I don't particularly like the fix, and it perhaps raises more questions than it answers. This PR aims to stimulate discussion on the topic.

The issue:
createDelegation mutation with only 1 email field provided, misleadingly leads to an error of: Could not create delegation ... %{delegate_id: ["can't be blank"], delegator_id: ["can't be blank"]} This error does not help the user identify the missing email field.

The workaround:
Detect such a missing field (and the presence of one email field), and add the complimentary email field with a value of nil. Now a missing :delegator_email returns an error of Could not create delegate ... email: ["can't be blank"], which is more useful.

Observations:
I have placed the 'fix' in the beginning of the lib/liquid_voting_web/resolvers/delegations.ex create_delegation/3 function. It could go elsewhere (pattern matching in lib/liquid_voting/delegations.ex create_delegation/1, for example).

Questions raised:
This seems to be a product of having 2 different ways to identify participants at the absinthe layer (ids & emails). Should we be handling all possible cases? E.g. 1 id & 1 email, 2 ids, 2 emails, only 1 id, only 1 email, or nothing. We might well now be handling all these cases appropriately, but I would have to test more thoroughly to check, and I wanted to know if this issue signals a need for a more fundamental shift in approach, or just a different approach, before I start down that road.
Also, does none of this matter, since the user UI will 'decide' how to fill in fields and these edge cases will never arise (is this YAGNI)?

Thoughts?

[oliverbarnes]

Should we be handling all possible cases? E.g. 1 id & 1 email, 2 ids, 2 emails, only 1 id, only 1 email, or nothing.

Just two cases: either both ids are present, or both emails. So if a platform chooses to use the id strategy, they need to provide both ids from existing participants. If they want to use emails instead, either from existing participants or new ones, they need to provide them for both participants as well.

And I see two ways to handle this:

One might be to add a resolver-level validator for the presence of either the two ids or the two emails, and if args don't comply then Delegations.create_delegation() never gets called and a custom error message is returned.

The other might be to let Delegations.create_delegation() execute and then check the error changeset for the missing id errors. If found, then return a custom error message for the resolver.

What do you think?

[jinjagit]

One might be to add a resolver-level validator for the presence of either the two ids or the two emails, and if args don't comply then Delegations.create_delegation() never gets called and a custom error message is returned.

The other might be to let Delegations.create_delegation() execute and then check the error changeset for the missing id errors. If found, then return a custom error message for the resolver.

I like the former, as it would enable helpful errors and also is more efficient (since it does not hit the create_delegation functions if an error is raised beforehand). The latter would need work to avoid the misleading errors that come from submitting one or zero emails (since these go to the id-based create_delegation/1 and return 'id can't be blank' for both delegator and delegate - regardless of whether one email was provided or not).

[davefrey]

I like the former as well.

I'd extract that case though so the pipeline reads in a meaningful way, something like:
|> Delegations.validate_participants

[davefrey]

Hmm, now I see Delegate.validate_participants_different(changeset) which is a good sibling.
I'm too new to this code and elixir, but couldn't we have a
|> Delegate.validate_participant_identities(changeset) and be specific in the error? Multi would rollback the Voting.upsert_participant calls right?

[oliverbarnes]

It would, but that happens in the data context level, which the former solution tries to avoid :)

I like validate_participants, or
validate_participant_args

I'm not sure about which is more efficient, actually. Ecto and postgres are both fast, but so is pattern matching. Difference might be negligible, would need to benchmark. Checking the id error changeset is probably not a lot of work either, actually.

But separation of concerns and clarity of intent are good reasons to do it at the resolver level, imho

[jinjagit]

Cool, I'll write a validate_participants function at the resolver level, with suitable error messages if not receiving 2 * ids or 2 * emails. Thanks for the input!

Also switching this to draft / WIP for now.

[jinjagit]

First attempt at validation at the resolver level.
I decided to try writing comments in doc format. Not sure how useful the examples are. Thoughts?

[oliverbarnes]

Looks great 👌 Nothing to add!

@niklaslong would you like to have a look?

[niklaslong]

Great work, I left some suggestions!

[niklaslong]

Suggestion: refactor this to use with, something like this:



with {:ok, args} <- validate_participant_args(args),
     {:ok, delegation} <- Delegations.create_delegation(args) do
  {:ok, delegation}
else
  error -> error
end

(Map.put(args, :organisation_id) could be done beforehand)

[jinjagit]

@niklaslong I like this idea, but how do I include the 3 different types of error tuples that could be returned (1 type from the call to validate_participant() and 2 different types possible from call to create_delegation()) in the with structure you suggest?
There seems to be only 1 error case, in the else block, whilst we could get errors (with different forms) for either call.

[niklaslong]

You could move the three errors associated with create_delegation to the else block and bubble up the ones from validate_participant with the catch-all.

[jinjagit]

Do you mean this kind of approach?

def convert_to_csv(source_filename) do
    with {:l1, {:ok, :xlsx}} <- {:l1, detect_file_type(source_filename)},
         {:l2, {:ok, csv_filename}} <- {:l2, convert_excel_to_csv(source_filename)} do
  {:ok, csv_filename}
else
  {:l1, {:error, error}} -> {:error, error}  # (such as file not found)
  {:l1, {:ok, _other}} -> {:error, :not_an_excel_file}
  {:l2, error} -> error
end

[niklaslong]

Something else I've noticed: we have two different forms for the error messages. Might be nice to homogenise this (later PR of course, but worth discussing, I think)?

[jinjagit]

To be clear, what are you seeing as the default / desired error structure? Is it this:
{:ok, %{errors: [%{message: message, details: details}]}}
As in test/liquid_voting_web/absinthe/mutations/create_participant_test.exs, for example.

[niklaslong]

I don't know what the default or desired structure is, that's the reason for opening #123. However, the structure we've had in the resolvers up until now is {:error, message, detail}.

[jinjagit]

I'm disappearing down a bit of a rabbit hole here, trying to get the errors to play well with the error -> error part, since create_delegation() can also return errors from upsert_participant, and these errors are not all formatted the way you are envisioning. Thus, I would need to change the error shapes elsewhere (as in the example above, which seems to be changeset error), which seems like scope creep.
How about we go with the steps wrappers for now, and simplify after the errors issue is resolved?

[niklaslong]

In that case I'd revert to the nested case statements for now, we don't want the added complexity of steps in our return values. Let's save the rabbit-hole for another day once #123 is discussed.

[jinjagit]

Actually, this almost works...

with {:ok, args} <- validate_participant_args(args),
         {:ok, delegation} <- Delegations.create_delegation(args) do
      {:ok, delegation}
    else
      {:error, message} ->
        {:error, message}

      {:error, changeset} ->
        {:error,
         message: "Could not create delegation", details: ChangesetErrors.error_details(changeset)}

      {:error, name, changeset, _} ->
        {:error,
         message: "Could not create #{name}", details: ChangesetErrors.error_details(changeset)}
    end

Just getting one (other) test throwing an error now.

[jinjagit]

@niklaslong I have implemented your suggested changes (beware the 'outdated' code thang). Thanks!

[jinjagit]

@niklaslong I think I finally have a simpler version, in line with your ideas, but still deferring reworking the error formats until another issue. Thanks for helping me with this, and especially with understanding the matching issue!

[niklaslong]

LGTM, thanks for the great work @jinjagit! 🎉

[oliverbarnes]

LGTM 👍

And I'll take a look at the new issue on error message formatting soon