Add organizer to meetings and private and transparent by isaacmg410 · Pull Request #3136 · decidim/decidim

isaacmg410 · 2018-04-05T10:59:33Z

🎩 What? Why?

This PR adds the type of meetings functionality and the organizer to the meeting.
The types of meetings are not visible on the public side, the markdown is pending.

📌 Related Issues

Related to Improvement to meetings MVP #3044

📋 Subtasks

mrcasals · 2018-04-11T07:50:45Z

decidim-meetings/app/assets/javascripts/decidim/meetings/admin/meetings_form.js.es6

+        );
+      },
+      renderItem: function (item, search) {
+        let sanitizedSearch = search.replace(/[-/\\^$*+?.()|[\]{}]/g, "\\$&");


Can you add a comment explaining what this Regexp is looking for?

mrcasals · 2018-04-11T07:52:19Z

decidim-meetings/app/controllers/decidim/meetings/admin/meetings_controller.rb

        end
+
+        def organizers
+          respond_to do |format|


CodeClimate reports this method as not covered by any test, maybe move this logic to a service object so it can be easily unit-tested?

controller tests for organizers done

mrcasals · 2018-04-11T07:53:04Z

decidim-meetings/app/helpers/decidim/meetings/admin/application_helper.rb

        include Decidim::MapHelper
+
+        MEETING_PUBLIC_TYPES = %w(public private other).freeze
+        MEETING_OPEN_TYPES = %w(open close other).freeze


Shouldn't this be closed (note the d)?

mrcasals · 2018-04-11T07:56:12Z

decidim-meetings/config/locales/en.yml

            title: Close meeting
+        meeting_copies:
+          form:
+            select_open_type: Select an open type


I feel like these sentences are not very clear, can you ask product for a better sentence? Open/closed are not types of "open", but different levels of accessibility to me 😕

The related issue has some sentences explaining what "open", "public" and "transparent" concepts mean for a meeting. I'd add this as a help text to the input, at least.

mrcasals · 2018-04-11T07:56:49Z

decidim-meetings/config/locales/en.yml

+        private: private
+        public: public
+      meeting_transparent_types:
+        opaque: opaque


What does "opaque meeting" imply? 😕

mrcasals · 2018-04-11T07:59:35Z

@isaacmg410 can you please add some screenshosts of how this would look?

isaacmg410 · 2018-04-11T08:11:37Z

@mrcasals we are still working on this, pending to talk with product about how private_meetings should work better.
Any idea @decidim/lot-core ?

oriolgual · 2018-04-24T07:45:02Z

decidim-meetings/app/commands/decidim/meetings/admin/copy_meeting.rb

            location_hints: @form.location_hints,
            services: @form.services_to_persist.map { |service| { "title" => service.title, "description" => service.description } },
            component: @meeting.component,
+            is_private: @form.is_private,


Could you please you private and transparent? See https://github.com/bbatsov/ruby-style-guide#bool-methods-prefix (not sure why Rubocop doesn't complain)

but private is a reserved key, no? so maybe we can use private_meeting. What you think?

Oh, didn't though of that. It seems it can be used but I'm not sure if there will be a problem. Maybe @deivid-rodriguez knows about ut?

You can use private? instead, which should not clash

I'd say private is a valid method name... I can't think of any ambiguity from using it.

That is a valid style remark, but in my opinion is ok. If the column name is an adjective, boolean is the only type that makes sense, no? And it's also the style used with other boolean columns. Basically we never use is_ prefixes.

Actually I just read https://stackoverflow.com/questions/16099073/ruby-reserved-class-method-names and it looks like @isaacmg410 is right and we shouldn't do this... For example, if you define a private class method, it makes the private keyword lose its meaning and instead calls the class method... So sorry for the extra friction here Isaac but I think it's probably safer to go back to your original version?

😕 so.... some possibilities:
is_private
private_meeting
for_only_invited
only_invited

Which of the options do you think is better?
@decidim/lot-core @deivid-rodriguez

private_meeting works for me!

private_meeting looks perfect to me as well! Sorry again @isaacmg410 for the back and forth.

oriolgual · 2018-04-24T07:45:58Z

decidim-meetings/app/controllers/decidim/meetings/admin/meetings_controller.rb

+        def organizers
+          respond_to do |format|
+            format.json do
+              query = current_organization.users&.order(name: :asc)


If users is an ActiveRecord::Relation you don't need the &

oriolgual · 2018-04-24T07:46:33Z

decidim-meetings/app/controllers/decidim/meetings/admin/meetings_controller.rb

+              else
+                query = query.where("name ILIKE ?", "%#{params[:term]}%")
+              end
+              render json: query.all.collect { |p| [p.id, p.name, p.nickname] }


Maybe query.pluck(:id, :name, :nickname)?

oriolgual · 2018-04-24T07:47:08Z

decidim-meetings/app/controllers/decidim/meetings/meetings_controller.rb

      end
+
+      def current_user_can_visit_meeting?
+        (meeting.try(:is_private?) &&


Adding a return unless meeting would prevent all the trys

Also, maybe this logic should be inside a method in a Meeting?

oriolgual · 2018-04-24T07:48:35Z

decidim-meetings/app/controllers/decidim/meetings/meetings_controller.rb

      end

+      def show
+        check_current_user_can_visit_meeting


Is this method used elsewhere? If not, consider moving the logic here.

oriolgual · 2018-04-24T07:49:34Z

decidim-meetings/app/helpers/decidim/meetings/meetings_helper.rb

        CGI.unescapeHTML html_truncate(description, max_length: max_length, tail: tail)
      end
+
+      def meeting_type_badge_css_class(type)


Could you please addd some docs?

oriolgual · 2018-04-24T07:53:03Z

decidim-meetings/app/controllers/decidim/meetings/admin/meetings_controller.rb

          end
        end

+        def organizers


Instead of adding a custom action, could you add an OrganizersController with an index action? (scoped to a meeting)

is this necessary to be scoped to a meeting? if we are returning all the users of the current organization?

Also in accountability there are something similar with proposals and there is created as a custom action.

What is better?

Oh, if it's used somewhere else then no need to scope it to a meeting!

What about this? I'm OK with not scoping it to a meeting but I'd create an OrganizersController anyway.

Done! I create a UsersController to be reusable to other components

mrcasals · 2018-04-24T07:53:38Z

decidim-meetings/app/presenters/decidim/meetings/admin_log/meeting_presenter.rb

+            public_type_other: :string,
+            transparent_type: :string,
+            transparent_type_other: :string,
+            organizer_id: :integer


This will only show the ID, which doesn't provide any info the the user checking the log. Can you make a new ResourcePresenter for the organizer so it's properly displayed?

You can check NewsletterResourcePresenter for example

isaacmg410 · 2018-05-03T06:32:22Z

@decidim/lot-core Requested changes applied, can you review it please?

oriolgual · 2018-05-03T07:12:56Z

decidim-meetings/app/models/decidim/meetings/meeting.rb

+        return false if participatory_space.try(:private_space?) && participatory_space.try(:transparent?)
+      end
+
+      def can_participate_meeting?(current_user)


Why this needs to be a separate method from can_participate?

Because Rubocop complains...

I think you can add a skip rule to rubocop.yml

isaacmg410 · 2018-05-09T07:12:57Z

@decidim/lot-core all checks passed :)

* meeting types * add database fields * add fields to commands and js * add organizer to meetings, pending frontend * add organizer to frontend, pending specs * improve specs, and locales * remove comments * fix data picker builder form * fix locales, and form builder * add changelog line. * change data picker, to select picker of belongs_to * fix codeclimate * uncoment specs * remove comment routes * mettings only visibles for users registerd * fix meeting_form, add labels, pending specs * fix some specs * fix rubocop offenses * fix specs * add space before %> on layout. * rmeove unnecessary regexp and add spec for organizers json * apply changes requested on review and add system spec * fix rubocop offenses, and requested changes * remove method on controller and use one of model * fix methods for specs * change booleans field names, specs, and apply requested changes * change to private_meeting * Change organizers action to users controller index action * fix rubocop and when copy meeting * group two methods into one

Isaac Massot added 9 commits April 3, 2018 16:05

meeting types

6383aa1

add database fields

bb4edac

add fields to commands and js

4e15c80

add organizer to meetings, pending frontend

fae85f5

Merge branch 'master' into 3044_meeting_types

a0be0c0

add organizer to frontend, pending specs

9f409e7

improve specs, and locales

3f3738c

remove comments

1aeaba1

fix conflicts

23c6ccb

ghost assigned isaacmg410 Apr 5, 2018

ghost added the status: WIP label Apr 5, 2018

Isaac Massot added 2 commits April 5, 2018 15:38

fix data picker builder form

f8e37ea

fix locales, and form builder

ce2dc7d

isaacmg410 changed the title ~~[WIP] Add meeting types selector and organizer to meetings~~ Add meeting types selector and organizer to meetings Apr 6, 2018

add changelog line.

9aeb7fa

xabier mentioned this pull request Apr 6, 2018

Improvement to meetings MVP #3044

Closed

16 tasks

isaacmg410 added in-review and removed status: WIP labels Apr 6, 2018

isaacmg410 changed the title ~~Add meeting types selector and organizer to meetings~~ [WIP] Add meeting types selector and organizer to meetings Apr 9, 2018

isaacmg410 added the status: WIP label Apr 9, 2018

Isaac Massot added 2 commits April 10, 2018 08:48

Merge branch 'master' into 3044_meeting_types

fca40b0

change data picker, to select picker of belongs_to

62ed9d9

isaacmg410 changed the title ~~[WIP] Add meeting types selector and organizer to meetings~~ Add meeting types selector and organizer to meetings Apr 10, 2018

Isaac Massot added 4 commits April 10, 2018 10:09

fix codeclimate

1263896

Merge branch 'master' into 3044_meeting_types

38887fc

uncoment specs

6c72589

remove comment routes

5ceb39b

mrcasals reviewed Apr 11, 2018

View reviewed changes

ghost added the status: WIP label Apr 24, 2018

oriolgual suggested changes Apr 24, 2018

View reviewed changes

mrcasals suggested changes Apr 24, 2018

View reviewed changes

Isaac Massot added 3 commits April 24, 2018 13:28

change booleans field names, specs, and apply requested changes

597c140

Merge branch 'master' into 3044_meeting_types

93f3ac0

change to private_meeting

3088516

isaacmg410 removed the status: WIP label Apr 25, 2018

oriolgual previously approved these changes Apr 26, 2018

View reviewed changes

Merge branch 'master' into 3044_meeting_types

9b0e489

isaacmg410 dismissed oriolgual’s stale review via 9b0e489 April 27, 2018 09:19

ghost added the status: WIP label Apr 27, 2018

Merge branch 'master' into 3044_meeting_types

13e47ef

isaacmg410 removed the status: WIP label May 3, 2018

oriolgual suggested changes May 3, 2018

View reviewed changes

isaacmg410 mentioned this pull request May 3, 2018

An assembly member can be an existing user #3302

Merged

1 task

Isaac Massot added 2 commits May 4, 2018 10:57

merge master

8a4e7f7

Change organizers action to users controller index action

848af84

ghost added the status: WIP label May 4, 2018

Isaac Massot added 3 commits May 4, 2018 13:00

fix rubocop and when copy meeting

82fa065

group two methods into one

9f6e1e5

Merge branch 'master' into 3044_meeting_types

6aa9fb5

isaacmg410 removed the status: WIP label May 8, 2018

oriolgual approved these changes May 9, 2018

View reviewed changes

mrcasals approved these changes May 9, 2018

View reviewed changes

mrcasals merged commit eb75f6f into master May 9, 2018

mrcasals deleted the 3044_meeting_types branch May 9, 2018 07:24

rbngzlv mentioned this pull request May 10, 2018

Move users endpoint from core to admin #3381

Merged

1 task

Uh oh!

Conversation

isaacmg410 commented Apr 5, 2018 • edited 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!

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!

mrcasals commented Apr 11, 2018

Uh oh!

isaacmg410 commented Apr 11, 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!

isaacmg410 Apr 24, 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!

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!

isaacmg410 commented May 3, 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!

isaacmg410 commented Apr 5, 2018 •

edited

Loading

isaacmg410 Apr 24, 2018 •

edited

Loading