Patch #37065

When someone is member of watcher group, 'watched_by' may be wrong and incomplete

Added by salman mp about 1 month ago. Updated 2 days ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Marius BALTEANU% Done:

0%

Category:Email notifications
Target version:5.0.2

Description

Suppose that 'user' is member of 'group'. Result of issue.watched_by is limited to those issues that 'user' is watcher of theme, and those issues that 'group' is watchers of them and 'user' is no watcher, excluded.

Related to #4511

count_on_group_watchers.patch Magnifier (1003 Bytes) salman mp, 2022-05-01 14:45

count_group_watchers_v2.patch Magnifier (1.01 KB) salman mp, 2022-05-01 15:16

0001-Respect-group-memberships-when-checking-if-an-object.patch Magnifier (3.35 KB) Holger Just, 2022-06-16 17:44


Related issues

Related to Redmine - Feature #4511: Allow adding user groups as watchers for issues Closed 2010-01-01

Associated revisions

Revision 21661
Added by Marius BALTEANU 6 days ago

Respect group memberships when checking if an object is watched (#37065).

Patch Holger Just.

Revision 21667
Added by Marius BALTEANU 5 days ago

Merged r21661 to 5.0-stable (#37065).

History

#2 Updated by Holger Just about 1 month ago

  • Related to Feature #4511: Allow adding user groups as watchers for issues added

#3 Updated by Holger Just 9 days ago

Attached is a patch against current trunk which fixes this behavior. The patch was extracted from Planio.

The patch is more exhaustive than the previous ones by Salman:

  • It avoid having to load all groups as AR records
  • It also patches the instance watched_by? method to match the behavior of the scope.
  • It adds a test :)

I have also updated the parameter name of the watched_by scope to make it clear that it accepts a principal (either a user or group), rather than a numeric ID. Previously, the method accepted both types. In all call-sites within Redmine however, only a user or group object is ever passed here, never a numeric ID.

With that being said, the way the scope is implemented, it will perform some SQL queries already when building the scope to get the groups of the passed user. This might be undesirable, e.g. if there are updates here which might affect the final query between building the scope and executing it. An alternative to the scope in the attached patch would thus be the following scope which builds a fully self-contained query where all data is only resolved during its execution:

scope :watched_by, lambda { |user_id|
  # Basic case: return objects watched by the queried principal
  scope = where("#{Watcher.table_name}.user_id = ?", user_id)

  # If the principal is not a group (i.e. either a User or the id of
  # either a User or Group)...
  unless user_id.is_a?(Group)
    # ... we make sure that the given id belongs to an actual User...
    if user_id.is_a?(Integer)
      is_user = where(User.where(id: user_id).arel.exists)
    else
      is_user = self
    end

    # ... and finally allow objects which are either watched by the
    # user (base case above) or by any group the user is a member of
    scope = scope.or(
      is_user.
      where("#{Watcher.table_name}.user_id IN (?)", Group.having_user(user_id).select(:id))
    )
  end

  # Finally, we have to join the watchers table at the end to simplify
  # the or condition above. The final SQL is the same in any case.
  scope.joins(:watchers)
}

This scope is functionally equivalent to the version in the patch (with the addition that we do support passed Integer arguments for the id of a user or group here). It will not make any queries on its own, thus avoiding additional roundtrips to the database. With that however, it will also not use any cached data (such as a previously cached principal.group_ids list).

#4 Updated by Marius BALTEANU 6 days ago

  • Category set to Email notifications
  • Assignee set to Marius BALTEANU
  • Target version changed from Candidate for next minor release to 5.0.2

I think it is safe to deliver this fix in 5.0.2.

#5 Updated by Marius BALTEANU 6 days ago

  • Status changed from New to Resolved

Patch committed, thanks!

#6 Updated by Marius BALTEANU 5 days ago

  • Status changed from Resolved to Closed

#7 Updated by salman mp 2 days ago

Holger Just wrote:

The patch is more exhaustive than the previous ones by Salman:

Good job. Thanks.

Also available in: Atom PDF