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) { 'pick_up@org.com' } |
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
isidzukuri
force-pushed
the
4077-add-pickup-person-to-the-distribution-notification
branch
3 times, most recently
from
400050b
to
813b1d9
Compare
|
Looks good to me, but the linter is complaining :) |
|
fixed 🏁 |
isidzukuri
force-pushed
the
4077-add-pickup-person-to-the-distribution-notification
branch
from
2670c1b
to
d62478e
Compare
|
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