Commit f1d5962e authored by James Southern's avatar James Southern

Mark all comment notifications read

parent 21cf2d1c
......@@ -183,6 +183,12 @@ def add_intercooler_routes(config: Configurator) -> None:
'/user/{username}/default_listing_options',
factory=user_by_username,
)
add_ic_route(
'user_mark_comments_read',
'/user/{username}/mark_comments_read',
factory=user_by_username,
)
  • It seems wrong to me to attach this under /users/{username}/ seems better if it was factory=LoggedInFactory for fewer db_requests? But no where else seemed that sensible either.

    I guess I could create api/web/notifications.py and put it there?

    Edited by James Southern
  • I think this is fine. You could probably call it mark_all_notifications_read or something similar to be a bit more explicit about what it's doing, but I think it makes sense to be attached to a user, that's what it does - marks all of a particular user's notifications as read.

Please register or sign in to reply
class LoggedInFactory:
......
......@@ -5,7 +5,19 @@
{% block title %}Unread notifications{% endblock %}
{% block main_heading %}Unread notifications{% endblock %}
{% block main_heading %}Unread notifications
{% if not request.user.auto_mark_notifications_read %}
<button
class="btn btn-link-minimal ml-2"
data-ic-put-to="{{ request.route_url(
'ic_user_mark_comments_read',
username=request.user.username,
) }}",
data-ic-target="closest main"
>Mark all read</button>
{% endif %}
{% endblock %}
{% block content %}
{% if notifications %}
......
......@@ -8,11 +8,16 @@ from pyramid.httpexceptions import HTTPForbidden, HTTPUnprocessableEntity
from pyramid.request import Request
from pyramid.response import Response
from sqlalchemy.exc import IntegrityError
from sqlalchemy.dialects.postgresql import insert
  • Minor style quibbles: all the imports should be alphabetized, so this one should be above the sqlalchemy.exc line, and the zope.sqlalchemy one should be at the bottom.

Please register or sign in to reply
from zope.sqlalchemy import mark_changed
from tildes.lib.datetime import utc_now
  • This one should be grouped with the other tildes imports below (above all the tildes.models imports).

Please register or sign in to reply
from webargs.pyramidparser import use_kwargs
from tildes.enums import LogEventType, TopicSortOption
from tildes.models.log import Log
from tildes.models.user import User, UserInviteCode
from tildes.models.comment import CommentNotification
from tildes.models.topic import TopicVisit
from tildes.schemas.fields import Enum, ShortTimePeriod
from tildes.schemas.topic import TopicSchema
from tildes.schemas.user import UserSchema
......@@ -211,3 +216,58 @@ def put_filtered_topic_tags(request: Request, tags: str) -> dict:
request.user.filtered_topic_tags = result.data['tags']
return IC_NOOP
@ic_view_config(
route_name='user_mark_comments_read',
request_method='PUT',
#permission='mark_all_read'edit_default_listing_options',
  • Didn't work out where these get granted yet.

  • Permissions come from the __acl__ method on the "target" object (which in this case is the user): https://gitlab.com/tildes/tildes/blob/master/tildes/tildes/models/user/user.py#L122

    Users are currently granted all permissions on themselves (but really, it should probably be changed to be explicit about it), so you can just make up a permission name that makes sense and it will work for the user to be able to mark their own notifications as read.

Please register or sign in to reply
)
def mark_users_comments_read(request: Request) -> Response:
  • Minor thing: in general, the view functions are named with the HTTP method at the start, so this one should be put_...

Please register or sign in to reply
"""Clear all user comment notifications"""
user = request.user
notifications = (
  • Overall, this is dangerous (and one of the reasons that doing "mark all as read" is a bit tricky). Imagine I open my notifications page, and leave it open for 10 minutes or something (perhaps even just while I'm reading my notifications). After that time, I click "mark all as read", but I don't realize that 2 more notifications have come in while I've had the page open.

    If you do it this way, those 2 notifications are also going to get marked as read, without the user ever seeing them. That's probably not good, and will mean that people can miss notifications without realizing it. So it probably needs to be implemented in a way more like "mark all notifications read that were created before time X" where X is the created_time from the newest notification shown on the page when they loaded it.

    Does that make sense?

  • Perfectly.

    I'll include the notification's id in the put request.

Please register or sign in to reply
request.query(CommentNotification)
.join_all_relationships()
.filter(
CommentNotification.user == user,
CommentNotification.is_unread == True, # noqa
)
.all()
)
# If the user has the "track comment visits" feature enabled, we want to
  • This is modified from: https://gitlab.com/jms301/tildes/blob/master/tildes/tildes/views/api/web/comment.py#L300

    Possible it should be re-factored somewhere?

  • Yeah, we definitely don't want to have this big thing duplicated in here. Actually, thinking about it now - I guess this already isn't being done with the "automatically mark notifications as read" setting?

    I think it would probably be best to move this into a database trigger if that's feasible, so that it happens automatically when notifications are marked read, no matter how. I'd have to check it more carefully, but I think that should be possible.

  • Should I just nuke this bit of the code and let you implement as db_triggers? Don't think I want to delve into another new thing right now!

    Edited by James Southern
Please register or sign in to reply
# increment the number of comments they've seen in the thread that the
# comment came from, so that they don't *both* get a notification as well
# as have the thread highlight with "(1 new)". This should only happen if
# their last visit was before the comment was posted, however.
# Below, this is implemented as a INSERT ... ON CONFLICT DO UPDATE so that
# it will insert a new topic visit with 1 comment if they didn't previously
# have one at all.
if user.track_comment_visits:
for notification in notifications:
statement = (
insert(TopicVisit.__table__)
.values(
user_id=user.user_id,
topic_id=notification.comment.topic_id,
visit_time=utc_now(),
num_comments=1,
)
.on_conflict_do_update(
constraint=TopicVisit.__table__.primary_key,
set_={'num_comments': TopicVisit.num_comments + 1},
where=TopicVisit.visit_time < notification.comment.created_time,
)
)
request.db_session.execute(statement)
mark_changed(request.db_session)
for notification in notifications:
notification.is_unread = False
  • It's not too big of a deal in practice, but if we can get rid of the need to do the TopicVisit updates above, this whole thing could probably just be done with a single update statement. That is, you'd just change the query above so that instead of fetching all the notifications with .all(), you do something like .update({'is_unread': False}). That's a bit more efficient, but will only work if we don't need to actually do anything with the notifications inside the function here.

  • I agree but I think the db triggers are probably a bit beyond me for the moment. Could leave it in and open an issue to re factor out the two TopicVisit blocks after replacing with a db trigger?

  • Yeah that's fine, just go ahead and duplicate it for now, and I'll look into whether it's feasible to convert to a trigger in the future (there might be some trickiness to it that I'm not thinking of right now).

Please register or sign in to reply
return Response('Your comment notifications have been cleared.')
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment