4077 Add the partner's pickup person to the distribution notification #4104
Conversation
app/mailers/distribution_mailer.rb
Outdated
| if distribution.pick_up? | ||
| pickup_person_email = @partner.profile&.pick_up_email | ||
| cc.push(pickup_person_email) if pickup_person_email.present? && pickup_person_email != @partner.email | ||
| end |
There was a problem hiding this comment.
I'd probably do it more like this:
cc = [@partner.email]
cc.push(@partner.profile&.pick_up_email) if distribution.pick_up?
cc.compact!.uniq!There was a problem hiding this comment.
i like it! Cleaner and more readable. It requires one edit:
cc = [@partner.email]
cc.push(@partner.profile&.pick_up_email) if distribution.pick_up?
cc.compact!
cc.uniq!Does it look ok for you?
Im curious which variant works faster. However performance shouldn't be an issue here.
There was a problem hiding this comment.
addressed 48c65c2
| expect(mail.body.encoded).to match("picked up") | ||
| end | ||
|
|
||
| context 'when parners profile pick_up_email is present' do | ||
| let(:pick_up_email) { '[email protected]' } |
There was a problem hiding this comment.
We should be setting this for non-pickup distributions as well. That way the above tests will validate that it's only going out to the partner, not the pick-up address, when it's not set to pickup.
There was a problem hiding this comment.
addressed, please take a look 813b1d9
Im used to write specs in slightly different style, more relying on let instead of instance variables. Please let me know if it does not fit well
|
Looks good to me, but the linter is complaining :) |
|
fixed 🏁 |
|
Thanks. In the future, can you make sure not to force-push your branch? It makes it harder to review only the new commits. |
|
ok. I used |
|
@isidzukuri: Your PR |
isidzukuri
deleted the
4077-add-pickup-person-to-the-distribution-notification
branch
Resolves #4077
Description
Add the partner's pickup person to the distribution notification email (if the distribution is pickup).
Note: From the stakeholder meeting 20221207. We have a pickup person email, but they aren't notified
Type of change
How Has This Been Tested?
tested with rspec by matching parameters of outgoing email