Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
P2P matching & Form #730
P2P matching & Form #730
Changes from all commits
6538209
7c4d225
eb3bea3
3c967a7
d9d4e46
77edc60
1882769
ab37473
f692602
d6be53c
3cf7a3c
1d74214
bb1c2e8
16e9f33
fc97485
0e60800
91b07ce
9f79a66
69e6b6f
006a0ea
4dee09a
8ffb5fb
2337e58
ce12d1d
22de765
4eff8d5
5e3747c
ac5c9a2
a6abc95
0e5fc04
a0bb4e8
0ca77df
26b0e59
82ae80e
bab988e
f7774ce
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a great change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add the following comment above this method:
# FIXME: extract into an interactor
.I think this kind of use-case specific logic fits nicely into the interactor pattern which, among other benefits, helps to keep models from getting bloated. Unfortunately we don't have an example in the codebase i can point at currently (though we have pulling in the
active_interaction
gem), so probably makes sense to refactor this later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is really good to know. Do you mind if I try to implement
active_interaction
? I wasn't sure if you wanted to set up any conventions forinteractors
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, please go ahead if you have the time and are motivated! 💯
I don't have any strong conventions in mind, the gem is pretty good.
In a separate WIP branch, i've placed them in
app/interactions
. The example there uses therun!
form which assumes the interactor isn't trying to return any meaningful data back to the controller; there's the regularrun
form for when a result object is useful).One challenge you might run into is that
claims_controller
makes use ofdeliver_now_with_error_handling
which won't be available from within an interactor. My intention on that other branch is to extract that method into a utility that can be used from anywhere instead of having to be included as a concern.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maebeale , is this sufficient info on the auto-created Ask|Offer? Should we add the category as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooo. yes, we should add Category to the autogenerated Ask/Offer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍