Conferences related to other spaces by isaacmg410 · Pull Request #4339 · decidim/decidim

isaacmg410 · 2018-10-23T08:25:16Z

🎩 What? Why?

This PR adds the Relationship with other spaces. Each Conference-page should potentially be related to participatory processes, consultations and assemblies.

📌 Related Issues

Related to EPIC: Conference/events module #3709 Conference/events module 1st Iteration #4305
Fixes #?

📋 Subtasks

📷 Screenshots (optional)

isaacmg410 · 2018-10-23T08:27:25Z

@decidim/lot-px @decidim/product Can you review the screenshots attached to the description ti see if the design of the related spaces is correct? If it is not, we need the design soon...

decidim-conferences/app/helpers/decidim/conferences/admin/conferences_helper.rb

mrcasals · 2018-10-23T09:20:03Z

decidim-conferences/app/helpers/decidim/conferences/conferences_helper.rb

    module ConferencesHelper
      include Decidim::ResourceHelper
+
+      def participatory_processes_for_conference(conference_participatory_processes)


Move these new methods to cells

mrcasals · 2018-10-23T09:20:19Z

decidim-conferences/app/views/decidim/conferences/conferences/show.html.erb


    <%= attachments_for current_participatory_space %>

+    <%= participatory_processes_for_conference(conference_participatory_processes) if conference_participatory_processes.present? %>


Use the new cells here

carolromero · 2018-10-23T09:23:26Z

@decidim/lot-px @decidim/product Can you review the screenshots attached to the description ti see if the design of the related spaces is correct? If it is not, we need the design soon...

@isaacmg410 it is fine

isaacmg410 · 2018-10-23T13:17:19Z

@mrcasals requested changes applied 😄

isaacmg410 · 2018-10-24T06:44:13Z

@decidim/lot-core ready to be merged 😄

mrcasals

The current solution is linked to processes/assemblies/consultations. There's no way to use external participatory spaces, which I think would be nice.

Also, why not include initiatives here too? 😕

mrcasals · 2018-10-24T07:03:57Z

decidim-conferences/app/cells/decidim/conferences/participatory_spaces_cell.rb

+
+module Decidim
+  module Conferences
+    # This cell renders the participatory spaces card for an instance of a Participatory Space


These docs seem wrong, as reading this it seems it receives a single participatory space as model, but it in fact receives a collection of participatory spaces. Can you add a line explaining this please? 😄

mrcasals · 2018-10-24T07:05:31Z

decidim-conferences/app/commands/decidim/conferences/admin/create_conference.rb

+        def consultations(conference)
+          @consultations ||= conference.participatory_space_sibling_scope(:consultations)
+                                       .includes(:consultation)
+                                       .where(decidim_consultation_id: @form.consultations_ids)


I'm not sure about this, since it implies it knows a lot about how consultations work internally 😕

when you make conference.participatory_space_sibling_scope(:consultations) it returns questions. The Consultations module says that Question is a participatory space 😕

mrcasals · 2018-10-24T07:06:10Z

decidim-conferences/app/commands/decidim/conferences/admin/create_conference.rb

          )
        end
+
+        def participatory_processes(conference)


No need to pass conference around as a parameter, you have access to it globally already.

mrcasals · 2018-10-24T07:06:50Z

decidim-conferences/app/commands/decidim/conferences/admin/update_conference.rb

            .perform_later(@conference.id, checksum)
        end
+
+        def participatory_processes(conference)


Same, remove the param

mrcasals · 2018-10-24T07:07:53Z

decidim-conferences/app/commands/decidim/conferences/admin/update_conference.rb

+        def consultations(conference)
+          @consultations ||= conference.participatory_space_sibling_scope(:consultations)
+                                       .includes(:consultation)
+                                       .where(decidim_consultation_id: @form.consultations_ids)


Same problem, I think we know too much about how consultations work. What happens if the app doesn't have decidim-consultations? It's an optional gem, and it's not bundled with decidim metagem 😕

So the principal concern here is: why can't you use a simple where(id: @form.consultation_ids) here? Why all these includes and collects?

Is more or less what I say before. The Participatory Space in Consultations mdoule is Question. FI we need to related Conference with Consultation we need to do something like that

Although I agree it's weird, both Consultations and Questions are participatory spaces, how come if you ask for consultations you get questions?

The issue seems to be here:

https://github.com/decidim/decidim/blob/master/decidim-consultations/lib/decidim/consultations/participatory_space.rb#L3-L27

I don't know why questions are the space and consultations the resource.

What happens if you change the model_class_name to Consultation @isaacmg410?

Will many things break up? Then we will change the register_resource to Question?

I'm going to try it at my machine.

I changed:

participatory_space.model_class_name = "Decidim::Consultations::Question"

To

participatory_space.model_class_name = "Decidim::Consultation"

And

Decidim::Consultations::Question.where(organization: organization)

To

Decidim::Consultation.where(organization: organization)

At
https://github.com/decidim/decidim/blob/master/decidim-consultations/lib/decidim/consultations/participatory_space.rb

And all tests pass and the navigation seems to work OK. Now conference.participatory_space_sibling_scope(:consultations) returns consultations as expected.

I will try also, and if all is okey. Go for it?

mrcasals · 2018-10-24T07:09:21Z

decidim-conferences/app/controllers/decidim/conferences/conferences_controller.rb

+      end
+
+      def conference_consultations
+        @conference_consultations ||= @current_participatory_space.linked_participatory_space_resources("Consultation", "included_consultations")


Why are you using a String here instead of a Symbol?

because if I use a symbol :consultations it returns a Question

mrcasals · 2018-10-24T07:10:56Z

decidim-conferences/app/forms/decidim/conferences/admin/conference_form.rb

+        end
+
+        def consultations_for_select
+          @consultations_for_select ||= Decidim.find_participatory_space_manifest(:consultations)


Same, what happens with this collect if decidim-consultations is not installed?

The same as I said before

mrcasals · 2018-10-24T07:12:14Z

decidim-conferences/app/helpers/decidim/conferences/conferences_helper.rb

      include Decidim::ResourceHelper
+
+      def participatory_spaces_for_conference(conference_participatory_spaces)
+        cell("decidim/conferences/participatory_spaces", conference_participatory_spaces)


I think you can safely replace the method call with the cell "blabla" one, so we can remove this whole file

decidim-conferences/app/helpers/decidim/conferences/admin/conferences_helper.rb

mrcasals · 2018-10-24T07:14:05Z

decidim-conferences/app/controllers/decidim/conferences/conferences_controller.rb

        @stats ||= ConferenceStatsPresenter.new(conference: current_participatory_space)
      end
+
+      def conference_participatory_processes


This logic could be wrapped in cells too. Those cells can render ParticipatorySpacesCell directly.

What do you think about this?

isaacmg410 · 2018-10-24T07:48:03Z

@mrcasals

The current solution is linked to processes/assemblies/consultations. There's no way to use external participatory spaces, which I think would be nice.

Also, why not include initiatives here too? confused

How we can add here external participatory spaces?
The issue only says, processes, assemblies and consultations.

isaacmg410 · 2018-10-24T10:56:09Z

@decidim/lot-core, requested changes applied 😄

mrcasals · 2018-10-24T12:27:25Z

decidim-conferences/app/views/decidim/conferences/conferences/show.html.erb

-    <%= participatory_spaces_for_conference(conference_assemblies) if conference_assemblies.present? %>
-    <%= participatory_spaces_for_conference(conference_consultations) if conference_consultations.present? %>
+    <%= cell("decidim/conferences/linked_participatory_spaces", current_participatory_space) %>
+    <%#= participatory_spaces_for_conference(conference_participatory_processes) if conference_participatory_processes.present? %>


Can you remove this commented code please? 😄

Done! Sorry man! 😃

isaacmg410 · 2018-10-24T14:27:37Z

@decidim/lot-core all checks are green 😄 🎉

ghost assigned isaacmg410 Oct 23, 2018

ghost added the status: WIP label Oct 23, 2018

carolromero mentioned this pull request Oct 23, 2018

Conference/events module 1st Iteration #4305

Closed

27 tasks

mrcasals suggested changes Oct 23, 2018

View reviewed changes

isaacmg410 force-pushed the conferences_related_to_other_spaces branch 2 times, most recently from b5215a3 to 445b7ca Compare October 23, 2018 13:14

isaacmg410 added in-review and removed status: WIP labels Oct 23, 2018

ghost added the status: WIP label Oct 23, 2018

isaacmg410 removed the status: WIP label Oct 23, 2018

ghost added the status: WIP label Oct 23, 2018

isaacmg410 removed the status: WIP label Oct 24, 2018

mrcasals suggested changes Oct 24, 2018

View reviewed changes

isaacmg410 force-pushed the conferences_related_to_other_spaces branch from bd923b6 to 317ff39 Compare October 24, 2018 09:36

ghost added the status: WIP label Oct 24, 2018

Isaac Massot added 7 commits October 24, 2018 12:55

initial commit when relating conferences with other spaces

1de29e6

fix specs conferences linked to other resources

59a7a82

fix rubocop offenses

e9bc03a

move helpers to cell

2588779

add changelog line

d25d9b2

add translations to i18n-tasks to be ignored

1dad70b

apply requested changes, delete helper and move to cell

3cfd22c

isaacmg410 force-pushed the conferences_related_to_other_spaces branch from 317ff39 to 3cfd22c Compare October 24, 2018 10:55

isaacmg410 removed the status: WIP label Oct 24, 2018

mrcasals approved these changes Oct 24, 2018

View reviewed changes

mrcasals suggested changes Oct 24, 2018

View reviewed changes

remove comented code

1369d8e

ghost added the status: WIP label Oct 24, 2018

mrcasals approved these changes Oct 24, 2018

View reviewed changes

mrcasals merged commit f2a53ba into master Oct 25, 2018

mrcasals deleted the conferences_related_to_other_spaces branch October 25, 2018 07:11

ghost removed the status: WIP label Oct 25, 2018


		<%= attachments_for current_participatory_space %>

		<%= participatory_processes_for_conference(conference_participatory_processes) if conference_participatory_processes.present? %>

Uh oh!

Conversation

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

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

Uh oh!

isaacmg410 commented Oct 23, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carolromero commented Oct 23, 2018

Uh oh!

isaacmg410 commented Oct 23, 2018

Uh oh!

isaacmg410 commented Oct 24, 2018

Uh oh!

mrcasals left a comment

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!

oriolgual Oct 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isaacmg410 commented Oct 24, 2018

Uh oh!

isaacmg410 commented Oct 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isaacmg410 commented Oct 24, 2018

Uh oh!

isaacmg410 commented Oct 23, 2018 •

edited

Loading

oriolgual Oct 24, 2018 •

edited

Loading