Merge pull request #2142 from mschnitzer/issue_1970_2

[webui] Submit Requests: Add hint for project maintainers, when target package has maintainers

ReleaseNotes-2.8

@@ -32,6 +32,8 @@ Features
  * Allow admins to lock or delete users and their home projects via new command
  * Users can be declared sub accounts of other users. Useful for automated scripts.
  * Allow triggering services from the UI.
+ * Show a hint to project maintainers, when he/she is not a package maintainer of
+   the target package of a request
 
 Incompatible changes:
 =====================

src/api/app/assets/stylesheets/webui/application/style.css.scss

@@ -394,3 +394,8 @@ div.dataTables_wrapper:after {
 #schedulerstatus, #building_all, #idlehosts {
   padding-top: 1em;
 }
+
+// Some general stuff for avoiding inline css
+.padding-10pt {
+  padding: 10pt;
+}

src/api/app/controllers/webui/request_controller.rb

@@ -110,6 +110,17 @@ def show
     @history = History.find_by_request(@bsreq, {withreviews: 1})
     @actions = @req['actions']
 
+    # retrieve a list of all package maintainers that are assigned to at least one target package
+    @package_maintainers = get_target_package_maintainers(@actions) || []
+
+    # search for a project, where the user is not a package maintainer but a project maintainer and show
+    # a hint if that package has some package maintainers (issue#1970)
+    projects = @actions.map{|action| action[:tprj]}.uniq
+    maintainer_role = Role.find_by_title("maintainer")
+
+    @show_project_maintainer_hint = (!@package_maintainers.empty? && !@package_maintainers.include?(User.current) &&
+      projects.any? { |project| Project.find_by_name(project).user_has_role?(User.current, maintainer_role) })
+
     @request_before = nil
     @request_after = nil
     index = session[:request_numbers].try(:index, @number)
@@ -122,6 +133,11 @@ def show
     @comments = @bsreq.comments
   end
 
+  def package_maintainers_dialog
+    @maintainers = get_target_package_maintainers(params[:actions])
+    render_dialog unless @maintainers.empty?
+  end
+
   def sourcediff
     check_ajax
     render :partial => 'shared/editor', :locals => {:text => params[:text],
@@ -432,6 +448,11 @@ def main_object
 
   private
 
+  def get_target_package_maintainers(actions)
+    actions.uniq!{ |action| action[:tpkg] }
+    actions.flat_map { |action| Package.find_by_project_and_name(action[:tprj], action[:tpkg]).try(:maintainers) }.compact.uniq
+  end
+
   def change_state(newstate, params)
     req = BsRequest.find_by_number(params[:number])
     if req.nil?

src/api/app/mixins/has_relationships.rb

@@ -76,6 +76,13 @@ def add_role(what, role)
     end
   end
 
+  def maintainers
+    direct_users = relationships.with_users_and_roles_query.maintainers.pluck('users.login as login').map{|user| User.find_by_login!(user)}
+    users_in_groups = relationships.with_groups_and_roles_query.maintainers.pluck('groups.title as title'). \
+      map{|title| Group.find_by_title!(title).users}.flatten
+    (direct_users + users_in_groups).uniq
+  end
+
   def remove_all_persons
     check_write_access!
     self.relationships.users.delete_all

src/api/app/models/relationship.rb

@@ -43,13 +43,14 @@ class SaveError < APIException; end
   scope :packages, -> { where("package_id is not null") }
   scope :groups, -> { where("group_id is not null") }
   scope :users, -> { where("user_id is not null") }
-  scope :with_users_and_roles, lambda {
-    joins(:role, :user).order('role_name, login').
-      pluck('users.login as login, roles.title AS role_name')
+  scope :with_users_and_roles_query, lambda {
+    joins(:role, :user).order('roles.title, users.login')
   }
-  scope :with_groups_and_roles, lambda {
-    joins(:role, :group).order('role_name, title').
-      pluck('groups.title as title', 'roles.title as role_name')
+  scope :with_groups_and_roles_query, lambda {
+    joins(:role, :group).order('roles.title, groups.title')
+  }
+  scope :maintainers, lambda {
+    where('roles.title' => 'maintainer')
   }
 
   # we only care for project<->user relationships, but the cache is not *that* expensive
@@ -156,6 +157,14 @@ def self.discard_cache
     Rails.cache.delete('forbidden_projects')
   end
 
+  def self.with_users_and_roles
+    with_users_and_roles_query.pluck('users.login as login, roles.title AS role_name')
+  end
+
+  def self.with_groups_and_roles
+    with_groups_and_roles_query.pluck('groups.title as title', 'roles.title as role_name')
+  end
+
   private
 
   def discard_cache

src/api/app/views/webui/request/_package_maintainers_dialog.html.erb

@@ -0,0 +1,23 @@
+  <div class="dialog" id="disable_mask"></div>
+  <div class="dialog darkgrey_box" id="branch_dialog">
+    <div class="box box-shadow">
+      <h2 class="box-header"><%= pluralize(@maintainers.size, "Package Maintainer") %></h2>
+      <div class="dialog-content">
+        <table>
+          <tbody>
+          <% @maintainers.each do |m| %>
+          <tr>
+            <td>
+              <%= user_with_realname_and_icon(m, short: true) %>
+            </td>
+          </tr>
+          <% end %>
+          </tbody>
+         </table>
+
+        <div class="dialog-buttons">
+         <%= remove_dialog_tag('Close') %>
+        </div>
+      </div>
+    </div>
+  </div>

src/api/app/views/webui/request/show.html.erb

@@ -37,6 +37,10 @@
           by <%= user_with_realname_and_icon(@req['creator'], no_icon: true, short: true) %> <%= fuzzy_time(@req['created_at']) %></li>
         <li><%= sprite_tag('information') %> In
           state <%= link_to @state, { :anchor => 'request_history' }, { :style => "color: #{request_state_color(@state)};" } %></li>
+        <% unless @package_maintainers.empty? %>
+        <li><%= sprite_tag('eye') %> <%= pluralize(@package_maintainers.size, "package maintainer") %>
+          (<%= link_to('show who', { :action => :package_maintainers_dialog, :actions => @actions }, :remote => true) %>)</li>
+        <% end %>
         <% if @superseding %>
             <li><%= sprite_tag('information', title: '') %> Supersedes
               <% @superseding.each do |supersed| %>
@@ -229,6 +233,15 @@
       <div class="review_descision_display" style="<%= 'display: none;' if @can_add_reviews && @my_open_reviews.length > 0 %>" id="review_descision_display_-1">
         <% if @can_handle_request %>
             <%= form_tag({ :action => 'changerequest' }, { id: 'request_handle_form' }) do %>
+                <% if @show_project_maintainer_hint %>
+                    <div class="ui-state-highlight ui-corner-all ui-widget-shadow padding-10pt">
+                      <span class="ui-icon ui-icon-info">
+                        You are a <strong>project maintainer</strong> but not a <strong>package maintainer</strong>. This package
+                        has <strong><%= pluralize(@package_maintainers.size, "package maintainer") %></strong> assigned. Please keep
+                        in mind that also package maintainers would like to review this request.
+                      </span>
+                    </div>
+                <% end %>
                 <p><%= text_area_tag(:reason, nil, :placeholder => 'Please add a comment', :size => '80x2', :style => 'width: 99%') %></p>
 
                 <p>
@@ -237,6 +250,7 @@
                       <% if @state == 'review' %>
                           <%= submit_tag 'Accept request', name: 'accepted', id: 'accept_request_button', data: { confirm: 'Do you really want to approve this request, despite of open review requests?' } %>
                       <% else %>
+
                           <%= submit_tag 'Accept request', name: 'accepted', id: 'accept_request_button' %>
                       <% end %>
                       <%= submit_tag 'Decline request', name: 'declined' if !@is_author %>

src/api/config/routes.rb

@@ -270,6 +270,7 @@ def self.matches?(request)
       get 'request/set_incident_dialog' => :set_incident_dialog
       post 'request/set_incident' => :set_incident
       post 'request/comments/:number' => :save_comment
+      get 'request/package_maintainers_dialog' => :package_maintainers_dialog
     end
 
     controller 'webui/search' do