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

Remove mandrill #956

Merged
merged 8 commits into from Jan 21, 2019
Merged

Remove mandrill #956

merged 8 commits into from Jan 21, 2019

Conversation

jonodrew
Copy link
Contributor

@jonodrew jonodrew commented Jan 11, 2019

The aim of this PR is to remove mandrill from supplier-frontend entirely. Maintaining multiple clients complicates the codebase and takes up brain space for developers. Ticket
In addition, the mandrill client is now deprecated

  1. The first commit removes a route that wasn't used any more. It also removes the associated email template
  2. The second commit brings in new functionality from utils
  3. The third commit replaces mandrill with notify in the framework_dashboard
  4. The fourth commit replaces mandrill with notify in the view_contract_variation route
  5. The fifth commit removes mandrill from a number of tests where it was passed in but not used. I think this clears the tests up a little and makes it clearer what is being tested.

@jonodrew jonodrew force-pushed the jk-remove-mandrill branch 3 times, most recently from e63f2bb to d1883e7 Compare January 14, 2019 16:27
Copy link
Contributor

@katstevens katstevens left a comment

Choose a reason for hiding this comment

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

A few things to look at - happy to chat through in person on Wed or on hangout tomorrow.

tests/app/main/test_frameworks.py Show resolved Hide resolved
app/main/views/frameworks.py Outdated Show resolved Hide resolved
app/main/views/frameworks.py Outdated Show resolved Hide resolved
tests/app/main/test_frameworks.py Outdated Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
app/main/views/frameworks.py Outdated Show resolved Hide resolved
app/main/views/frameworks.py Outdated Show resolved Hide resolved
app/main/views/frameworks.py Show resolved Hide resolved
tests/app/main/test_frameworks.py Outdated Show resolved Hide resolved
tests/app/main/test_frameworks.py Show resolved Hide resolved
@jonodrew jonodrew force-pushed the jk-remove-mandrill branch 3 times, most recently from aafcb4f to e2b50f0 Compare January 17, 2019 08:44
config.py Outdated Show resolved Hide resolved
Copy link
Contributor

@benvand benvand left a comment

Choose a reason for hiding this comment

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

Looks generally good, couple of comments about variable names etc, but conceptually looks great.

This commit removes the upload_framework_agreement route. It is a route used for uploading a framework agreement pre-g-cloud 8. As we're about to move to G-Cloud 11, this route is no longer necessary. In the interests of keeping the codebase clean I've removed the route and its corresponding tests. I've also removed the email template, as we're not emailing anyone from this route any more.
This commit removes the use of mandrill from the framework_dashboard route. It's been replaced with notify and a for-loop. I've also had to create a new template in the Notify UI for new DOS applications. The ids for these two templates are in a dictionary that's passed to the DMNotifyClient when it's initialised. The templates that mandrill used have been deleted. The tests have been updated to reflect the fact that notify is called.

I've also removed an unnecessary patch in test_framework::test_interest_registered_in_framework_on_post
@jonodrew jonodrew changed the title [WIP]Remove mandrill Remove mandrill Jan 17, 2019
This commit changes existing tests for the `view_contract_variation` view so that they work with the new Notify client. The method we’re testing is instantiated at the top of this class as notify_send_email, so we can use self.notify_send_email to check if it’s called rather than patching all over the place.
@jonodrew jonodrew force-pushed the jk-remove-mandrill branch 2 times, most recently from c7884fe to 666e108 Compare January 18, 2019 09:49
This commit switches mandrill for notify when emailing users a confirmation of their agreement to a variation in the framework. This change will require that a developer adds an appropriate template to Notify, or that there is a change in the future to make the template sufficiently generic that we can hard-code it.
@jonodrew jonodrew force-pushed the jk-remove-mandrill branch 2 times, most recently from b185544 to 923378f Compare January 18, 2019 10:42
This commit adds two templates, for g-cloud and digital outcomes and specialists, so the config file. This ensures they're picked up whenever the client is initialised, and means there's only one place to add/remove/replace them.
Copy link
Contributor

@katstevens katstevens left a comment

Choose a reason for hiding this comment

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

Epic work 👍 @benvand any further comments?

lot['slug']: [draft for draft in complete_drafts if draft['lotSlug'] == lot['slug']]
for lot in framework['lots']
}
lot_results = {k: lot_result(v) for k, v in complete_drafts_by_lot.items()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Readability could benefit from (IMO)

lot_results = {lot_slug: lot_result(draft_services) for lot_slug, draft_services in complete_drafts_by_lot.items()}

I think this has the advantage of cementing the structure of the above comprehension. But it's a bit of a nitpick tbh :)

Copy link
Contributor

@benvand benvand left a comment

Choose a reason for hiding this comment

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

Nice work! Let's give it a spin!

@benvand
Copy link
Contributor

benvand commented Jan 21, 2019

(👋 bye Mandrill!)

@jonodrew
Copy link
Contributor Author

(👋 bye Mandrill!)

Not quite...still that pesky Zendesk/Salesforce question to solve!

@jonodrew jonodrew merged commit 5b2f83f into master Jan 21, 2019
@jonodrew jonodrew deleted the jk-remove-mandrill branch January 21, 2019 09:00
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

3 participants