-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix #611, Ref #708: fix Acoustic newsletters subscription timestamps #710
Conversation
In this pull-request, I fix a piece of code that never worked: the newsletter subscription timestamps sent to Acoustic were never read from the database. Basically schemas do not read the timestamps fields from the DB. This could be a possible fix, but my confidence in its elegance is kind of low. This codebase has a really dark power of demotivation. Especially if you ever worked with tools like Django Rest Framework. Plus, most test case assertions are not designed in a flexible way.
# In the `ContactSchema` instance, the newsletters and waitlists are | ||
# instances of `NewsletterSchema` and `WaitlistSchema`, | ||
# which loose the `create_timestamp` and `update_timestamp` fields. | ||
# Note: the whole code base with regards to models and schemas is a real house of cards. | ||
contact_newsletters = get_newsletters_by_email_id(db, email_id) | ||
contact.newsletters = [ | ||
NewsletterTableSchema.from_newsletter(nl) for nl in contact_newsletters | ||
] | ||
contact_waitlists = get_waitlists_by_email_id(db, email_id) | ||
contact.waitlists = [ | ||
WaitlistTableSchema.from_waitlist(wl) for wl in contact_waitlists | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of putting my entire initial review here because I think this section represents the "main idea" of this PR -- when we build a contact, include created
and updated
timestamps with waitlists and newsletters. Here are my thoughts:
- Is there a reason we only want to do this waitlist and newsletter "patching" for contacts here in this function? It seems like we'd want to include these everywhere, so we may want to change the
ContactSchema
from
class ContactSchema(ComparableBase):
"""A complete contact."""
...
newsletters: List[NewsletterSchema] = []
waitlists: List[WaitlistSchema] = []
to
class ContactSchema(ComparableBase):
"""A complete contact."""
...
newsletters: List[NewsletterTableSchema] = []
waitlists: List[WaitlistTableSchema] = []
or add the missing fields directly to the existing schemas.
In fact, saying that this function returns a ContactSchema
might be "cheating" a bit since we define a Contact's newsletters and waitlists as NewsletterSchema
s and WaitlistSchema
s respectively, but now we're returning different models.
- if that doesn't work and we only want these new schemas here, I think that if they are truly a 1:1 mapping to the SQLAlchemy models, we can use Pydantic's ORM Mode rather than creating these custom class methods to convert ORM objects to Pydantic models
Replaced by #711 |
Fix #611, Ref #708
In this pull-request, I fix a piece of code that never worked: the newsletter subscription timestamps sent to Acoustic were never read from the database.
Basically schemas do not read thetimestamps fields from the DB. This could be a possible fix, but my confidence in its elegance is kind of low.
This codebase has a really dark power of demotivation. Especially if you ever worked with tools like Django Rest Framework.
Plus, most test case assertions are not designed in a flexible way.