Refactors Personalizations logic to simplify flow

[sklarsa]

Will end up closing #92

Still a WIP though..

[sklarsa]

@alichass what do you think about the direction that I'm going in here?

[alichass]

@alichass what do you think about the direction that I'm going in here?

Does definitely make sense in terms of trying to maximize the flexibility of personalization customization for the user .

I just personally wanted a user (me included) to avoid trying to have to construct Personalization objects (and therefore avoid having to learn the nitty gritty of the sendgrid python package), which is why I just went with a simple list of dicts that should match the attrs of the "standard" way this package is used.

Perhaps there's room for both? that is to say msg.personalizations could be list[dict] or list[Personalization] and similarly _build_sg_personalization could take either dict or Personalization as the existing_personalizations attr

[sklarsa]

Perhaps there's room for both?

Yeah definitely! Just requires a little more coding on my part. Should have time today or tomorrow to clean this up

[alichass]

Yeah definitely! Just requires a little more coding on my part. Should have time today or tomorrow to clean this up

Let me know if you want any help

[sklarsa]

If you can wait until the end of the week, I'll try to finish it up before then

[sklarsa]

@alichass I added Dict compatibility for personalizations. LMK your thoughts and if you don't have any objections, we can get this merged and released

[sklarsa]

closes #87 closes #92

[alichass]

Hi @sklarsa sorry for the late reply - this looks great, I do have a question though, namely regarding some of the logic with "tos".

line 435

The personalizations list[dict] or list[Personalization] dosnt really make sense without a "to" key/ "tos" attr in each respectively- otherwise, from the code it seems like what would happen is every email in msg.to would get every personalization row sent to them (if not given).

dict_to_personalization

Just wondering, is the package now expecting the name of the "to" keys in list[dict] to be "tos"? No issues at all with this change just need to account for on my end whenever this gets released. It seems like "tos" are being set twice - see below.

lines 274-279

If i've understood this correctly, Personalization_object.tos is an iterable of non-strings. However - the only time it would be passed in is if msg.personalizations is a list[Personalization] or list[dict] in which case it seems like personalization.tos would just be overwritten by itself (current code with my comments below)

if to: # This should always be the case if msg.personalizations is not set
    # populate a personalization with msg.to 
    if type(to[0]) == str:
        for addr in to:
            personalization.add_to(Email(*self._parse_email_address(addr)))
    else:   # will only every happen if msg.personalizations is set  - unnecessary overwrite
        personalization.tos = to

would this not make more sense?

if not personalization.tos:
    if type(to[0]) == str:
        for addr in to:
            personalization.add_to(Email(*self._parse_email_address(addr)))
    else:
        personalization.tos = to

Thanks again!

[sklarsa]

Yes great catch! Can you make a PR with that fix?

As for the dict schema, I guess we could accept "to" or "tos".. since we're accepting a Dict and defining our own schema, we can choose whatever is the most useful :-)

[alichass]

Yes great catch! Can you make a PR with that fix?

As for the dict schema, I guess we could accept "to" or "tos".. since we're accepting a Dict and defining our own schema, we can choose whatever is the most useful :-)

Will do (i'll catch both)

[alichass]

yo sorry for the delay - will have a PR by the end of the weekend