Skip to content
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

Email to developer on plan change performed via API #709

Merged
merged 3 commits into from Mar 19, 2019

Conversation

guicassolato
Copy link
Contributor

@guicassolato guicassolato commented Mar 14, 2019

What this PR does / why we need it
This PR makes application plan changes performed by the provider through the API to send a notification message to the buyer as well, as it does when performed in the Admin Portal.

54424581-7f778200-4713-11e9-8832-960af2f96ea8

Which issue(s) this PR fixes
Closes THREESCALE-923.

@guicassolato guicassolato force-pushed the fix/THREESCALE-923-email-plan-change-api branch 2 times, most recently from 26fc943 to 00e7b28 Compare March 14, 2019 13:04
@codecov
Copy link

codecov bot commented Mar 14, 2019

Codecov Report

Merging #709 into master will decrease coverage by <.01%.
The diff coverage is 91.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #709      +/-   ##
==========================================
- Coverage   92.84%   92.84%   -0.01%     
==========================================
  Files        2387     2387              
  Lines       77516    77523       +7     
==========================================
+ Hits        71967    71973       +6     
- Misses       5549     5550       +1
Impacted Files Coverage Δ
...ts/cinstances/cinstance_plan_changed_event_test.rb 100% <ø> (ø) ⬆️
app/lib/logic/plan_changes.rb 87.5% <ø> (-0.12%) ⬇️
app/observers/message_observer.rb 100% <100%> (ø) ⬆️
...ications/new_notification_system_migration_test.rb 98.41% <100%> (+0.07%) ⬆️
...notifications/new_notification_system_migration.rb 100% <100%> (ø) ⬆️
...n/admin/api/buyers_applications_controller_test.rb 100% <100%> (ø) ⬆️
test/unit/observers/cinstance_observer_test.rb 100% <100%> (ø) ⬆️
test/unit/observers/message_observer_test.rb 100% <100%> (ø) ⬆️
app/mailers/mail_preview.rb 34.88% <33.33%> (+1.14%) ⬆️
test/workers/create_service_token_worker_test.rb 100% <0%> (ø) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b1e6c5...2d83ee7. Read the comment docs.

@guicassolato guicassolato force-pushed the fix/THREESCALE-923-email-plan-change-api branch 4 times, most recently from 4c61691 to e6229c3 Compare March 15, 2019 10:32
@guicassolato guicassolato self-assigned this Mar 15, 2019
@guicassolato guicassolato changed the title [wip] Email to developer on plan change performed via API Email to developer on plan change performed via API Mar 15, 2019
@guicassolato guicassolato requested a review from a team March 15, 2019 11:21
@Martouta Martouta self-requested a review March 15, 2019 15:18
@guicassolato guicassolato force-pushed the fix/THREESCALE-923-email-plan-change-api branch 3 times, most recently from ffddb4d to 5643347 Compare March 18, 2019 10:14
@Martouta Martouta self-requested a review March 18, 2019 10:39
Martouta
Martouta previously approved these changes Mar 18, 2019
Copy link
Contributor

@Martouta Martouta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this new version 👍 Good job! 🥇

@Martouta Martouta requested a review from macejmic March 18, 2019 11:44
@@ -1,4 +1,7 @@
class NotificationMailer < ActionMailer::Base
include CMS::EmailTemplate::MailerExtension
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this class is only for Provider

Provider users can choose which notification they get in their notification preferences
Developers do not really have this option.

@macejmic can we have a way to use subscribe_for_notification in another class?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really, we would need to create the same logic for developers.

@guicassolato guicassolato changed the title Email to developer on plan change performed via API [wip] Email to developer on plan change performed via API Mar 18, 2019
@guicassolato guicassolato force-pushed the fix/THREESCALE-923-email-plan-change-api branch from b444f6d to c2c1a74 Compare March 18, 2019 16:51
@guicassolato guicassolato requested review from hallelujah, Martouta and macejmic and removed request for macejmic March 18, 2019 17:18
Martouta
Martouta previously approved these changes Mar 18, 2019
Copy link
Contributor

@Martouta Martouta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 And I appreciate the tests to understand a bit of what's going on 😄 but I definitely don't know enough about this notifications/emails logic so it would be good if @hallelujah and @macejmic can review too 😄

macejmic
macejmic previously approved these changes Mar 19, 2019

def notify_plan_change_developer(contract)
return unless contract.is_a? Cinstance
contract.messenger.plan_change_for_buyer(contract).deliver
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to use asynchronous mailing.
But I think it is OK for now as it was like that in the controller anyway.

@guicassolato guicassolato dismissed stale reviews from macejmic and Martouta via 2d83ee7 March 19, 2019 11:25
@guicassolato guicassolato force-pushed the fix/THREESCALE-923-email-plan-change-api branch from c2c1a74 to 2d83ee7 Compare March 19, 2019 11:25
@guicassolato guicassolato changed the title [wip] Email to developer on plan change performed via API Email to developer on plan change performed via API Mar 19, 2019
Copy link
Contributor

@Martouta Martouta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@guicassolato guicassolato merged commit cc5bced into master Mar 19, 2019
@guicassolato guicassolato deleted the fix/THREESCALE-923-email-plan-change-api branch March 19, 2019 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants