P2P matching & Form #730
P2P matching & Form #730
Conversation
Adds claim ask button & ignore vendor/bundle folder
eabousaif
assigned indiebrain, evertoncunha, thestephenmarshall, acherukuri and eabousaif
safe navigates person name & email
Create a match for contribution & logs the communication
This reverts commit 6538209.
app/views/peer_to_peer_match_mailer/peer_to_peer_email.html.erb
Outdated
Show resolved
Hide resolved
app/models/person.rb
Outdated
| def anonymize_email | ||
| return if email.blank? | ||
|
|
||
| at_index = email.index("@") | ||
| words = email.split(".") | ||
| top_level_domain = words.pop | ||
|
|
||
| masked_email = "*" * words.join(".").length | ||
| masked_email[at_index] = "@" | ||
| masked_email.concat(".", top_level_domain) | ||
| end | ||
|
|
||
| def anonymize_name | ||
| return if name.blank? | ||
|
|
||
| names = name.split(" ") | ||
| names.map do |n| | ||
| initial = n.first | ||
| n = '*' * n.length | ||
| initial + n[1..-1] | ||
| end.join(" ") | ||
| end | ||
|
|
There was a problem hiding this comment.
This logic isn't part of the Person object's core responsibility so, adhering to the SPR, i'd love to get them extracted into a utility module.
Maybe something like app/lib/anonymize.rb, so that they can be used like this:
def anonymized_name_and_email
"#{Anonymize.name(name)} #{Anonymize.email(email)"
endNote: in a departure from rails norms, i personally don't like using mixins for such cases. Happy to say more about that if anyone's interested :).
This would also make these methods very easy to test.
There was a problem hiding this comment.
I would definitely like to know more about not using mixins for these scenarios :). Also, I have moved this logic to Anonymize class.
| @@ -41,6 +41,23 @@ def self.follow_up_status(follow_up_status) | |||
| result | |||
| end | |||
|
|
|||
| def self.create_match_for_contribution!(contribution, current_user) | |||
There was a problem hiding this comment.
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.
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 for interactors.
There was a problem hiding this comment.
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 the run! form which assumes the interactor isn't trying to return any meaningful data back to the controller; there's the regular run form for when a result object is useful).
One challenge you might run into is that claims_controller makes use of deliver_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.
| def self.create_offer_for_ask!(ask, current_user) | ||
| Offer.create!(person: current_user.person, service_area: ask.service_area) | ||
| end | ||
|
|
||
| def self.create_ask_for_offer!(offer, current_user) | ||
| Ask.create!(person: current_user.person, service_area: offer.service_area) | ||
| end |
There was a problem hiding this comment.
@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.
ooo. yes, we should add Category to the autogenerated Ask/Offer
acherukuri
force-pushed
the
add-peer-to-peer-matching
branch
from
31f4966
to
0ca77df
Compare
There was a problem hiding this comment.
Last of multi-part review (helpers).
I hope all my suggestions weren't discouraging 😬 ! Please feel free to take the ones you agree with and leave the rest -- the code is already good and already provides much needed functionality!
🔥 🙏🏾.
(P.S, thanks for already turning around some of the earlier suggestions!)
app/controllers/claims_controller.rb
Outdated
| if current_user.person.blank? | ||
| Person.create_from_peer_to_peer_params!(current_user, name: peer_to_peer_match_params[:peer_alias], | ||
| preferred_contact_method_id: peer_to_peer_match_params[:preferred_contact_method_id], | ||
| contact_info: peer_to_peer_match_params[:preferred_contact_details]) | ||
| end |
There was a problem hiding this comment.
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.
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.
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.
@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.
Looks great 🚀 . Agree on making the interactor refactoring a separate branch 👍🏾.
acherukuri
force-pushed
the
add-peer-to-peer-matching
branch
from
e0db503
to
26b0e59
Compare
Person.preferred_contact_info already contains some of the logic to get the field value associated with the preffered_contact_method. Also feels more straightforward, easier to follow, if we explicitly set locals in the controller action instead of relying on a helper to figure them out. Also renames preferred_contact_details -> preferred_contact_info just to stay consistent with the existing method name.
@exbinary Nope, not at all discouraging. I'm happy I got to work on this project. Happy to help in any way I could. Moreover, I've picked up some new concepts and made a few friends :). |
As have i ❤️ :) |
|
@exbinary @maebeale Merge PR button is enabled for me but I'm not sure if I can merge the PR by myself or you wanna do it? I thought I would bring up this question just in case. |
|
Oh, one last thing -- don't we need to share the claimer's email with the claimee? I think we need to add that to the email body. |
|
@acherukuri agree w @exbinary's point about including the email address in the email that's sent and adding Category to autogenerated Asks/Offers, but those can be made in to new issues. :greenlightemoji: for merging it! :) |
| @@ -0,0 +1,24 @@ | |||
| class Anonymize | |||
| @@ -9,10 +9,10 @@ class CommunicationLog < ApplicationRecord | |||
|
|
|||
| scope :needs_follow_up, ->(boolean=nil){ where(needs_follow_up: boolean || true) } | |||
|
|
|||
| def self.log_submission_email(email_object, delivery_status, submission, delivery_method=nil, current_user=nil) | |||
| def self.create_offer_for_ask!(ask, current_user) | ||
| Offer.create!(person: current_user.person, service_area: ask.service_area) | ||
| end | ||
|
|
||
| def self.create_ask_for_offer!(offer, current_user) | ||
| Ask.create!(person: current_user.person, service_area: offer.service_area) | ||
| end |
There was a problem hiding this comment.
ooo. yes, we should add Category to the autogenerated Ask/Offer
|
|
||
| <%= simple_form_for :claim, url: contribution_claims_path(contribution) do |f| %> | ||
| <%= f.input :peer_alias, as: :string, label: false, placeholder: "Alias or handle (do not use your real name here)", required: true %> | ||
| <%= f.input :preferred_contact_method_id, collection: Array(ContactMethod.email), label: "Preferred contact method", required: true %> |
Enables peer to peer communication hidden between a feature toggle. The flow allows someone to claim either an offer or an ask and get in touch with the other party.