Add subscriptions functionality #194
Conversation
Robin5605
force-pushed
the
subscriptions
branch
2 times, most recently
from
050b646 to
dbd93d9
Compare
Robin5605
force-pushed
the
subscriptions
branch
from
dbd93d9 to
bcd618f
Compare
This should cover the if branch in the `create_subscription_route` function that looks up by email address, if it's given.
Robin5605
requested review from
sid-maddy,
shenanigansd,
AbooMinister25,
jonathan-d-zhang and
import-pandas-as-numpy
| if nn_name: | ||
| query = query.where(Scan.name == name) | ||
| if nn_version: | ||
| query = query.where(Scan.version == version) | ||
| if nn_since: | ||
| query = query.where(Scan.finished_at >= dt.datetime.fromtimestamp(since, tz=dt.timezone.utc)) | ||
|
|
||
| data = session.scalars(query) | ||
| packages = session.scalars(query).unique().all() | ||
|
|
||
| log.info("Package information queried") |
There was a problem hiding this comment.
Can this be debug or lower
|
|
||
|
|
||
| class AddSubscription(Subscription): | ||
| """Payload for when a user wants to subscribe to a malicious package.""" |
There was a problem hiding this comment.
Not necessarily malicious.
| """Payload for when a user wants to subscribe to a malicious package.""" | |
| """Payload for when a user wants to subscribe to a package.""" |
|
|
||
|
|
||
| class RemoveSubscription(Subscription): | ||
| """Payload for when a user wants to unsubscribe from a malicious package.""" |
There was a problem hiding this comment.
| """Payload for when a user wants to unsubscribe from a malicious package.""" | |
| """Payload for when a user wants to unsubscribe from a package.""" |
| except PackageNotFoundError: | ||
| continue | ||
|
|
||
| package = session.scalar(select(Package).where(Package.name == package_metadata.title)) |
There was a problem hiding this comment.
We should only make 3 queries in this endpoint. 1 to deduplicate and 2 to insert the new packages and scans. We should collect all the Scans at once, then insert everything at the end, instead of querying for every package.
| package = session.scalar(select(Package).where(Package.name == name)) | ||
| if package is None: | ||
| package = Package(name=name) | ||
|
|
||
| package.scans.append(scan) | ||
| session.add(package) |
There was a problem hiding this comment.
We can minimize overhead here by inserting both of these in one query, rather than sending a query, running some logic, then sending another query.
|
|
||
| person = session.scalar(query) | ||
| if not person: | ||
| person = Person(discord_id=payload.discord_id, email_address=payload.email_address) |
There was a problem hiding this comment.
Having this endpoint be the only way to create Person entries seems off. We need some way to manage these.
| person = session.scalar(select(Person).where(Person.id == payload.user_id)) | ||
| if person is None: | ||
| raise HTTPException(404, detail="A user with that ID could not be found") | ||
|
|
||
| package = session.scalar(select(Package).where(Package.name == payload.package_name)) | ||
| if package is None: | ||
| raise HTTPException(404, detail="A package with that name could not be found") | ||
|
|
||
| person.packages.remove(package) | ||
|
|
||
| session.commit() |
There was a problem hiding this comment.
Can't we delete directly from the subscriptions table and save a query?
| if person is None: | ||
| raise HTTPException(404, detail="A user with that ID could not be found") | ||
|
|
||
| subscribed_packages = [package.name for package in person.packages] |
There was a problem hiding this comment.
nit: Either put this list comp inline with the GetPerson creation like in get_all_people_route or make the same variable in the other function.
| @@ -13,7 +13,8 @@ def oldest_queued_package(db_session: Session): | |||
| return db_session.scalar(select(func.min(Scan.queued_at)).where(Scan.status == Status.QUEUED)) | |||
|
|
|||
|
|
|||
| def test_min_queue_date_of_queued_rows(test_data: list[Scan], db_session: Session): | |||
| def test_min_queue_date_of_queued_rows(db_session: Session): | |||
| test_data = db_session.scalars(select(Scan)).all() | |||
There was a problem hiding this comment.
Why get rid of the fixture to just grab the data on the first line for every test?
| queued_at=datetime.now(), | ||
| queued_by="remmy", | ||
| ) | ||
| person = Person(discord_id=123, email_address="[email protected]") |
There was a problem hiding this comment.
Maybe extract this and other repeated data to a fixture
Closes #193