Private processes or assemblies by isaacmg410 · Pull Request #2618 · decidim/decidim

isaacmg410 · 2018-02-01T08:03:25Z

🎩 What? Why?

This PR adds the functionality of making a participatory space private and make it visible only for a series of users selected. Users are invited by sending an email.

📌 Related Issues

Fixes Private processes or assemblies #2282

📋 Subtasks

Add CHANGELOG entry
Add private space to assembly and proccess
Invite users by email to assembly and process
Show private spaces only if current_user is assigned to these space
Show stats only when the spaces are public
Rspec Test

isaacmg410 · 2018-02-01T14:09:15Z

decidim-accountability/lib/decidim/accountability/feature.rb

@@ -27,7 +27,16 @@
  end

  feature.register_stat :results_count, primary: true, priority: Decidim::StatsRegistry::HIGH_PRIORITY do |features, _start_at, _end_at|


Is there any way to filter these features, depending on the participatory_space is private or public? I'm using the query below because I have not found any other way, but I think it's not the most optimal way .. @decidim/lot-core, any idea?

@isaacmg410 The HomeStats Presenter only returns features from spaces matching the public_spaces scope:

decidim/decidim-core/app/presenters/decidim/home_stats_presenter.rb

Lines 77 to 81 in 1d57472

def public_participatory_spaces

@public_participatory_spaces ||= Decidim.participatory_space_manifests.flat_map do |manifest|

manifest.participatory_spaces.call(organization).public_spaces

end

end

You need to modify this scope in each space to return only those that are public. I would not change this result depending on the user, so I would only return those spaces that are public for everyone.

carolromero · 2018-02-05T15:31:53Z

Hi! For not having staled PRs on @decidim/product we're gonna change the policy for merging this kind of issues: let's merge it and not have a bottleneck with design (cc/ @decidim/lot-px). When the design for this kind of issues is ready, then we'll open a new PR with only design changes.

We're afraid that if we let the time pass only for design it'd be much more difficult to merge given our current pace.

codecov · 2018-02-06T13:42:46Z

Codecov Report

Merging #2618 into master will decrease coverage by 0.02%.
The diff coverage is 96.6%.

@@            Coverage Diff             @@
##           master    #2618      +/-   ##
==========================================
- Coverage   98.69%   98.66%   -0.03%     
==========================================
  Files        1674     1692      +18     
  Lines       39871    40329     +458     
==========================================
+ Hits        39349    39791     +442     
- Misses        522      538      +16

isaacmg410 · 2018-02-07T07:53:52Z

@decidim/lot-core do you know why codecov say this?

No coverage uploaded for pull request base (master@8387db0). Click here to learn what that means. The diff coverage is 81.39%."

mrcasals · 2018-02-07T07:56:25Z

@isaacmg410 it's because tests are failing, and coverage is calculated after the tests are run. Please fix the tests (and the conflicts), and once the suite is green Codecov will update its comment accordingly.

isaacmg410 · 2018-02-07T07:59:47Z

@mrcasals also codeclimate is saying that there are similar blocks.
I have followed the same strategy as creating admin roles, but in this case codeclimate not say anything about similar blocks, how can I do that codeclimate not take into account?

mrcasals · 2018-02-07T08:13:39Z

@isaacmg410 if you can't find a good way to abstract this, then I say we can ignore these warnings. There's one of them that can be fixed automatically though 😄

isaacmg410 · 2018-02-08T16:21:07Z

@decidim/lot-core ready to review. About the codeclimate issues to fix, I can't find a good way to abstract this.

mrcasals · 2018-02-09T09:05:21Z

decidim-assemblies/app/controllers/decidim/assemblies/assemblies_controller.rb


-      def show; end
+      def show
+        redirect_to "/404" unless (current_participatory_space.private_space? &&


can you move this to something like current_user_can_visit_space? so we encapsulate the logic? I feel this is hard to follow

Is this returning a 404 Not Found HTTP status? I think we should.

mrcasals · 2018-02-09T09:07:49Z

decidim-assemblies/app/models/decidim/assembly.rb

+      joins("LEFT JOIN decidim_assembly_private_users ON
+             decidim_assembly_private_users.decidim_assembly_id = decidim_assemblies.id")
+        .where("(private_space = true and decidim_assembly_private_users.decidim_user_id
+             = #{user} ) or private_space = false")


Don't interpolate strings here, use ? to replace the values: .where("my_field = ?", my_value)

mrcasals · 2018-02-09T09:08:52Z

decidim-assemblies/app/models/decidim/assembly.rb

      slug
    end
+
+    def private_space?


Unneeded, Rails adds this for you.

mrcasals · 2018-02-09T09:10:15Z

decidim-assemblies/app/models/decidim/assembly.rb

+      private_space
+    end
+
+    def self.private_space


def private_assemblies maybe? So it doesn't clash with meaning with Decidim::Participable.public_spaces...

mrcasals · 2018-02-09T09:12:01Z

decidim-assemblies/app/models/decidim/assembly.rb

+      where(private_space: true)
+    end
+
+    def self.public_assembly


def public_assemblies, maybe? Although this might clash in meaning with Decidim::Participable.public_spaces...

Maybe something like def non_private_assemblies?

mrcasals · 2018-02-09T09:19:02Z

decidim-participatory_processes/app/models/decidim/participatory_process.rb

      slug
    end
+
+    def private_space?


mrcasals · 2018-02-09T09:19:10Z

decidim-participatory_processes/app/models/decidim/participatory_process.rb

+      private_space
+    end
+
+    def self.private_space


Same as in assemblies

mrcasals · 2018-02-09T09:19:18Z

decidim-participatory_processes/app/models/decidim/participatory_process.rb

+      where(private_space: true)
+    end
+
+    def self.public_process


Same as in assemblies

mrcasals · 2018-02-09T09:20:40Z

decidim-participatory_processes/app/models/decidim/participatory_process_private_user.rb

+# frozen_string_literal: true
+
+module Decidim
+  # An ParticipatoryProcessPrivateUser associate user with participatory process


Can you improve the docs? Reading this I don't understand why this class is used. Saying something like: "this class gives a given User access to a given private Process", or something like this, would be good.

Same for assemblies.

mrcasals · 2018-02-09T09:22:41Z

decidim-participatory_processes/config/locales/en.yml

          success: Participatory process group updated successfully.
+      participatory_process_private_users:
+        destroy:
+          success: Participatory process private user destroyed successfully.


I think this naming might be confusing for the end user. What about something like: "Private Process User Access"? Would this be clearer?

In case of doubt refer to @decidim/product 🤷‍♂️

josepjaume

Really good work! I posted some comments regarding some naming/design details I think could be improved. Some involve a bit of a refactoring over existing code, others are just a matter of separating concerns.

Sorry to be so nitpicky, but both processes and assemblies are central constructs of the project and are both gonna be used as an example for further development, so we need to put extra care in naming, conventions, etc.

But as I said, good work! This wasn't a trivial task to solve 👍

josepjaume · 2018-02-13T08:43:22Z

decidim-participatory_processes/app/models/decidim/participatory_process.rb

    scope :upcoming, -> { where(arel_table[:start_date].gt(Time.current)) }
    scope :active, -> { where(arel_table[:start_date].lteq(Time.current).and(arel_table[:end_date].gt(Time.current).or(arel_table[:end_date].eq(nil)))) }

+    scope :private_spaces_user, lambda { |user|


Since we're implementing this both in assemblies and participatory_processes, i'd scope the method name to something more specific, for readability purposes. private_participatory_processes_for and private_assemblies_for looks better to me. Same thing for assemblies. Otherwise the next developer might think there's a common abstraction - which there isn't.

I'm also not sure wether we should be returning all the public assemblies/processes as well. Looking at the method's name, one might expect for it to return only the private spaces for that user.

what you think if the scope name will be public_and_private_participatory_processes_for, as it returns all processes for the current_user?

What about just returning the private ones instead and dealing with that at the Queries down below? In case not, what about visible_participatory_processes_for?

josepjaume · 2018-02-13T09:01:19Z

decidim-assemblies/app/queries/decidim/assemblies/private_assemblies.rb

+module Decidim
+  module Assemblies
+    # This query class filters assemblies given a current_user.
+    class PrivateAssemblies < Rectify::Query


I've struggled a bit to understand what this class actually does. It looks like instead of returning PrivateAssemblies, it actually returns VisibleAssemblies right? If a user is provided, it will scope them to all the assemblies the user can see. Otherwise, it will return all the public assemblies.

josepjaume · 2018-02-13T09:03:49Z

decidim-assemblies/app/models/decidim/assembly.rb

+      where(private_space: true)
+    end
+
+    def self.non_private_assemblies


This class already extends Participable, which includes some functionality to figure out public spaces, as a means to provide statistics on the home page and other places. We probably should be using that instead (probably overriding it for the assemblies and processes case):

https://github.com/decidim/decidim/blob/master/decidim-core/lib/decidim/participable.rb#L82

Otherwise, data from private spaces might leak to statistics.

If I didn't go wrong. It was said that statistics would not take into account private processes, no?

josepjaume · 2018-02-13T09:04:28Z

decidim-participatory_processes/app/models/decidim/participatory_process.rb

+      where(private_space: true)
+    end
+
+    def self.non_private_processes


Same as in assemblies: We should probably be extending this instead: https://github.com/decidim/decidim/blob/master/decidim-core/lib/decidim/participable.rb#L82

josepjaume · 2018-02-13T09:09:25Z

...ory_processes/app/queries/decidim/participatory_processes/private_participatory_processes.rb

+module Decidim
+  module ParticipatoryProcesses
+    # This query class filters participatory processes given a current_user.
+    class PrivateParticipatoryProcesses < Rectify::Query


Same concerns as in PrivateAssemblies, please check them out 👍

josepjaume · 2018-02-13T09:10:10Z

...s/app/controllers/decidim/participatory_processes/participatory_process_groups_controller.rb

      def participatory_processes
-        @participatory_processes ||= group.participatory_processes.published
+        @participatory_processes ||= if current_user
+                                       group.participatory_processes.private_spaces_user(current_user.id).published


Maybe we could use the PrivateParticipatoryProcesses query here instead? It looks like it's mostly doing the same thing.

We are querin the processes of a group of processes... I don't thins that is the same..

Oh sorry! My fault.

xabier · 2018-02-14T07:20:47Z

almost there! 😄

Requested Changes applied

mrcasals · 2018-03-07T08:19:09Z

decidim-admin/app/commands/decidim/admin/create_participatory_space_private_user.rb


      def create_or_invite_user
-        @user ||= existing_user || new_user
+        @create_or_invite_user ||= existing_user || new_user


This seems to be copied from another command. The @user variable was set here so that the attr_reader works. Now the user method returns nil since this change.

@mrcasals then how can we solve this ? Naming/MemoizedInstanceVariableName: Memoized variable @user does not match method name create_or_invite_user. Use @ create_or_invite_user instead.

Make the method be called def user and remove the attr_reader

mrcasals · 2018-03-07T08:50:02Z

decidim-admin/app/commands/decidim/admin/create_participatory_space_private_user.rb


        ActiveRecord::Base.transaction do
-          create_or_invite_user
+          @user = @user ||= existing_user || new_user


This double assign is weird 😕

you are doing the same in @user = @user ||= existing_user || new_user in CreateAssemblyAdmin... 😕

And it's being discussed in the PR that added it. Might be some weird Rubocop change, but it's still weird and non-idiomatic. Can you change it, please? 😄

Sorry - I believe that's a Rubocop automated change 🤷‍♂️

So, can this PR be accepted?

isaacmg410 · 2018-03-15T07:41:53Z

Can you review it and accept PR? @decidim/lot-core

mrcasals · 2018-03-15T08:57:54Z

@isaacmg410 after merging #2851 some conflicts appeared, can you check them please?

isaacmg410 · 2018-03-15T09:07:34Z

@mrcasals done

josepjaume

Going in! 🎉🎉🎉🎉

#### 🎩 What? Why? This PR fixes that as a normal user (no private user) I can comment on a private assembly where is available. Meetigns, Proposals, etc. #### 📌 Related Issues - Related to #2618 #3438 - Fixes #4869 #### 📋 Subtasks - [x] Add `CHANGELOG` entry - [x] Add tests ### 📷 Screenshots (optional) ![Description](URL)

isaacmg410 added in-progress status: ❌ blocked status: design required labels Feb 1, 2018

ghost assigned isaacmg410 Feb 1, 2018

isaacmg410 commented Feb 1, 2018

View reviewed changes

carolromero removed the status: design required label Feb 5, 2018

isaacmg410 force-pushed the feature/2282_private_processes_or_assemblies branch from 9e67ab9 to 0d18580 Compare February 6, 2018 13:42

isaacmg410 removed the status: ❌ blocked label Feb 7, 2018

isaacmg410 force-pushed the feature/2282_private_processes_or_assemblies branch 3 times, most recently from 6eb36c9 to f60ee69 Compare February 8, 2018 15:14

isaacmg410 changed the title ~~[WIP] Feature/2282 private processes or assemblies~~ Feature/2282 private processes or assemblies Feb 8, 2018

isaacmg410 added in-review and removed in-progress labels Feb 8, 2018

mrcasals changed the title ~~Feature/2282 private processes or assemblies~~ Private processes or assemblies Feb 9, 2018

mrcasals previously requested changes Feb 9, 2018

View reviewed changes

isaacmg410 force-pushed the feature/2282_private_processes_or_assemblies branch from f60ee69 to bb32814 Compare February 12, 2018 09:25

josepjaume previously requested changes Feb 13, 2018

View reviewed changes

isaacmg410 force-pushed the feature/2282_private_processes_or_assemblies branch from cda1817 to c87ca5e Compare February 13, 2018 12:12

isaacmg410 mentioned this pull request Feb 20, 2018

Private processes or assemblies #2282

Closed

Isaac Massot added 2 commits March 7, 2018 09:08

Merge branch 'master' into feature/2282_private_processes_or_assemblies

c2d4e10

change MemoizedInstanceVariableName

5d4ceb7

isaacmg410 dismissed josepjaume’s stale review via 5d4ceb7 March 7, 2018 08:12

mrcasals reviewed Mar 7, 2018

View reviewed changes

change method Naming/MemoizedInstanceVariableName

e6327ab

mrcasals reviewed Mar 7, 2018

View reviewed changes

Isaac Massot added 6 commits March 7, 2018 10:00

change assigntion weird

d558376

update with master

cbba5f1

remove blank line

5e652fa

update with master and fix conflicts

9d4853d

fix specs

85d1a3a

Merge branch 'master' into feature/2282_private_processes_or_assemblies

c1f101b

mrcasals previously approved these changes Mar 15, 2018

View reviewed changes

oriolgual previously approved these changes Mar 15, 2018

View reviewed changes

Isaac Massot added 2 commits March 15, 2018 10:03

Merge branch 'master' into feature/2282_private_processes_or_assemblies

ff6d946

update master and fix conflicts

cf15bf6

isaacmg410 dismissed stale reviews from oriolgual and mrcasals via cf15bf6 March 15, 2018 09:06

josepjaume approved these changes Mar 15, 2018

View reviewed changes

josepjaume merged commit 419e3ba into master Mar 15, 2018

josepjaume deleted the feature/2282_private_processes_or_assemblies branch March 15, 2018 21:54

ghost removed the in-review label Mar 15, 2018

isaacmg410 mentioned this pull request May 16, 2018

Non private users can participate to a private, transparent assembly #3438

Merged

4 tasks

isaacmg410 mentioned this pull request Jun 11, 2018

fix private assemblies showing more than once for private users #3638

Merged

3 tasks

isaacmg410 mentioned this pull request Feb 22, 2019

[WIP] fix user can comment in private assembly even if user don't have permissions #4893

Closed

5 tasks

isaacmg410 mentioned this pull request Mar 4, 2019

Fix users can comment into private assembly #4924

Merged

2 tasks

		@@ -27,7 +27,16 @@
		end

		feature.register_stat :results_count, primary: true, priority: Decidim::StatsRegistry::HIGH_PRIORITY do \|features, _start_at, _end_at\|

	def public_participatory_spaces
	@public_participatory_spaces \|\|= Decidim.participatory_space_manifests.flat_map do \|manifest\|
	manifest.participatory_spaces.call(organization).public_spaces
	end
	end

Uh oh!

Conversation

isaacmg410 commented Feb 1, 2018 • edited by mrcasals Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carolromero commented Feb 5, 2018

Uh oh!

codecov bot commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

isaacmg410 commented Feb 7, 2018

Uh oh!

mrcasals commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isaacmg410 commented Feb 7, 2018

Uh oh!

mrcasals commented Feb 7, 2018

Uh oh!

isaacmg410 commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josepjaume left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isaacmg410 Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabier commented Feb 14, 2018

Uh oh!

isaacmg410 commented Feb 1, 2018 •

edited by mrcasals

Loading

codecov bot commented Feb 6, 2018 •

edited

Loading

mrcasals commented Feb 7, 2018 •

edited

Loading

isaacmg410 Feb 13, 2018 •

edited

Loading