Update template response to include a list of template_email_file names #4664
Conversation
rparke
force-pushed
the
rp-return-list-of-filenames-in-serialized-template-response
branch
from
7104e89 to
81e6985
Compare
| if files == []: | ||
| return [] | ||
|
|
||
| return [file.filename for file in files] |
There was a problem hiding this comment.
Will need more than just the filename if admin and API are going to share this data
There was a problem hiding this comment.
a dict we can serialise the same way we do templates?
There was a problem hiding this comment.
Yeah, needs to be a dict with:
idfilenamelink_textretention_periodvalidate_users_email
Best way to do this is probs to define a serialize method on the TemplateEmailFiles model then here you can do:
| return [file.filename for file in files] | |
| return [file.serialize() for file in files] |
There was a problem hiding this comment.
I agree with @quis that a dict for each file would be more useful than just a filename. If we do that, PR title and commit message will have to be updated, too.
I wonder if the SerialisedEmailFileCollection object that we have added in a previous PR could come handy here? Or are serialised models only for internal API usage?
There was a problem hiding this comment.
@CrystalPea we can start piping the output of this into SerialisedEmailFileCollection as a followup, see #4670 for a proof of concept
This adds an additional key/value pair to templates GET endpoints that will: - always return an empty list for sms and letter templates - return an empty list for an email template with no files attached - return a list of one or more files for an email template with files attached Returning a list of filenames means we can reuse all of our caching logic for templates in both api (e.g. in serialised models) and admin, storing this within the cache keys for templates. The list of files is added as an attribute at the point of serialisation, it is not stored as an attribute/relation in the SQLAlchemy model. We can't store this attribute in the templates table, and it can't be found by using a JOIN: Templates and TemplateEmailFiles are loosely coupled whereby one or many TemplateEmailFiles row can be associated with 1 or many versions of a template. Relational databases don't properly model this, logic in our dao functions is instead used for querying and aggregating data. This isn't ideal, but we couldn't think of a properly relational way to do this that didn't otherwise break normalisation etc.
Co-authored-by: Chris Hill-Scott <[email protected]>
It is never called by admin, and results in a too large payload for a service with lots of templates
rparke
force-pushed
the
rp-return-list-of-filenames-in-serialized-template-response
branch
from
3958f96 to
7779ef7
Compare
Co-authored-by: Chris Hill-Scott <[email protected]>
| is_precompiled_letter = fields.Method("get_is_precompiled_letter") | ||
| created_at = FlexibleDateTime() | ||
| updated_at = FlexibleDateTime() | ||
| template_email_files = fields.Method("get_template_email_files", allow_none=True) |
There was a problem hiding this comment.
I wonder - since this is a property that's on a template already, maybe we can call it just email_files? So when we call it, it's like template.email_files, instead of template.template_email_files? What do you reckon?
There was a problem hiding this comment.
We call it template_email_files everywhere else.
But we could just call it email_files here. I don't have a strong preference
There was a problem hiding this comment.
We call them email_files on SerialisedTemplate, too 🙌🏼 . Not feeling about this strongly - but I do think it's easier to read with less repetition.
There was a problem hiding this comment.
Makes sense, I can change it
Read the commit message carefully for background on what this is doing and why. Feel very free to push back if we think this will cause issues.