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

Features/group-interactors #83

Merged
merged 5 commits into from
Jan 30, 2016
Merged

Features/group-interactors #83

merged 5 commits into from
Jan 30, 2016

Conversation

npauzenga
Copy link
Owner

Hey @enriikke. I presume this will be the last PR before power (and hope) are lost. Once mankind re-builds we can resume.

Good luck out there, hope to see ya on the other side.

fixes #75

@@ -1,2 +1,13 @@
class CreateGroup < StandardInteraction
def validate_input
context.fail!(errors: "invalid input") unless context.group_params
Copy link
Owner Author

Choose a reason for hiding this comment

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

@enriikke so now that I've done a few Interactors in this way (using validate_input, execute, validate_output) I'm sort of wondering if this might not be ideal. I was initially thinking that I'd want validate_input to do some more complex validation but in reality I end up just checking, in all of my Interactors, that the parameters are correct.

Do you think we'd be better served by a simple initialize method in each Interactor that would then throw an ArgumentError if something wasn't provided? It seems like if I did that I wouldn't need to define a new error if the params aren't provided BUT it would throw that error right away, not giving my controller a chance to serialize/render the JSON response.

Copy link
Owner Author

Choose a reason for hiding this comment

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

So I think the new error handling in #95 covers this a bit better. But I suppose I could override initialize with more explicit arguments i.e.

initialize(user_params, id)
  ...
end

to maybe throw an argument error if it's not called with the correct arguments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I really love how you are working on improving this. I think specifying the required input as parameters would work great. You might need to figure out how to generalize that though, so that it can be applied to all interactions. Or maybe is just something that every interaction defines on its own.


def execute
context.group = context.user.groups.create(context.group_params)
end
Copy link
Owner Author

Choose a reason for hiding this comment

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

@enriikke I think this is the cleanest way to create a Group and a UserGroupMembership at the same time. context.user is my current_user, which I think is alright and I can't think of an instance where that will be a problem right now but perhaps you have thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this works great. In theory, if we needed to reuse this interaction for an admin (so that they could create groups for any given user), they can call this interaction and pass in any user needed.

@enriikke
Copy link
Collaborator

Awesome work @npauzenga!!! 🍶 🚢 💺

npauzenga added a commit that referenced this pull request Jan 30, 2016
@npauzenga npauzenga merged commit 7db54a6 into staging Jan 30, 2016
@npauzenga npauzenga deleted the features/group-interactors branch January 30, 2016 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants