Error-checking in Mail helper/ASM #413
Conversation
If the optional ASM is specified, the API says the group_id is mandatory.
API says "You can specify up to 25 groups to display." If param is too long, raise ValueError.
thinkingserious
added
the
status: code review request
Codecov Report
@@ Coverage Diff @@
## master #413 +/- ##
=========================================
Coverage ? 83.36%
=========================================
Files ? 30
Lines ? 1022
Branches ? 159
=========================================
Hits ? 852
Misses ? 82
Partials ? 88
Continue to review full report at Codecov.
|
Do we really need this? It makes Codecov happy though.
|
Oops, just saw the header "The following objects are meant to be extended with validation". 🤦 If that's still true you can close this. Otherwise I'll add some more elsewhere in the helper. |
There was a problem hiding this comment.
This looks good, I added a couple comments to see what you think.
Thanks, @gabrielkrell!!
sendgrid/helpers/mail/mail.py
Outdated
|
|
||
| if groups_to_display is not None: | ||
| self._groups_to_display = groups_to_display | ||
| def __init__(self, group_id, groups_to_display=None): |
There was a problem hiding this comment.
What do you think about changing L710 to be
self.groups_to_display = groups_to_display
and then moving the validation and exception to line 724 ?
I think that this would allow any setting of the groups to be handled in the same way, whether by init or otherwise.
There was a problem hiding this comment.
Should we also add logic to group_id that causes a similar exception if it is turned on and there are more than 25 groups?
There was a problem hiding this comment.
You're right; I'll put that in the property.
If you're OK with doing error-checking in Mail I'll do that and look for other API limits that we can verify. I think there are a few character limits somewhere.
There was a problem hiding this comment.
I think that it makes sense to try to catch the limitations in the natural and earliest bottlenecks. That way, the code fails quickly, rather than after running all the way through and then failing.
There was a problem hiding this comment.
Oh I mean like in the Mail helper and the small classes (e.g. Personalization), not just the Mail class. I saw that big comment about leaving validation up to the user so I wanted to make sure it was OK to do it ourselves here.
Oops. Move groups_to_display validation to setter, not init.
There was a problem hiding this comment.
Hey @gabrielkrell can you double check the merge conflicts? It looks like #472 may have conflicts with your work here.
|
@gabrielkrell I just saw your reply ^ |
gabrielkrell
changed the title
|
@gabrielkrell are we good to go? (Don't worry about the codeclimate report right now) |
gabrielkrell
changed the title
|
Yeah I think this is ready. It'll stay a small PR for now; I haven't had time to put in checks for other API limits. |
|
Hello @gabrielkrell, |
Among other things, API docs say that if the
asmfield is provided,group_idis required andgroups_to_displayis limited to 25 groups. Change Mail helper's ASM class so that it (a) requires agroup_idand (b) raises an exception if more than 25 groups to display are provided.There are some other limits that the API specifies, but I guess they'll go in separate PRs since I probably won't finish them within the next few days.