Details

    • Type: Sub-task
    • Status: Unverified Fix
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 3.1.0-b2
    • Fix Version/s: 3.1.0-b3
    • Component/s: Notification System
    • Labels:
      None

      Description

      Scenario 1:

      1. Subscribe to a forum as UserA
      2. Reply as UserB
      3. Reply as UserC
      4. Put UserB on Moderation Queue
      5. Edit the reply as UserB

      Expected: UserA has a notification for a new reply by UserC
      Actual: UserA has no notification at all

      Scenario 2:

      1. Subscribe to a forum as UserA
      2. Reply as UserB
      3. Reply as UserC
      4. Put UserC on Moderation Queue
      5. Edit the reply as UserC

      Expected: UserA has a notification for a new reply by UserB
      Actual: UserA has a notification for a new reply by "UserB and UserC"

        Activity

        Hide
        nickvergessen Joas Schilling added a comment -

        Okay, after some stumbling through the code there are 2 possible ways to fix this:

        Expand the responders array

        We could expand the responders array in notification_data with the new item id. However this makes adding/deleting of a notification really painful:

        • We add a lot of data to the notification_data field (which is of type text). *Note:* there is a risk of running into a SQL-Error when a lot of people respond to a post even now (length is never checked). We store a serialized array of [ poster_id, username ]:

          i:3;a:2:{s:9:"poster_id";i:48;s:8:"username";s:0:"";}

          which is about 53+ characters per user (max. 1.13k users allowed). With post ID added, we end up with

          i:3;a:3:{s:7:"post_id";i:58;s:9:"poster_id";i:48;s:8:"username";s:0:"";}

          (73+ characters) per response (max. 820 responses) (NOTE response vs user difference!)

        • To add a new reponse, we just need to add the value to the array (no big change)
        • To display a notification, we need to array_unique() the user names before using them
        • When deleting a post, we need to check whether responders is set and whether this post is set as a responder. If it is the notification and responders are set, we take the first responder and make it the notification. If the post is set as a responder, we need to delete it from there. (This requires a notification_data LIKE 'i:<postid>;' query while deleting notifications, not the best idea on the biggest table of your board...)

        Remove the responders array, add subsequent notifications as their own rows

        • This circumvents the field length problem of solution 1
        • To add a new "response", we simply add a new notification
        • To display notifications, we need to group them by the topic-ID (item_parent_id) and generate the responders array on the fly. Note: as we display: "a, b, c and x other users" for responders, we can just grab the number of notifications and the last (n - x) which shouldn't be too difficult/heavy. To save some db ressources, we could also add a "branch" column to the notification table and give responses the same branch id (current notification id), until it is marked as read. (Should be fairly easy)
        • When deleting the post, we just drop the notification by item_id

        I think option 2 is the better choice, as it does not run into problems with bigger boards as easy
        Any opinions?

        Show
        nickvergessen Joas Schilling added a comment - Okay, after some stumbling through the code there are 2 possible ways to fix this: Expand the responders array We could expand the responders array in notification_data with the new item id. However this makes adding/deleting of a notification really painful: We add a lot of data to the notification_data field (which is of type text). * Note: * there is a risk of running into a SQL-Error when a lot of people respond to a post even now (length is never checked). We store a serialized array of [ poster_id, username ]: i:3;a:2:{s:9:"poster_id";i:48;s:8:"username";s:0:"";} which is about 53+ characters per user (max. 1.13k users allowed). With post ID added, we end up with i:3;a:3:{s:7:"post_id";i:58;s:9:"poster_id";i:48;s:8:"username";s:0:"";} (73+ characters) per response (max. 820 responses) (NOTE response vs user difference!) To add a new reponse, we just need to add the value to the array (no big change) To display a notification, we need to array_unique() the user names before using them When deleting a post, we need to check whether responders is set and whether this post is set as a responder. If it is the notification and responders are set, we take the first responder and make it the notification. If the post is set as a responder, we need to delete it from there. (This requires a notification_data LIKE 'i:<postid>;' query while deleting notifications, not the best idea on the biggest table of your board...) Remove the responders array, add subsequent notifications as their own rows This circumvents the field length problem of solution 1 To add a new "response", we simply add a new notification To display notifications, we need to group them by the topic-ID ( item_parent_id ) and generate the responders array on the fly. Note: as we display: "a, b, c and x other users" for responders, we can just grab the number of notifications and the last (n - x) which shouldn't be too difficult/heavy. To save some db ressources, we could also add a "branch" column to the notification table and give responses the same branch id (current notification id), until it is marked as read. (Should be fairly easy) When deleting the post, we just drop the notification by item_id I think option 2 is the better choice, as it does not run into problems with bigger boards as easy Any opinions?
        Hide
        nickvergessen Joas Schilling added a comment -

        Note: We use 'TEXT_UNI' for notification_data which is limited to 4k chars on the smallest DB.
        So the numbers mentioned above have to be divided by 16:
        75 users vs 53 responses

        Show
        nickvergessen Joas Schilling added a comment - Note: We use 'TEXT_UNI' for notification_data which is limited to 4k chars on the smallest DB. So the numbers mentioned above have to be divided by 16: 75 users vs 53 responses
        Hide
        nickvergessen Joas Schilling added a comment -

        Another idea brought up by bantu is to make board-notifications work exactly like emails/jabber:
        Do not add notifications (responders) for additional posts, until the user has read the topic
        This resolves the problems mentioned above, as we wouldn't delete the notification anymore and the notification_data would not grow infinite.
        However it reduces the fancyness of notifications quite a bit, as they are not up to date anymore.

        Show
        nickvergessen Joas Schilling added a comment - Another idea brought up by bantu is to make board-notifications work exactly like emails/jabber: Do not add notifications (responders) for additional posts, until the user has read the topic This resolves the problems mentioned above, as we wouldn't delete the notification anymore and the notification_data would not grow infinite. However it reduces the fancyness of notifications quite a bit, as they are not up to date anymore.
        Hide
        EXreaction EXreaction [X] (Inactive) added a comment -

        Option 2 and grouping by the item_parent_id and notification type sounds like a nice option to me, then it can be applied towards other notification types as well.

        Show
        EXreaction EXreaction [X] (Inactive) added a comment - Option 2 and grouping by the item_parent_id and notification type sounds like a nice option to me, then it can be applied towards other notification types as well.

          People

          • Assignee:
            nickvergessen Joas Schilling
            Reporter:
            nickvergessen Joas Schilling
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development