-
-
Notifications
You must be signed in to change notification settings - Fork 61
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
Conversation
Adds claim ask button & ignore vendor/bundle folder
safe navigates person name & email
Create a match for contribution & logs the communication
This reverts commit 6538209.
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.
Third of multi-part review (mailer)
app/views/peer_to_peer_match_mailer/peer_to_peer_email.html.erb
Outdated
Show resolved
Hide resolved
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.
Fourth of multi-part review (models).
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)"
end
Note: 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 for interactors
.
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 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.
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
31f4966
to
0ca77df
Compare
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.
Fifth of multi-part review (controllers, views).
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.
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.
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 👍🏾.
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! :) |
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.
SOOOO excited about this feature. THANK YOU, folks!!!
@@ -0,0 +1,24 @@ | |||
class Anonymize |
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!
@@ -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) |
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
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.
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
|
||
<%= 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 %> |
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.
👍
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.