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

[AC-1693] Send InvoiceUpcoming Notification to Client Owners #3319

Conversation

amorask-bitwarden
Copy link
Contributor

@amorask-bitwarden amorask-bitwarden commented Oct 3, 2023

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

This PR updates our handling of the invoice.upcoming Stripe webhook event so that, if the incoming invoice is for an Organization, we send an email notification not only to the organization's billing address, but also to the organization's client owners.

❌ This PR is based off an open PR here: #3305. Once this PR is merged, I'll rebase this PR off master.

For review purposes, everything including and after the Add Organization_ReadOwnerEmailAddresses SPROC commit should be reviewed as part of this PR.

Code changes

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@amorask-bitwarden amorask-bitwarden force-pushed the billing/AC-1693/send-invoiceupcoming-reseller-client-owner branch from b674504 to c972048 Compare October 3, 2023 15:40
@amorask-bitwarden amorask-bitwarden changed the title [AC-1693] Send InvoiceUpcoming Notification to Client Owners [AC-1693] [DNM] Send InvoiceUpcoming Notification to Client Owners Oct 3, 2023
@bitwarden-bot
Copy link

bitwarden-bot commented Oct 3, 2023

Logo
Checkmarx One – Scan Summary & Details02aad89b-6bed-40be-9370-2fdf19a16fc3

New Issues

Severity Issue Source File / Package Checkmarx Insight
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 114 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 114 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 114 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 114 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 114 Attack Vector
LOW Log_Forging /src/Billing/Controllers/StripeController.cs: 117 Attack Vector

@amorask-bitwarden amorask-bitwarden marked this pull request as ready for review October 3, 2023 16:36
@amorask-bitwarden amorask-bitwarden requested a review from a team as a code owner October 3, 2023 16:36
@djsmith85 djsmith85 requested a review from a team October 4, 2023 11:27
Copy link
Contributor

@cturnbull-bitwarden cturnbull-bitwarden left a comment

Choose a reason for hiding this comment

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

You can update this PR to target billing/PM-212/billing-email-syncing instead of master so as to not pollute the diff with the changes in #3319. Once that PR gets merged and the branch deleted, then this PR will automatically get updated to target master again

@cturnbull-bitwarden cturnbull-bitwarden requested a review from a team October 4, 2023 14:52
@amorask-bitwarden amorask-bitwarden changed the base branch from master to billing/PM-212/billing-email-syncing October 4, 2023 15:33
@amorask-bitwarden
Copy link
Contributor Author

You can update this PR to target billing/PM-212/billing-email-syncing instead of master so as to not pollute the diff with the changes in #3319. Once that PR gets merged and the branch deleted, then this PR will automatically get updated to target master again

@cturnbull-bitwarden Good call! Done.

@amorask-bitwarden amorask-bitwarden force-pushed the billing/PM-212/billing-email-syncing branch from 715bcff to cc5b4dd Compare October 5, 2023 13:11
@amorask-bitwarden amorask-bitwarden force-pushed the billing/AC-1693/send-invoiceupcoming-reseller-client-owner branch from c972048 to a8891af Compare October 5, 2023 13:12
cyprain-okeke
cyprain-okeke previously approved these changes Oct 6, 2023
Copy link
Contributor

@cyprain-okeke cyprain-okeke left a comment

Choose a reason for hiding this comment

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

LGTM

@amorask-bitwarden amorask-bitwarden force-pushed the billing/PM-212/billing-email-syncing branch from cc5b4dd to be4658c Compare October 6, 2023 13:13
@amorask-bitwarden amorask-bitwarden force-pushed the billing/AC-1693/send-invoiceupcoming-reseller-client-owner branch from f8354d1 to 7bebd6e Compare October 6, 2023 13:13
@amorask-bitwarden amorask-bitwarden force-pushed the billing/PM-212/billing-email-syncing branch from be4658c to 27c099d Compare October 11, 2023 19:46
@amorask-bitwarden amorask-bitwarden requested a review from a team as a code owner October 11, 2023 19:46
Base automatically changed from billing/PM-212/billing-email-syncing to master October 11, 2023 19:57
@amorask-bitwarden amorask-bitwarden dismissed cyprain-okeke’s stale review October 11, 2023 19:57

The base branch was changed.

@bitwarden-devops-bot bitwarden-devops-bot temporarily deployed to QA Cloud October 13, 2023 18:21 Inactive
@amorask-bitwarden amorask-bitwarden changed the title [AC-1693] [DNM] Send InvoiceUpcoming Notification to Client Owners [AC-1693] Send InvoiceUpcoming Notification to Client Owners Oct 16, 2023
Hardened against missing entity IDs in Stripe events in the StripeEventService. Updated ValidateCloudRegion to not use a refresh/expansion for the customer because the invoice.upcoming event does not have an invoice.Id. Updated the StripeController's handling of invoice.upcoming to not use a refresh/expansion for the subscription because the invoice does not have an ID.
@amorask-bitwarden amorask-bitwarden force-pushed the billing/AC-1693/send-invoiceupcoming-reseller-client-owner branch from 42b09d1 to c375a64 Compare October 23, 2023 15:55
@amorask-bitwarden amorask-bitwarden merged commit c442bae into master Oct 23, 2023
10 of 11 checks passed
@amorask-bitwarden amorask-bitwarden deleted the billing/AC-1693/send-invoiceupcoming-reseller-client-owner branch October 23, 2023 17:46
cturnbull-bitwarden pushed a commit that referenced this pull request Nov 1, 2023
* Add Organization_ReadOwnerEmailAddresses SPROC

* Add IOrganizationRepository.GetOwnerEmailAddressesById

* Add SendInvoiceUpcoming overload for multiple emails

* Update InvoiceUpcoming handler to send multiple emails

* Cy's feedback

* Updates from testing

Hardened against missing entity IDs in Stripe events in the StripeEventService. Updated ValidateCloudRegion to not use a refresh/expansion for the customer because the invoice.upcoming event does not have an invoice.Id. Updated the StripeController's handling of invoice.upcoming to not use a refresh/expansion for the subscription because the invoice does not have an ID.

* Fix broken test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants