Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create delegation with ids uses all validations #143

Merged

Conversation

jinjagit
Copy link
Member

@jinjagit jinjagit commented Oct 5, 2020

closes #137

@jinjagit jinjagit self-assigned this Oct 5, 2020
Copy link
Member Author

@jinjagit jinjagit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some inline explanatory comments.

Copy link
Member

@oliverbarnes oliverbarnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice PR, thanks for the inline PR comments explaining some of the changes.

What do you think of updating the pre-existing absinthe test to have a CreateDelegation.WithEmails namespace to the , to be consistent with CreateDelegation.WithIds?

Only change suggestion I have. Otherwise, it's good to merge 💯

test/liquid_voting/delegations_test.exs Show resolved Hide resolved
The delegation can be created by ID or by email.
The delegation can be created using participant IDs or emails.

If created using participant emails, new participant(s) will be created if
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jinjagit
Copy link
Member Author

jinjagit commented Oct 5, 2020

Very nice PR, thanks for the inline PR comments explaining some of the changes.

What do you think of updating the pre-existing absinthe test to have a CreateDelegation.WithEmails namespace to the , to be consistent with CreateDelegation.WithIds?

Only change suggestion I have. Otherwise, it's good to merge 100

I agree, and done 👍

@jinjagit jinjagit requested a review from oliverbarnes October 5, 2020 19:09
@oliverbarnes oliverbarnes merged commit e738dc6 into master Oct 5, 2020
@oliverbarnes oliverbarnes deleted the create-delegation-with-ids-uses-all-validations branch October 5, 2020 19:20
@oliverbarnes
Copy link
Member

Edited the commit to mention the upsert and conflict resolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change multi-clause create_delegation/1 to use all validations when attrs include participant_ids
2 participants