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 32 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.
I think we might also need to handle when the current_user changes their contact preference and info.
IIUC, such a change would currently be ignored.
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.
Yes, you're right. That change is completely ignored as of now. We're only considering email. My take would be to create a notification interface and depending on the contact preference method, we can either notify via an email or SMS.
Do you want me to make any specific changes here or to the form to restrict only to email?
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.
We can discuss more at our 9am if you like, but i'm leaning toward only allowing email on the form for this iteration.
In the controller, if we have to fabricate a person instance, we can force email to be the preferred contact method.
In addition, it would be good to save the email address if the person already existed but in this case, maybe leave their preference alone, just updating their email field?
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.
@exbinary I have made changes to restrict the preferred contact method to just email for now and also saving email to the person record if absent in the system. Let me know how that looks.
I'm gonna branch off of this branch and start working on implementing
active_interaction
. I thought that shouldn't stop us from merging these changes.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.
Looks great 🚀 . Agree on making the interactor refactoring a separate branch 👍🏾.
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