Adds API for activity feed #11690
Adds API for activity feed #11690
Conversation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
This PR adds a new API for patron activity feeds that displays recent bookshelf events from either followed accounts or trending public reading logs. The API is exposed through a public get_activity_feed function which returns up to 3 recent events with a 5-minute cache.
Changes:
- Added
ActivityFeedclass with methods to retrieve and cache activity feeds from followed users or trending public logs - Added
PubSub.is_followingmethod to check if a patron follows any accounts - Implemented caching with 5-minute TTL for both feed types with datetime deserialization support
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
openlibrary/plugins/upstream/mybooks.py |
Adds ActivityFeed class with feed retrieval logic, caching, and public API function get_activity_feed |
openlibrary/core/follows.py |
Adds is_following method to check if a user follows anyone |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @classmethod | ||
| def is_following(cls, username): | ||
| oldb = db.get_db() | ||
| query = """ | ||
| SELECT EXISTS( | ||
| SELECT 1 | ||
| FROM follows | ||
| WHERE subscriber=$subscriber | ||
| ) | ||
| """ | ||
| result = oldb.query(query, vars={'subscriber': username}) | ||
| return result and result[0].get('exists', False) |
There was a problem hiding this comment.
The new is_following method in PubSub lacks test coverage. Given that the repository has comprehensive test coverage for similar database operations, tests should be added to verify: (1) Returns True when a user has at least one subscription, (2) Returns False when a user has no subscriptions, (3) Handles edge cases like non-existent usernames gracefully.
| Bookshelves.add_solr_works(logged_books) | ||
|
|
There was a problem hiding this comment.
The loop condition if len(feed) > 2 will break when the feed has 3 items, but the method's documentation and the issue requirements state it should return "up to three" events. This off-by-one error means the function could return 3 items when it should stop at 3 (the condition should be >= 3 to stop once we have 3 items). However, given that the requirements ask for "up to three" and the docstring says "up to three", the current implementation is actually correct. But the use of > 2 is less clear than >= 3 for readability.
| if acct := OpenLibraryAccount.get_by_username(_username): | ||
| user = acct.get_user() | ||
| return user and user.preferences().get('public_readlog', 'no') == 'yes' | ||
| return False |
There was a problem hiding this comment.
The nested function has_public_reading_log is defined but could benefit from error handling. If acct.get_user() returns None (which is checked), but if user.preferences() raises an exception or returns None, this could cause an AttributeError. Consider adding a try-except block or additional None checks to make the function more robust.
| if acct := OpenLibraryAccount.get_by_username(_username): | |
| user = acct.get_user() | |
| return user and user.preferences().get('public_readlog', 'no') == 'yes' | |
| return False | |
| try: | |
| acct = OpenLibraryAccount.get_by_username(_username) | |
| if not acct: | |
| return False | |
| user = acct.get_user() | |
| if not user: | |
| return False | |
| prefs = user.preferences() | |
| if not prefs: | |
| return False | |
| return prefs.get('public_readlog', 'no') == 'yes' | |
| except Exception: | |
| # If preferences are unavailable or an error occurs, treat the reading log as non-public | |
| return False |
| def get_activity_feed(cls, username): | ||
| """Returns up to three of the most recent events from one of two feeds. | ||
| If the given user is not following others, the feed will contain recently | ||
| logged books. Otherwise, the feed will contain events from the patron's | ||
| `PubSub.get_feed` results. | ||
| """ | ||
| following = PubSub.is_following(username) | ||
| if following: | ||
| feed = cls.get_cached_pub_sub_feed(username) | ||
| else: | ||
| feed = cls.get_cached_trending_feed(username) | ||
|
|
||
| return feed or [], following | ||
|
|
||
| @classmethod | ||
| def get_pub_sub_feed(cls, username): | ||
| feed = PubSub.get_feed(username, limit=3) | ||
| return feed | ||
|
|
||
| @classmethod | ||
| def get_cached_pub_sub_feed(cls, username): | ||
| five_minutes = 5 * dateutil.MINUTE_SECS | ||
| mc = memcache_memoize( | ||
| cls.get_pub_sub_feed, | ||
| key_prefix="mybooks.pubsub.feed", | ||
| timeout=five_minutes, | ||
| prethread=caching_prethread(), | ||
| ) | ||
| results = mc(username) | ||
|
|
||
| for r in results: | ||
| if isinstance( | ||
| r['created'], str | ||
| ): # `datetime` objects are stored in cache as strings | ||
| # Update `created` to datetime, which is the type expected by `datestr` (called in card template) | ||
| r['created'] = datetime.fromisoformat(r['created']) | ||
| return results | ||
|
|
||
| @classmethod | ||
| def get_trending_feed(cls, username): | ||
| def has_public_reading_log(_username): | ||
| if acct := OpenLibraryAccount.get_by_username(_username): | ||
| user = acct.get_user() | ||
| return user and user.preferences().get('public_readlog', 'no') == 'yes' | ||
| return False | ||
|
|
||
| logged_books = Bookshelves.get_recently_logged_books(limit=10) | ||
| Bookshelves.add_solr_works(logged_books) | ||
|
|
||
| feed = [] | ||
| for idx, item in enumerate(logged_books): | ||
| if ( | ||
| item['work'] | ||
| and item['username'] != username | ||
| and has_public_reading_log(item['username']) | ||
| ): | ||
| feed.append(item) | ||
| if len(feed) > 2: | ||
| break | ||
|
|
||
| return feed | ||
|
|
||
| @classmethod | ||
| def get_cached_trending_feed(cls, username): | ||
| five_minutes = 5 * dateutil.MINUTE_SECS | ||
| mc = memcache_memoize( | ||
| cls.get_trending_feed, |
There was a problem hiding this comment.
The get_activity_feed method returns a tuple (feed, following), but the public API function get_activity_feed at line 724 also returns this tuple. The return type is not documented anywhere. Consider adding type hints to both methods to clarify what is being returned, especially since the second value following is a boolean that indicates whether the user is following others.
| else: | ||
| feed = cls.get_cached_trending_feed(username) | ||
|
|
||
| return feed or [], following |
There was a problem hiding this comment.
In get_activity_feed, line 670 returns feed or [], following. If feed is None or falsy, it returns an empty list. However, looking at the methods get_cached_pub_sub_feed and get_cached_trending_feed, they both return the results from the cache, which could be an empty list []. The expression [] or [] evaluates to [], and None or [] also evaluates to [], so this works correctly. However, the logic is slightly confusing. Consider being more explicit, such as return feed if feed is not None else [], following or ensure the cached methods never return None.
| return feed or [], following | |
| return (feed if feed is not None else []), following |
| class ActivityFeed: | ||
|
|
||
| @classmethod | ||
| def get_activity_feed(cls, username): | ||
| """Returns up to three of the most recent events from one of two feeds. | ||
| If the given user is not following others, the feed will contain recently | ||
| logged books. Otherwise, the feed will contain events from the patron's | ||
| `PubSub.get_feed` results. | ||
| """ | ||
| following = PubSub.is_following(username) | ||
| if following: | ||
| feed = cls.get_cached_pub_sub_feed(username) | ||
| else: | ||
| feed = cls.get_cached_trending_feed(username) | ||
|
|
||
| return feed or [], following | ||
|
|
||
| @classmethod | ||
| def get_pub_sub_feed(cls, username): | ||
| feed = PubSub.get_feed(username, limit=3) | ||
| return feed | ||
|
|
||
| @classmethod | ||
| def get_cached_pub_sub_feed(cls, username): | ||
| five_minutes = 5 * dateutil.MINUTE_SECS | ||
| mc = memcache_memoize( | ||
| cls.get_pub_sub_feed, | ||
| key_prefix="mybooks.pubsub.feed", | ||
| timeout=five_minutes, | ||
| prethread=caching_prethread(), | ||
| ) | ||
| results = mc(username) | ||
|
|
||
| for r in results: | ||
| if isinstance( | ||
| r['created'], str | ||
| ): # `datetime` objects are stored in cache as strings | ||
| # Update `created` to datetime, which is the type expected by `datestr` (called in card template) | ||
| r['created'] = datetime.fromisoformat(r['created']) | ||
| return results | ||
|
|
||
| @classmethod | ||
| def get_trending_feed(cls, username): | ||
| def has_public_reading_log(_username): | ||
| if acct := OpenLibraryAccount.get_by_username(_username): | ||
| user = acct.get_user() | ||
| return user and user.preferences().get('public_readlog', 'no') == 'yes' | ||
| return False | ||
|
|
||
| logged_books = Bookshelves.get_recently_logged_books(limit=10) | ||
| Bookshelves.add_solr_works(logged_books) | ||
|
|
||
| feed = [] | ||
| for idx, item in enumerate(logged_books): | ||
| if ( | ||
| item['work'] | ||
| and item['username'] != username | ||
| and has_public_reading_log(item['username']) | ||
| ): | ||
| feed.append(item) | ||
| if len(feed) > 2: | ||
| break | ||
|
|
||
| return feed | ||
|
|
||
| @classmethod | ||
| def get_cached_trending_feed(cls, username): | ||
| five_minutes = 5 * dateutil.MINUTE_SECS | ||
| mc = memcache_memoize( | ||
| cls.get_trending_feed, |
There was a problem hiding this comment.
The new ActivityFeed class and the is_following method in PubSub lack test coverage. Given that the repository has comprehensive test coverage for similar core functionality (e.g., Bookshelves and BookshelvesEvents in openlibrary/tests/core/test_db.py), tests should be added to verify the behavior of these new methods. Recommended tests include: (1) Testing is_following returns True when a user follows someone and False otherwise, (2) Testing get_activity_feed returns the correct feed type based on following status, (3) Testing the caching behavior of both feed types, (4) Testing get_trending_feed correctly filters out private reading logs and the current user's activities.
| def has_public_reading_log(_username): | ||
| if acct := OpenLibraryAccount.get_by_username(_username): | ||
| user = acct.get_user() | ||
| return user and user.preferences().get('public_readlog', 'no') == 'yes' | ||
| return False | ||
|
|
There was a problem hiding this comment.
The get_trending_feed method has a significant performance issue. For each item in logged_books (up to 10 items), it calls has_public_reading_log which performs a database lookup via OpenLibraryAccount.get_by_username. This creates an N+1 query problem where you could have up to 10 additional database queries executed in a loop. Consider optimizing this by either: (1) batch-fetching all account information upfront, or (2) implementing a more efficient SQL query that joins the necessary tables to filter accounts with public reading logs in a single query.
| def has_public_reading_log(_username): | |
| if acct := OpenLibraryAccount.get_by_username(_username): | |
| user = acct.get_user() | |
| return user and user.preferences().get('public_readlog', 'no') == 'yes' | |
| return False | |
| # Cache per-username reading log visibility within this call to avoid | |
| # repeated database lookups for the same account. | |
| public_log_cache = {} | |
| def has_public_reading_log(_username): | |
| if _username in public_log_cache: | |
| return public_log_cache[_username] | |
| result = False | |
| if acct := OpenLibraryAccount.get_by_username(_username): | |
| user = acct.get_user() | |
| result = bool( | |
| user and user.preferences().get('public_readlog', 'no') == 'yes' | |
| ) | |
| public_log_cache[_username] = result | |
| return result |
| if acct := OpenLibraryAccount.get_by_username(_username): | ||
| user = acct.get_user() | ||
| return user and user.preferences().get('public_readlog', 'no') == 'yes' | ||
| return False |
There was a problem hiding this comment.
The get_trending_feed method filters items checking both item['work'] exists and that the username is not the current user. However, if no items pass the filter conditions, the loop could iterate through all 10 items without finding 3 valid ones. The function will return fewer than 3 items in this case, which is acceptable, but the enumeration with idx is not used. Remove the unused idx variable from the loop.
| timeout=five_minutes, | ||
| prethread=caching_prethread(), | ||
| ) | ||
| results = mc(username) | ||
|
|
||
| for r in results: | ||
| if isinstance( | ||
| r['created'], str | ||
| ): # `datetime` objects are stored in cache as strings | ||
| # Update `created` to datetime, which is the type expected by `datestr` (called in card template) | ||
| r['created'] = datetime.fromisoformat(r['created']) | ||
| return results | ||
|
|
||
| @classmethod | ||
| def get_trending_feed(cls, username): | ||
| def has_public_reading_log(_username): | ||
| if acct := OpenLibraryAccount.get_by_username(_username): | ||
| user = acct.get_user() | ||
| return user and user.preferences().get('public_readlog', 'no') == 'yes' | ||
| return False | ||
|
|
||
| logged_books = Bookshelves.get_recently_logged_books(limit=10) | ||
| Bookshelves.add_solr_works(logged_books) | ||
|
|
||
| feed = [] | ||
| for idx, item in enumerate(logged_books): | ||
| if ( | ||
| item['work'] | ||
| and item['username'] != username | ||
| and has_public_reading_log(item['username']) | ||
| ): | ||
| feed.append(item) | ||
| if len(feed) > 2: | ||
| break | ||
|
|
||
| return feed | ||
|
|
There was a problem hiding this comment.
The datetime conversion logic modifies the cached results in-place. While this works, the same conversion code is duplicated in both get_cached_pub_sub_feed and get_cached_trending_feed. Consider extracting this datetime conversion logic into a shared helper method to follow the DRY principle and improve maintainability.
| item['work'] | ||
| and item['username'] != username | ||
| and has_public_reading_log(item['username']) | ||
| ): | ||
| feed.append(item) |
There was a problem hiding this comment.
The cache key prefix for get_cached_pub_sub_feed is "mybooks.pubsub.feed" and for get_cached_trending_feed is "mybooks.trending.feed". Both methods receive username as a parameter, which is used to generate cache keys. However, the memcache_memoize function will include the username in the cache key automatically. This means different users will have different cache entries, which is correct for get_pub_sub_feed (since each user has different followings), but for get_trending_feed, the username is only used for filtering out the user's own activities. This means all users who don't follow anyone will get the same trending feed, but it will be cached separately for each user, which is inefficient. Consider caching the trending feed globally and filtering out the current user's activities after retrieving from cache.
2 participants
Advances #10242
Adds API for patron activity feeds.
New API is exposed via
publicfunctionget_activity_feed, which takes a patron'susernameand returns a list of up to three recent bookshelf events. If the patron follows others, the list will be comprised of events generated by followed accounts. Otherwise, the list will contain the must recent bookshelf events triggered by patrons that have a public reading log.Results are cached for five minutes.
Technical
A new
PubSubmethod namedis_followinghas been added to support this feature.is_followingreturnsTrueif the patron follows any account.ActivityFeed'sget_pub_sub_feedmethod returns a list of recent activity from accounts that a patron follows. If the patron does not follow others, theget_trending_feedmethod is used to fetch the recent activity from accounts that have public reading logs. This works by first fetching the10most recent events, then filtering out events that were generated by accounts having private reading logs.Testing
Screenshot
Stakeholders