Skip to content

Conversation

@tbkot
Copy link
Contributor

@tbkot tbkot commented Aug 31, 2020

Problem

There is an error in \Drupal\activity_creator\ActivityNotifications::deleteNotificationsByIds. On line 260 of ActivityNotifications.php the line ->condition('aid', $activity_ids) is used, where $activity_ids is an array. However, the default comparison operator for condition is = which causes issues in the query, resulting in unremoved activities.

Solution

  • Ensure the correct operator is used in the condition, in this case, add "IN" as the third argument. Similar to what's done on line 196 of the same file
  • Create an update hook that cleans up any notifications for whom the user or associated entity no longer exists.

Issue tracker

https://www.drupal.org/project/social/issues/3162557

…eate update hook for removing not used notifications.
@tbkot tbkot added type: bug Fixes a bug in Open Social team: enterprise This PR originates from the ECI team status: needs review This pull request is waiting for a requested review labels Aug 31, 2020
@ribel ribel added the needs work: maintainer input This pull request needs input from a tech lead label Sep 8, 2020
@ribel ribel self-requested a review September 8, 2020 09:49
Copy link
Contributor

@ribel ribel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@ribel
Copy link
Contributor

ribel commented Sep 8, 2020

Hey @ronaldtebrake,
We had issues when users received notification about entities that were already deleted.
This PR should prevent this. I think @Kingdutch suggested solution, which @dmytriikaiun implemented.
Could you check if we can include this in upcoming 8.5 release?

@ribel ribel removed the status: needs review This pull request is waiting for a requested review label Sep 8, 2020
@ribel ribel added this to the 8.5 milestone Sep 8, 2020
@ronaldtebrake
Copy link
Contributor

Hi @ribel, nice work looks good to me also in 8.5! :)

@ribel ribel removed the needs work: maintainer input This pull request needs input from a tech lead label Sep 9, 2020
@ribel ribel changed the base branch from 8.x-9.x to 10.0.x September 9, 2020 12:23
@ribel ribel changed the title Issue #3162557 by Kingdutch: Update delete notification condition. Cr… Issue #3162557 by tBKoT, Kingdutch: Update delete notification condition Sep 9, 2020
@ribel ribel merged commit ac11c1f into 10.0.x Sep 9, 2020
@ribel ribel deleted the feature/3162557-notification-are-not-actualy-deleted branch September 9, 2020 12:24
ribel pushed a commit that referenced this pull request Sep 9, 2020
…ion. Create update hook for removing not used notifications. (#1965)
ribel pushed a commit that referenced this pull request Sep 9, 2020
…ion. Create update hook for removing not used notifications. (#1965)
@ribel ribel added the backport: verified This pull request has been back ported to an older minor version label Sep 9, 2020
@ribel
Copy link
Contributor

ribel commented Sep 9, 2020

🍒 picked to 8.x and 9.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport: verified This pull request has been back ported to an older minor version team: enterprise This PR originates from the ECI team type: bug Fixes a bug in Open Social

Development

Successfully merging this pull request may close these issues.

4 participants