Project

General

Profile

Actions

Patch #37065

closed

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

Added by salman mp over 2 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Category:
Email notifications
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:

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


Files


Related issues

Related to Redmine - Feature #4511: Allow adding user groups as watchers for issuesClosedGo MAEDA2010-01-01

Actions
Actions #2

Updated by Holger Just over 2 years ago

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

Updated by Holger Just over 2 years 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).

Actions #4

Updated by Marius BĂLTEANU over 2 years ago

  • Category set to Email notifications
  • Assignee set to Marius BĂLTEANU
  • 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.

Actions #5

Updated by Marius BĂLTEANU over 2 years ago

  • Status changed from New to Resolved

Patch committed, thanks!

Actions #6

Updated by Marius BĂLTEANU over 2 years ago

  • Status changed from Resolved to Closed
Actions #7

Updated by salman mp over 2 years ago

Holger Just wrote:

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

Good job. Thanks.

Actions

Also available in: Atom PDF