Bugfixing (#5383)

* Add missing JS file to the manifest * Don't crash when the SMS service raises an error * Don't crash when the nickname is blank * Use blank? instead of empty? with nil * Don't crash when there's no proposal * Fix scope selection * Don't crash when there's no highlighted scope in a consultation * Use MeetingPresenter when the author is a meeting * Fix freetext input when disabled. Closes #5075 * Fix user manager permissions. Closes #5030 * Add CHANGELOG
decidim · Oct 2, 2019 · 390f8da · 390f8da
1 parent 0777381
commit 390f8da
Show file tree

Hide file tree

Showing 17 changed files with 119 additions and 68 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -70,7 +70,8 @@
 - **decidim-core**: Fix rendering when custom colors exist [#5347](https://github.com/decidim/decidim/pull/5347)
 - **decidim-core**: Fix component generator [#5348](https://github.com/decidim/decidim/pull/5348)
 - **decidim-core**: Fix email notifications [#5370](https://github.com/decidim/decidim/pull/5370)
-- **decidim-assemblies**, **decidim-core**, **decidim-generators**, **decidim-initiatives**, **decidim-meetings**, **decidim-system** [\#5329](https://github.com/decidim/decidim/pull/5329)
+- **decidim-assemblies**, **decidim-core**, **decidim-generators**, **decidim-initiatives**, **decidim-meetings**, **decidim-system** Various bugfixes [\#5376](https://github.com/decidim/decidim/pull/5376)
+- **decidim-conferences**, **decidim-consultations**, **decidim-core**, **decidim-forms**, **decidim-meetings**, **decidim-proposals**, **decidim-verifications**** Various bugfixes [\#5383](https://github.com/decidim/decidim/pull/5383)
 
 **Removed**:
 

diff --git a/decidim-conferences/app/controllers/decidim/conferences/conference_program_controller.rb b/decidim-conferences/app/controllers/decidim/conferences/conference_program_controller.rb
@@ -11,7 +11,7 @@ class ConferenceProgramController < Decidim::Conferences::ApplicationController
       helper_method :collection, :conference, :meeting_days, :meeting_component
 
       def show
-        raise ActionController::RoutingError, "No meetings for this conference " if meetings.empty?
+        raise ActionController::RoutingError, "No meetings for this conference " if meetings.blank?
 
         enforce_permission_to :list, :program
         redirect_to decidim_conferences.conference_path(current_participatory_space) unless current_user_can_visit_space?

diff --git a/...nsultations/app/views/decidim/consultations/consultations/_highlighted_questions.html.erb b/...nsultations/app/views/decidim/consultations/consultations/_highlighted_questions.html.erb
@@ -1,10 +1,12 @@
 <% if consultation.highlighted_questions.any? %>
   <div class="row section" id="highlighted-questions">
-    <h2 class=section-heading>
-      <%= t "consultations.highlighted_questions.title",
-            scope: "decidim",
-            scope_name: translated_attribute(consultation.highlighted_scope.name) %>
-    </h2>
+    <% if consultation.highlighted_scope.present? %>
+      <h2 class=section-heading>
+        <%= t "consultations.highlighted_questions.title",
+              scope: "decidim",
+              scope_name: translated_attribute(consultation.highlighted_scope.name) %>
+      </h2>
+    <% end %>
 
     <% consultation.highlighted_questions.each do |question| %>
       <%= render partial: "decidim/consultations/consultations/question", locals: { question: question } %>

diff --git a/decidim-core/app/assets/config/decidim_core_manifest.js b/decidim-core/app/assets/config/decidim_core_manifest.js
@@ -14,3 +14,4 @@
 // = link decidim/floating_help.js
 // = link decidim/extras/_social_share.css
 // = link decidim/social_share.js
+// = link decidim/represent_user_group.js
diff --git a/decidim-core/app/cells/decidim/coauthorships_cell.rb b/decidim-core/app/cells/decidim/coauthorships_cell.rb
@@ -55,8 +55,10 @@ def presenter_for_author(authorable)
         "#{model.class.parent}::OfficialAuthorPresenter".constantize.new
       elsif authorable.user_group
         Decidim::UserGroupPresenter.new(authorable.user_group)
-      else
+      elsif authorable.author.is_a?(Decidim::User)
         Decidim::UserPresenter.new(authorable.author)
+      elsif authorable.author.is_a?(Decidim::Meeting)
+        Decidim::MeetingPresenter.new(authorable.author)
       end
     end
 

diff --git a/decidim-core/app/controllers/decidim/profiles_controller.rb b/decidim-core/app/controllers/decidim/profiles_controller.rb
@@ -65,10 +65,12 @@ def ensure_profile_holder_is_a_user
     end
 
     def ensure_profile_holder
-      raise ActionController::RoutingError, "No user or user group with the given nickname" unless profile_holder
+      raise ActionController::RoutingError, "No user or user group with the given nickname" if !profile_holder || profile_holder.nickname.blank?
     end
 
     def profile_holder
+      return if params[:nickname].blank?
+
       @profile_holder ||= Decidim::User.find_by(
         nickname: params[:nickname],
         organization: current_organization

diff --git a/decidim-core/app/controllers/decidim/scopes_controller.rb b/decidim-core/app/controllers/decidim/scopes_controller.rb
@@ -8,13 +8,11 @@ class ScopesController < Decidim::ApplicationController
     def picker
       enforce_permission_to :pick, :scope
 
-      title = params[:title] || t("decidim.scopes.picker.title", field: params[:field]&.downcase)
-      root = current_organization.scopes.find(params[:root]) if params[:root]
       context = root ? { root: root.id, title: title } : { title: title }
       required = params[:required] && params[:required] != "false"
+      current = (root&.descendants || current_organization.scopes).find_by(id: params[:current]) if params[:current].present?
 
-      if params[:current]
-        current = (root&.descendants || current_organization.scopes).find_by(id: params[:current]) || root
+      if current
         scopes = current.children
         parent_scopes = current.part_of_scopes(root)
       else
@@ -26,5 +24,17 @@ def picker
       render :picker, layout: nil, locals: { required: required, title: title, root: root, current: current, scopes: scopes.order(name: :asc),
                                              parent_scopes: parent_scopes, global_value: params[:global_value], context: context }
     end
+
+    private
+
+    def title
+      @title ||= params[:title] || t("decidim.scopes.picker.title", field: params[:field]&.downcase)
+    end
+
+    def root
+      return if params[:root].blank?
+
+      @root ||= current_organization.scopes.find(params[:root])
+    end
   end
 end
diff --git a/decidim-core/app/permissions/decidim/permissions.rb b/decidim-core/app/permissions/decidim/permissions.rb
@@ -11,8 +11,8 @@ def permissions
       search_scope_action?
 
       return permission_action unless user
-      return user_manager_permissions if not_admin? && user_manager?
 
+      user_manager_permissions
       manage_self_user_action?
       authorization_action?
       follow_action?
@@ -171,14 +171,5 @@ def not_already_active?(authorization)
     def user_manager_permissions
       Decidim::UserManagerPermissions.new(user, permission_action, context).permissions
     end
-
-    def not_admin?
-      !user.admin?
-    end
-
-    # Whether the user has the user_manager role or not.
-    def user_manager?
-      user.role? "user_manager"
-    end
   end
 end
diff --git a/decidim-core/app/permissions/decidim/user_manager_permissions.rb b/decidim-core/app/permissions/decidim/user_manager_permissions.rb
@@ -3,8 +3,10 @@
 module Decidim
   class UserManagerPermissions < DefaultPermissions
     def permissions
-      allow! if read_admin_dashboard_action?
-      allow! if impersonate_managed_user_action?
+      if user_manager?
+        allow! if read_admin_dashboard_action?
+        allow! if impersonate_managed_user_action?
+      end
 
       permission_action
     end
@@ -20,5 +22,10 @@ def impersonate_managed_user_action?
       permission_action.subject == :managed_user &&
         permission_action.action == :impersonate
     end
+
+    # Whether the user has the user_manager role or not.
+    def user_manager?
+      user && !user.admin? && user.role?("user_manager")
+    end
   end
 end
diff --git a/decidim-core/spec/permissions/decidim/permissions_spec.rb b/decidim-core/spec/permissions/decidim/permissions_spec.rb
@@ -108,7 +108,21 @@
     context "when user is a user manager" do
       let(:user) { create :user, :user_manager }
 
-      it_behaves_like "delegates permissions to", Decidim::UserManagerPermissions
+      context "when reading the admin dashboard" do
+        let(:action) do
+          { scope: :public, action: :read, subject: :admin_dashboard }
+        end
+
+        it { is_expected.to eq true }
+      end
+
+      context "when impersonating a managed user" do
+        let(:action) do
+          { scope: :public, action: :impersonate, subject: :managed_user }
+        end
+
+        it { is_expected.to eq true }
+      end
     end
 
     context "when managing self user" do

diff --git a/decidim-forms/app/assets/javascripts/decidim/forms/option_attached_inputs.component.js.es6 b/decidim-forms/app/assets/javascripts/decidim/forms/option_attached_inputs.component.js.es6
@@ -14,7 +14,7 @@
         const $field = $(el);
         const enabled = $field.is(":checked");
 
-        $field.parents("label").find(this.dependentInputSelector).prop("disabled", !enabled);
+        $field.parents("div.collection-input").find(this.dependentInputSelector).prop("disabled", !enabled);
       });
     }
 

diff --git a/decidim-forms/app/views/decidim/forms/questionnaires/_answer.html.erb b/decidim-forms/app/views/decidim/forms/questionnaires/_answer.html.erb
@@ -19,70 +19,76 @@
     <% answer.question.answer_options.each_with_index do |answer_option, idx| %>
       <% choice_id = "#{field_id}_choices_#{idx}" %>
 
-      <%= label_tag "#{choice_id}_body" do %>
-        <%= radio_button_tag "questionnaire[answers][#{answer_idx}][choices][][body]",
-                             translated_attribute(answer_option.body),
-                             answer_option.id == choice.try(:answer_option_id),
-                             id: "#{choice_id}_body", disabled: disabled %>
-
-        <%= translated_attribute(answer_option.body) %>
+      <div class="collection-input">
+        <%= label_tag "#{choice_id}_body" do %>
+          <%= radio_button_tag "questionnaire[answers][#{answer_idx}][choices][][body]",
+                              translated_attribute(answer_option.body),
+                              answer_option.id == choice.try(:answer_option_id),
+                              id: "#{choice_id}_body", disabled: disabled %>
+
+          <%= translated_attribute(answer_option.body) %>
+
+          <%= hidden_field_tag "questionnaire[answers][#{answer_idx}][choices][][answer_option_id]",
+                              answer_option.id,
+                              id: "#{choice_id}_answer_option",
+                              disabled: true %>
+        <% end %>
 
         <% if answer_option.free_text %>
           <%= text_field_tag "questionnaire[answers][#{answer_idx}][choices][][custom_body]",
-                             choice.try(:custom_body),
-                             id: "#{choice_id}_custom_body",
-                             disabled: true %>
+          choice.try(:custom_body),
+          id: "#{choice_id}_custom_body",
+          disabled: true %>
         <% end %>
-
-        <%= hidden_field_tag "questionnaire[answers][#{answer_idx}][choices][][answer_option_id]",
-                             answer_option.id,
-                             id: "#{choice_id}_answer_option",
-                             disabled: true %>
-      <% end %>
+      </div>
     <% end %>
   </div>
 <% when "multiple_option" %>
   <div class="check-box-collection">
     <% answer.question.answer_options.each_with_index do |answer_option, idx| %>
       <% choice = answer.selected_choices.find { |choice| choice.answer_option_id == answer_option.id } %>
 
-      <%= label_tag do %>
-        <%= check_box_tag "questionnaire[answers][#{answer_idx}][choices][#{idx}][body]",
-                          translated_attribute(answer_option.body),
-                          choice.present?, disabled: disabled %>
+      <div class="collection-input">
+        <%= label_tag do %>
+          <%= check_box_tag "questionnaire[answers][#{answer_idx}][choices][#{idx}][body]",
+                            translated_attribute(answer_option.body),
+                            choice.present?, disabled: disabled %>
 
-        <%= translated_attribute(answer_option.body) %>
+          <%= translated_attribute(answer_option.body) %>
+
+          <%= hidden_field_tag "questionnaire[answers][#{answer_idx}][choices][#{idx}][answer_option_id]", answer_option.id %>
+        <% end %>
 
         <% if answer_option.free_text %>
           <%= text_field_tag "questionnaire[answers][#{answer_idx}][choices][#{idx}][custom_body]",
-                             choice.try(:custom_body),
-                             disabled: true %>
+          choice.try(:custom_body),
+          disabled: true %>
         <% end %>
-
-        <%= hidden_field_tag "questionnaire[answers][#{answer_idx}][choices][#{idx}][answer_option_id]", answer_option.id %>
-      <% end %>
+      </div>
     <% end %>
   </div>
 <% when "sorting" %>
   <div class="sortable-check-box-collection">
     <% answer.question.answer_options.each_with_index do |answer_option, idx| %>
       <% choice = answer.selected_choices.find { |choice| choice.answer_option_id == answer_option.id } %>
 
-      <%= label_tag do %>
-        <%= check_box_tag "questionnaire[answers][#{answer_idx}][choices][#{idx}][body]",
-                          translated_attribute(answer_option.body),
-                          choice.present?, disabled: disabled %>
+      <div class="collection-input">
+        <%= label_tag do %>
+          <%= check_box_tag "questionnaire[answers][#{answer_idx}][choices][#{idx}][body]",
+                            translated_attribute(answer_option.body),
+                            choice.present?, disabled: disabled %>
 
-        <span class="position"><%= choice.try(:position) %></span>
+          <span class="position"><%= choice.try(:position) %></span>
 
-        <%= translated_attribute(answer_option.body) %>
+          <%= translated_attribute(answer_option.body) %>
 
-        <%= hidden_field_tag "questionnaire[answers][#{answer_idx}][choices][#{idx}][position]",
-                             choice.try(:position),
-                             disabled: true %>
+          <%= hidden_field_tag "questionnaire[answers][#{answer_idx}][choices][#{idx}][position]",
+                              choice.try(:position),
+                              disabled: true %>
 
-        <%= hidden_field_tag "questionnaire[answers][#{answer_idx}][choices][#{idx}][answer_option_id]", answer_option.id %>
-      <% end %>
+          <%= hidden_field_tag "questionnaire[answers][#{answer_idx}][choices][#{idx}][answer_option_id]", answer_option.id %>
+        <% end %>
+      </div>
     <% end %>
   </div>
 <% end %>

diff --git a/decidim-meetings/app/controllers/decidim/meetings/meetings_controller.rb b/decidim-meetings/app/controllers/decidim/meetings/meetings_controller.rb
@@ -11,10 +11,11 @@ class MeetingsController < Decidim::Meetings::ApplicationController
       helper_method :meetings, :meeting, :registration, :search
 
       def index
-        return unless search.results.empty? && params.dig("filter", "date") != "past"
+        return unless search.results.blank? && params.dig("filter", "date") != "past"
 
         @past_meetings = search_klass.new(search_params.merge(date: "past"))
-        unless @past_meetings.results.empty?
+
+        if @past_meetings.results.present?
           params[:filter] ||= {}
           params[:filter][:date] = "past"
           @forced_past_meetings = true

diff --git a/decidim-proposals/app/controllers/decidim/proposals/proposals_controller.rb b/decidim-proposals/app/controllers/decidim/proposals/proposals_controller.rb
@@ -54,7 +54,7 @@ def index
       end
 
       def show
-        raise ActionController::RoutingError, "Not Found" unless can_show_proposal?
+        raise ActionController::RoutingError, "Not Found" if @proposal.blank? || !can_show_proposal?
 
         @report_form = form(Decidim::ReportForm).from_params(reason: "spam")
       end

diff --git a/decidim-proposals/app/presenters/decidim/proposals/proposal_presenter.rb b/decidim-proposals/app/presenters/decidim/proposals/proposal_presenter.rb
@@ -18,8 +18,10 @@ def author
                       coauthorship = coauthorships.first
                       if coauthorship.user_group
                         Decidim::UserGroupPresenter.new(coauthorship.user_group)
-                      else
+                      elsif coauthorship.author.is_a?(Decidim::User)
                         Decidim::UserPresenter.new(coauthorship.author)
+                      elsif coauthorship.author.is_a?(Decidim::Meeting)
+                        Decidim::MeetingPresenter.new(coauthorship.author)
                       end
                     end
       end

diff --git a/decidim-verifications/app/commands/decidim/verifications/confirm_user_authorization.rb b/decidim-verifications/app/commands/decidim/verifications/confirm_user_authorization.rb
@@ -31,6 +31,8 @@ def call
         else
           broadcast(:invalid)
         end
+      rescue StandardError => e
+        broadcast(:invalid, e.message)
       end
 
       protected

diff --git a/...dim-verifications/spec/commmands/decidim/verifications/confirm_user_authorization_spec.rb b/...dim-verifications/spec/commmands/decidim/verifications/confirm_user_authorization_spec.rb
@@ -68,5 +68,15 @@ def verification_metadata
     it "confirms the authorization for the user" do
       expect { subject.call }.to change(authorizations, :count).by(1)
     end
+
+    context "when there's a problem with the SMS service" do
+      before do
+        expect(authorization).to receive(:grant!).and_raise(StandardError, "Somewthing went wrong")
+      end
+
+      it "is not valid" do
+        expect { subject.call }.to broadcast(:invalid)
+      end
+    end
   end
 end