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

Add method to destroy a MailJet contact #58012

Merged

Conversation

bethanyaconnor
Copy link
Contributor

@bethanyaconnor bethanyaconnor commented Apr 15, 2024

Adds a method to destroy a MailJet contact. In a follow-up, I'll call this as part of the account purger.

Note that, for some reason, the API to delete a contact is only available through cURL.

@bethanyaconnor bethanyaconnor changed the title Bethany/lifecycle marketing/delete mailjet contact Add method to destroy a MailJet contact Apr 15, 2024
@bethanyaconnor bethanyaconnor marked this pull request as ready for review April 15, 2024 15:14
@bethanyaconnor bethanyaconnor requested a review from a team April 15, 2024 15:14
Copy link
Contributor

@TurnerRiley TurnerRiley left a comment

Choose a reason for hiding this comment

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

Looks great!


Net::HTTP.any_instance.expects(:request).with(mock_http_request)

MailJet.delete_contact(email)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: could there be another use of Mailjet::Contact.expects(:find).with(email).returns(mock_contact) after this last line but with .returns(nil) instead to confirm it was deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that would confirm it's actually deleted though, because we're not actually calling the MailJet service. Or am I misunderstanding?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh gotcha, I thought maybe we could check it with that line to have the format of 1) check that it returns the mock_contact like you'd expect, 2) delete it, 3) have the same check but expect it to be nil sort of deal. Totally fine if that wouldn't work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I walk through those steps, I'm not sure what you're imagining for step 3. Can you maybe write some pseudocode?

For clarity, what I'm understanding is:

  1. to change line 76 to be
Mailjet::Contact.expects(:find).with(email).returns(mock_contact).then.returns(nil)
  1. already done in line 84!

  2. not sure

Copy link
Contributor

Choose a reason for hiding this comment

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

Resolved offline: I misunderstood what the line was doing and this test makes sense!

@bethanyaconnor bethanyaconnor merged commit 8f9ccfc into staging Apr 16, 2024
2 checks passed
@bethanyaconnor bethanyaconnor deleted the bethany/lifecycle-marketing/delete-mailjet-contact branch April 16, 2024 19:25
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.

None yet

2 participants