User roles review #1605

beagleknight · 2017-07-13T14:02:43Z

🎩 What? Why?

We started with the column roles for the users table and then we added the ParticipatoryProcessUserRole model to handle roles for a specific participatory process.

I want to merge both so there will be admin users and users with certain roles for each participatory process.

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

Roles management:

Admin panel as a participatory process admin

Admin panel as a participatory process collaborator

Admin panel as a participatory process moderator (inside moderations page)

👻 GIF

codecov · 2017-07-13T14:02:45Z

Codecov Report

Merging #1605 into master will decrease coverage by 32.44%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1605       +/-   ##
===========================================
- Coverage   96.98%   64.54%   -32.45%     
===========================================
  Files         499       56      -443     
  Lines        8534      990     -7544     
===========================================
- Hits         8277      639     -7638     
- Misses        257      351       +94

codecov · 2017-07-13T14:02:45Z

Codecov Report

Merging #1605 into master will decrease coverage by 32.44%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1605       +/-   ##
===========================================
- Coverage   96.98%   64.54%   -32.45%     
===========================================
  Files         499       56      -443     
  Lines        8534      990     -7544     
===========================================
- Hits         8277      639     -7638     
- Misses        257      351       +94

codecov · 2017-07-13T14:02:45Z

Codecov Report

Merging #1605 into master will decrease coverage by 0.05%.
The diff coverage is 94.19%.

@@            Coverage Diff             @@
##           master    #1605      +/-   ##
==========================================
- Coverage   96.98%   96.93%   -0.06%     
==========================================
  Files         499      507       +8     
  Lines        8534     8549      +15     
==========================================
+ Hits         8277     8287      +10     
- Misses        257      262       +5

deivid-rodriguez · 2017-07-17T09:51:42Z

@beagleknight Thanks for paying off this technical debt! 😃

josepjaume

👏 👏 👏 👏 👏 👏 👏 👏 👏 👏

... but check my comments xDD

josepjaume · 2017-07-17T13:38:54Z

decidim-admin/app/models/decidim/admin/abilities/admin_user.rb

@@ -23,11 +21,10 @@ def initialize(user, _context)
          end

          can [:read, :update], Decidim::Organization do |organization|
-            organization == user.organization
+            organization == @user.organization


@user could be an instance method instead

josepjaume · 2017-07-17T13:41:17Z

decidim-admin/app/models/decidim/admin/abilities/admin_user.rb

@@ -36,7 +33,7 @@ def initialize(user, _context)
          can [:create, :index, :new, :read, :invite], User

          can [:destroy], [User] do |user_to_destroy|
-            user != user_to_destroy
+            @user != user_to_destroy


josepjaume · 2017-07-17T13:41:45Z

decidim-admin/app/models/decidim/admin/abilities/participatory_process_admin_user.rb

-          can :read, :admin_dashboard do
-            participatory_processes.any?
-          end
+      class ParticipatoryProcessAdminUser < Decidim::Abilities::ParticipatoryProcessAdminUser


Why the name change?

There's a controller level concern called ParticipatoryProcessAdmin, so renaming reduces confusion in that regard.

Then I'd go for appending Ability at the end. User is confusing as well!

Agreed, but then we need to rename most of the ability classes.

Well.. I personally dislike the User suffix but it looks like a convention we followed in the past and I just normalised it.
We could use Ability like: CurrentUserAbility, EveryoneAbility, ParticipatoryProcessAdminAbility and so on.
Is it fine for you? @josepjaume @deivid-rodriguez

Yeah, I think so. Sorry because it means a lot of renaming, but it definitely feels better.

@josepjaume Done 💥

josepjaume · 2017-07-18T06:31:11Z

decidim-admin/app/models/decidim/admin/abilities/participatory_process_admin_user.rb

          end

          can :manage, Attachment do |attachment|
-            participatory_processes.include?(attachment.attached_to)
+            can_manage_process?(attachment.attached_to)


This was here before, but maybe is too much of an asusmption? I believe attachments are polymorphic.

Yeah, it is polymorphic. I'll add attached_to.kind_of? ParticipatoryProcess then.

josepjaume · 2017-07-18T06:31:58Z

decidim-admin/app/models/decidim/admin/abilities/participatory_process_collaborator_user.rb

+    module Abilities
+      # Defines the abilities for a collaborator user in the admin
+      # section. Intended to be used with `cancancan`.
+      class ParticipatoryProcessCollaboratorUser < Decidim::Abilities::ParticipatoryProcessCollaboratorUser


Why appending User at the end of abilities? If we had to append anything, shouldn't it be Ability?

I believe appending User is the current convention in the code...

Read my comment above 😄

josepjaume · 2017-07-18T06:43:25Z

decidim-admin/app/views/layouts/decidim/admin/participatory_process.html.erb

-      <% if can? :update, participatory_process %>
-        <li <% if is_active_link?(decidim_admin.edit_participatory_process_path(participatory_process)) %> class="is-active" <% end %>>
-          <%= aria_selected_link_to t("info", scope: "decidim.admin.menu.participatory_processes_submenu"), decidim_admin.edit_participatory_process_path(participatory_process) %>
+      <% if can? :update, current_participatory_process %>


All this could probably be refactored into Decidim::Menu at some point, right?

Yeah, I think so!

josepjaume · 2017-07-18T06:44:47Z

decidim-admin/lib/decidim/admin/engine.rb

-          config.abilities += ["Decidim::Admin::Abilities::AdminUser"]
-          config.abilities += ["Decidim::Admin::Abilities::ParticipatoryProcessAdmin"]
-          config.abilities += ["Decidim::Admin::Abilities::CollaboratorUser"]
+          config.admin_abilities += ["Decidim::Admin::Abilities::AdminUser"]


Why not adding the whole array in a single statement?

config.admin_abilities += [ "Decidim::Admin::Abilities::AdminUser", # ... ]

It was like that before so I didn't thought about that 😂

josepjaume · 2017-07-18T07:00:17Z

decidim-core/lib/decidim/abilities/participatory_process_role_user.rb

+
+      # Grant access to admin panel by default.
+      def define_abilities
+        can :read, :admin_dashboard


Doesn't this mean every single user will end up seeing this? We probably need to check if the user has participatory_process_role.any?.

Not every single user because this is only applying for abilities who inherit this class. There is an extra check in the constructor as well:

# Define abilities if the user is not an admin and has at least one process to manage. if not_admin? && has_manageable_processes? define_abilities ... end

So the ability is not defined if the user has not a manageable process by his role.

Ok! I'm affraid this means we're hitting the DB each load just to check this, but it's fine. Let's see if it causes any performance degradation, hopefully not.

deivid-rodriguez · 2017-07-18T07:12:41Z

CHANGELOG.md

- French seed [\#1588](https://github.com/decidim/decidim/issues/1588)
- Add administration users page in users section \(admin panel\) [\#1582](https://github.com/decidim/decidim/issues/1582)
- Ugly margin under flash message [\#1550](https://github.com/decidim/decidim/issues/1550)
+_Added for new features._


This sentences shouldn't be there, right?

I thought they could be useful for the upcoming PRs as instructions. We can remove those lines when we do the next release 😄

deivid-rodriguez · 2017-07-18T07:14:26Z

decidim-admin/app/controllers/decidim/admin/participatory_process_user_roles_controller.rb

+        @collection ||= Decidim::ParticipatoryProcessUserRole
+                        .includes(:user)
+                        .where(participatory_process: current_participatory_process)
+                        .order(:role, "decidim_users.name")


Why removing the Rectify::Query? It felt nice to have it separate and unit tested.

I removed it as a part of a huge clean up 😅 . I like these abstractions but I prefer using them when it's needed and since I managed to remove all occurrences but one I think it's better to keep it the logic in the same place.

Well, this one was the only usage previously as well (apart from these duplicated collection and process_admin_roles methods). I still think it's nice to encapsulate the complexity of the query and have it tested. No big deal though :)

mrcasals · 2017-07-19T08:31:57Z

decidim-core/lib/decidim/core/engine.rb

@@ -79,11 +79,11 @@ class Engine < ::Rails::Engine

      initializer "decidim.inject_abilities_to_user" do |_app|
        Decidim.configure do |config|
-          config.abilities << "Decidim::Abilities::EveryoneAbililty"


Abililty LOL

* Remove user roles and add admin column * Removing dependencies between decidim-admin and other engines * Remove role? method completely * Fix rubocop issues * Fix engines DecidimAdmin dependencies * Fix rubocop issues * Order user roles by role and user name * Fix some permissions problems * [wip] fixing abilities * Add admin 'public' permissions * Fix participatory process admins abilities core/admin * Finish admin/core abilities * DRY some code * Fix constant autoload problems * Add participatory process moderator role * Fix rubocop issues * Add missing docs * Fix a few specs * Fix a few more specs * Fix some specs * Fix more specs * Fix proposals specs * Fix comments specs * Update CHANGELOG.md * Fix rubocop issue * Add feedback * Add 'Ability' suffix to all abilities classes * Remove unnecessary require * Fix renaming problems * Fix specs

beagleknight self-assigned this Jul 13, 2017

ghost added the in-progress label Jul 13, 2017

beagleknight changed the title ~~Remove user roles and add admin column~~ User roles review Jul 13, 2017

beagleknight added this to the 0.5.0 milestone Jul 14, 2017

beagleknight mentioned this pull request Jul 17, 2017

Stable releases and branching model #1387

Closed

beagleknight added 21 commits July 17, 2017 15:35

Remove user roles and add admin column

864081c

Removing dependencies between decidim-admin and other engines

59f2d6e

Remove role? method completely

fe09aa8

Fix rubocop issues

e849c0f

Fix engines DecidimAdmin dependencies

772bb3a

Fix rubocop issues

6710130

Order user roles by role and user name

54f5950

Fix some permissions problems

e963041

[wip] fixing abilities

3a26956

Add admin 'public' permissions

e9ba942

Fix participatory process admins abilities core/admin

f876bb0

Finish admin/core abilities

238151e

DRY some code

a1de290

Fix constant autoload problems

36386d9

Add participatory process moderator role

da3565e

Fix rubocop issues

8cd97ff

Add missing docs

239bf5b

Fix a few specs

6dd43c4

Fix a few more specs

0ac06aa

Fix some specs

e4b41a3

Fix more specs

aea9ef2

Fix proposals specs

7c4e29f

beagleknight force-pushed the enhance/user-roles branch from d2869f8 to 7c4e29f Compare July 17, 2017 13:35

beagleknight added 3 commits July 17, 2017 15:53

Fix comments specs

3647154

Update CHANGELOG.md

346f1f4

Fix rubocop issue

b69ceab

beagleknight force-pushed the enhance/user-roles branch from 2320b5f to b69ceab Compare July 17, 2017 14:15

beagleknight added in-review and removed in-progress labels Jul 17, 2017

josepjaume suggested changes Jul 18, 2017

View reviewed changes

mrcasals previously approved these changes Jul 18, 2017

View reviewed changes

deivid-rodriguez reviewed Jul 18, 2017

View reviewed changes

Add feedback

31151d1

beagleknight dismissed mrcasals’s stale review via 31151d1 July 18, 2017 12:48

beagleknight added 3 commits July 19, 2017 09:35

Add 'Ability' suffix to all abilities classes

419e792

Remove unnecessary require

5715c9e

Fix renaming problems

0d0dcb4

mrcasals reviewed Jul 19, 2017

View reviewed changes

Fix specs

c26090c

josepjaume approved these changes Jul 19, 2017

View reviewed changes

josepjaume merged commit 55ec1b8 into master Jul 19, 2017

ghost removed the in-review label Jul 19, 2017

josepjaume deleted the enhance/user-roles branch July 19, 2017 10:26

beagleknight mentioned this pull request Jul 21, 2017

Fix user roles broken migration #1626

Merged

This was referenced Jul 28, 2017

Add roles to users #1648

Merged

Fix sidekiq admin route constraint HospitaletDeLlobregat/decidim-hospitalet#164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User roles review #1605

User roles review #1605

beagleknight commented Jul 13, 2017 •

edited

codecov bot commented Jul 13, 2017

codecov bot commented Jul 13, 2017

codecov bot commented Jul 13, 2017 •

edited

deivid-rodriguez commented Jul 17, 2017

josepjaume left a comment •

edited

josepjaume Jul 17, 2017

josepjaume Jul 17, 2017

josepjaume Jul 17, 2017

deivid-rodriguez Jul 18, 2017

josepjaume Jul 18, 2017

deivid-rodriguez Jul 18, 2017

beagleknight Jul 18, 2017

josepjaume Jul 18, 2017

beagleknight Jul 19, 2017

josepjaume Jul 18, 2017

beagleknight Jul 18, 2017

josepjaume Jul 18, 2017

deivid-rodriguez Jul 18, 2017

beagleknight Jul 18, 2017

josepjaume Jul 18, 2017

beagleknight Jul 18, 2017

josepjaume Jul 18, 2017

beagleknight Jul 18, 2017

josepjaume Jul 18, 2017

beagleknight Jul 18, 2017 •

edited

josepjaume Jul 18, 2017

deivid-rodriguez Jul 18, 2017

beagleknight Jul 18, 2017

deivid-rodriguez Jul 18, 2017

beagleknight Jul 18, 2017

deivid-rodriguez Jul 18, 2017

mrcasals Jul 19, 2017

User roles review #1605

User roles review #1605

Conversation

beagleknight commented Jul 13, 2017 • edited

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

👻 GIF

codecov bot commented Jul 13, 2017

Codecov Report

codecov bot commented Jul 13, 2017

Codecov Report

codecov bot commented Jul 13, 2017 • edited

Codecov Report

deivid-rodriguez commented Jul 17, 2017

josepjaume left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beagleknight Jul 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beagleknight commented Jul 13, 2017 •

edited

codecov bot commented Jul 13, 2017 •

edited

josepjaume left a comment •

edited

beagleknight Jul 18, 2017 •

edited