Duke-GCB / D4S2

Web service to facilitate notification and transfer of projects in DukeDS
MIT License
0 stars 0 forks source link

Add email template set to Share, DDSDelivery and S3Delivery #184

Closed johnbradley closed 6 years ago

johnbradley commented 6 years ago

Adds email_template_set field to Share and DeliveryBase. Changes shares, deliveries, and s3-deliveries POST endpoints to fill in email_template_set in the associated model based on the current user. This email template set is then used for all emails sent from the Share/Delivery. Moves looking up email templates into a method that requires a request so the implications are more explicit(This email template is based on the user of the request).

Implementation Note:

Changes *DeliveryDetails replacing logic that looks up email templates with a email_template_set property.

Fixes #181

johnbradley commented 6 years ago

Right now the new email_template_set field allows null. Ideally this would be filled in by a migration. There is no direct link from a DDSDelivery to the User that created it and thereby the associated email template. However it looks like our history plugin records history_user and history_type records. https://github.com/Duke-GCB/D4S2/blob/f4a26705d349c0887258e7ceb04a53e0852f0986/d4s2_api/models.py#L139

It may be possible to find the history_type of created for each share and delivery then find the email template set based on UserEmailTemplateSet.

johnbradley commented 6 years ago

Foregoing the migration above. For now the email_template_set field will be nullable.

johnbradley commented 6 years ago

Need to add tests for when email_template_set is null.

johnbradley commented 6 years ago

show when a sender creates a delivery via the API, their template set is used for all emails related to the delivery, regardless of other template sets in the database or if the recipient user has a template set or not. Really more of an end-to-end test for a scenario

Where would this test live? It would have to test parts of d4s2_api_v1 and ownership.

dleehr commented 6 years ago

Where would this test live? It would have to test parts of d4s2_api_v1 and ownership.

That's a good question. This would be an integration test (or acceptance if we could loop in the external pieces that people use) so as you point out, it rises above a single app. I don't know that Django has an obvious convention about those (or where they would go)

We could create a subdirectory in the repo called integration_tests and put the test file there. This can be a simple directory - doesn't need to be a django "app" (so no models/views/migrations, not registered in settings.py).

We do a similar thing in bespin-api. There's a bespin-flavored OAuth backend in bespin-api/bespin/backend.py. That directory isn't a django "app", but the backend is used by settings_base.py. The backend_test.py is in the same directory and gets picked up by the test runner.

johnbradley commented 6 years ago

@dleehr This is ready for another look. The current version uses instance methods for the Mixin. The only part that is still bugging me is how this works with DeliveryPreviewView:

class DeliveryPreviewView(ModelWithEmailTemplateSetMixin, generics.CreateAPIView):
    permission_classes = (permissions.IsAuthenticated,)
    serializer_class = DDSDeliveryPreviewSerializer

    def create(self, request, *args, **kwargs):
        email_template_set = self.get_email_template_for_request()
...

We mixin another create method but we don't actually use it. We just do so to let the actual create method use the get_email_template_for_request method.

dleehr commented 6 years ago

We mixin another create method but we don't actually use it. We just do so to let the actual create method use the get_email_template_for_request method.

I think that's fine. There might be one more layer of refactoring to get that pebble out of the shoe, but I don't want to drag this on any further.