Close a meeting by oriolgual · Pull Request #525 · decidim/decidim

oriolgual · 2017-01-17T11:41:57Z

🎩 What? Why?

Adds a feature to close a meeting. In order to close it, you need to specify which proposals where created during that meeting.

To do this I've created a way to link resources (meetings, proposals, whatever) between them without having to create dependencies between engines.

📌 Related Issues

Related to Close a meeting #299

📋 Subtasks

📷 Screenshots (optional)

👻 GIF

mention-bot · 2017-01-17T11:41:59Z

@oriolgual, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mrcasals, @Dor3nz and @josepjaume to be potential reviewers.

houndci-bot · 2017-01-17T11:42:10Z

decidim-core/lib/decidim/resourceable.rb

Missing top-level module documentation comment.

houndci-bot · 2017-01-17T11:42:10Z

decidim-core/lib/decidim/resource_manifest.rb

Missing top-level class documentation comment.

houndci-bot · 2017-01-17T11:42:11Z

decidim-admin/lib/decidim/admin/features/base_controller.rb

Useless private access modifier.

codecov-io · 2017-01-17T13:29:04Z

Current coverage is 97.26% (diff: 98.76%)

Merging #525 into master will increase coverage by 0.05%

@@             master       #525   diff @@
==========================================
  Files           262        269     +7   
  Lines          4116       4277   +161   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4001       4160   +159   
- Misses          115        117     +2   
  Partials          0          0

Powered by Codecov. Last update 3a0693a...f6d79a2

mention-bot · 2017-01-17T15:31:47Z

@oriolgual, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mrcasals, @josepjaume and @Dor3nz to be potential reviewers.

oriolgual · 2017-01-20T15:25:18Z

I'll handle the autocomplete field in another PR

beagleknight · 2017-01-23T08:09:38Z

decidim-core/app/helpers/decidim/resource_helper.rb

I think we need to decide a style for this so everyone uses _ for private methods or not.

I don't have a strong opinion on this, but I'm OK with doing it.

mrcasals · 2017-01-23T08:19:18Z

decidim-core/lib/decidim/feature_manifest.rb

docs are outdated (the only param is a block)

mrcasals · 2017-01-23T08:20:01Z

decidim-core/lib/decidim/resource_manifest.rb

s/this/these

mrcasals · 2017-01-23T08:20:14Z

decidim-core/lib/decidim/resource_manifest.rb

s/is sets/sets/

mrcasals · 2017-01-23T08:23:10Z

decidim-core/lib/decidim/resource_manifest.rb

I wonder if this can break things at L'Hospitalet repo. In order to diferentiate the built-in engines form the ones from Decidim, I named the survey engine as decidim_hospitalet/surveys, and it looks like this could not work well. Is there any way to Overwrite this? Can we use an attribute for this and, if blank, delegate to this value?

This is confusing as the mounted_engine_name doesn't have anything to do with the engine_name, as routes are mounted using the feature_name instead.

@oriolgual maybe you could change this to mounted_feature_name?

This is actually the name of the mounted route: https://github.com/AjuntamentdeBarcelona/decidim/blob/master/decidim-core/config/routes.rb#L32

I guess it isn't related to the engine name per se (the class)

@oriolgual what I said holds true, maybe a renaming is due 🎩

OK, I hope we don't have collisions on this :/

@josepjaume but what I actually want is the name of the mounted engine that rails is suing so I can call: decidim_meetings.meeting_path for example, and this is not a feature it's an engine.

oh ok, nevermind!

mrcasals · 2017-01-23T08:27:17Z

Shouldn't we define the ResourceManifest in each existing feature, or this works automagically?

beagleknight

Outstanding work man! I just commented a few things because I don't understand some decisions. If you can elaborate them it will be useful.

beagleknight · 2017-01-23T08:10:11Z

decidim-core/app/models/decidim/resource_link.rb

s/eachother/each other

beagleknight · 2017-01-23T08:16:26Z

decidim-core/lib/decidim/resourceable.rb

If the link_name is supposed to be a String why do you need to call to_s?

This to_s is redundant, since if you pass a Symbol it will automatically be converted to a String when performing the query

beagleknight · 2017-01-23T08:16:31Z

decidim-core/lib/decidim/resourceable.rb

beagleknight · 2017-01-23T08:24:42Z

decidim-meetings/decidim-meetings.gemspec

Why do we need that dependency as development and not as a normal one?

Because this way you could have the meetings engine without the proposals one (this why I added the ResourceLink concept). But it's needed at dev in order to test it.

beagleknight · 2017-01-23T08:27:18Z

decidim-meetings/lib/decidim/meetings/admin_engine.rb

Why didn't use a scoped resource here?

Umm I guess I can do it.

beagleknight · 2017-01-23T08:29:57Z

decidim-proposals/decidim-proposals.gemspec

mrcasals · 2017-01-23T08:32:24Z

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

IMO this should be new/create instead of edit/update, since what you are dealing with here is the event of closing a meeting, not the meeting itself, so you are creating a "close this meeting" event, not editing the meeting

The thing is that there aren't any close meeting model, it's just updating an existing model. I guess I can add edit/update too but they'll do the same as new/create.

Yeah, I know there's no model for this. See this controller:

https://github.com/AjuntamentdeBarcelona/decidim/blob/master/decidim-admin/app/controllers/decidim/admin/participatory_process_step_activations_controller.rb

Activating a step is not related to an "activation model" either, but I'm creating the activation, that's why I used create/destroy there (destroy was removed since we decided that we could not deactivate steps anymore).

Anyway, that might just be me here, might be just my opinion.

I guess I can use new/create even when we're re-closing a meeting.

mrcasals · 2017-01-23T08:33:29Z

decidim-meetings/app/views/decidim/meetings/meetings/_linked_meetings.html.erb

Wrong identation

mrcasals · 2017-01-23T08:34:28Z

decidim-meetings/decidim-meetings.gemspec

Shouldn't this be add_dependency?

No, because we don't want meetings to depend on proposals.

mrcasals · 2017-01-23T08:36:48Z

decidim-meetings/spec/shared/manage_meetings_examples.rb

Group of People Complaining About Something LOL

mrcasals

See comments

oriolgual · 2017-01-23T09:40:10Z

@mrcasals you only need to define a ResourceManifest if you want to expose a resource (that's what we'll need to do at Results).

josepjaume

Epic work you did here. Can't help to think all this could be moved to a decidim-resources module.

josepjaume · 2017-01-23T08:54:54Z

decidim-core/app/helpers/decidim/resource_helper.rb

Still unsure wether the header should belong here, but let's leave it like that for the moment.

josepjaume · 2017-01-23T08:56:04Z

decidim-core/app/models/decidim/resource_link.rb

Even if we don't allow null names in the DB, I think we should validate presence here anyway.

It's just on the previous line!

josepjaume · 2017-01-23T08:58:35Z

decidim-core/lib/decidim/core/test/factories.rb

JúsepCI: Remove blank line

josepjaume · 2017-01-23T08:59:44Z

decidim-core/lib/decidim/feature_manifest.rb

Wrong docs: this line is just a copy-paste from FeatureManifest and doesn't make sense in this context

josepjaume · 2017-01-23T09:04:19Z

decidim-core/lib/decidim/resource_manifest.rb

This is confusing as the mounted_engine_name doesn't have anything to do with the engine_name, as routes are mounted using the feature_name instead.

@oriolgual maybe you could change this to mounted_feature_name?

josepjaume · 2017-01-23T09:42:39Z

decidim-core/lib/decidim/resourceable.rb

Isn't this kind of private-ish?

Not really, I'm using it at CloseMeetingForm to load all the proposals in the same process.

josepjaume · 2017-01-23T10:17:06Z

decidim-dev/lib/decidim/dev/test/rspec_support/feature.rb

Shouldn't we split this feature in smaller files somewhere else?

Maybe, but after having to deal with it and understanding it felt that it was easier to use it and modify it when everything is together. Otherwise it might be difficult to see the whole picture. I can split it though if you think it's worth it.

josepjaume · 2017-01-23T10:17:17Z

decidim-dev/lib/decidim/dev/test/rspec_support/feature.rb

josepjaume · 2017-01-23T10:18:21Z

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

Why not present??

josepjaume · 2017-01-23T10:20:13Z

decidim-meetings/lib/decidim/meetings/feature.rb

Don't we need to specify a resource name like register_feature?

We can, but since it's optional you can do it at the block.

- revert 7b6f2e5 for `author_event` - add conditional checks for `UserPresenter` on Organization class

- reverting 25bd9a4 - reverting 7b6f2e5 - better fix comes from decidim@7372946

* [BACKPORT] Fix user presenter for user groups (decidim#4974) * Fix user presenter for user groups (decidim#4973) * Fix user presenter for user groups The user groups do not have a `.deleted?` method defined for them which caused the user presenter to break the notifications view in case the notification was linking to a group user. * Add CHANGELOG entry * Fix changelog * 🐛 ⏪ Better fixes for decidim#525 - reverting 25bd9a4 - reverting 7b6f2e5 - better fix comes from decidim@7372946 * 🌐 amendable event FR label decidim#532 * 🐛 HashtagRenderer in comment events decidim#532 * 🐛 fix step_cta_path decidim#488 * 🌐 [pages] FR "Titre" >> "Page" !

oriolgual added the wip label Jan 17, 2017

houndci-bot reviewed Jan 17, 2017

View reviewed changes

decidim-core/lib/decidim/resourceable.rb Outdated

Copy link
Copy Markdown

houndci-bot Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing top-level module documentation comment.

houndci-bot reviewed Jan 17, 2017

View reviewed changes

decidim-core/lib/decidim/resource_manifest.rb Outdated

Copy link
Copy Markdown

houndci-bot Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing top-level class documentation comment.

houndci-bot reviewed Jan 17, 2017

View reviewed changes

decidim-admin/lib/decidim/admin/features/base_controller.rb Outdated

Copy link
Copy Markdown

houndci-bot Jan 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless private access modifier.

oriolgual requested review from ItsGenis, andreslucena, beagleknight, divins, lastpotion and mrcasals January 17, 2017 13:07

oriolgual force-pushed the feature/basic_meeting_close branch from 743495c to dbc5691 Compare January 17, 2017 13:29

oriolgual force-pushed the feature/basic_meeting_close branch from dbc5691 to 16e9c64 Compare January 17, 2017 15:31

oriolgual force-pushed the feature/basic_meeting_close branch 6 times, most recently from 87b6334 to e4c3745 Compare January 20, 2017 15:25

oriolgual removed the wip label Jan 20, 2017

oriolgual force-pushed the feature/basic_meeting_close branch 2 times, most recently from 4840706 to 718694f Compare January 21, 2017 12:02

oriolgual requested a review from josepjaume January 21, 2017 12:20

beagleknight reviewed Jan 23, 2017

View reviewed changes

mrcasals suggested changes Jan 23, 2017

View reviewed changes

beagleknight reviewed Jan 23, 2017

View reviewed changes

mrcasals reviewed Jan 23, 2017

View reviewed changes

decidim-meetings/app/views/decidim/meetings/meetings/_linked_meetings.html.erb Outdated

Copy link
Copy Markdown

Contributor

mrcasals Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong identation

mrcasals reviewed Jan 23, 2017

View reviewed changes

decidim-meetings/spec/shared/manage_meetings_examples.rb Outdated

Copy link
Copy Markdown

Contributor

mrcasals Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Group of People Complaining About Something LOL

mrcasals suggested changes Jan 23, 2017

View reviewed changes

josepjaume suggested changes Jan 23, 2017

View reviewed changes

Resources can be linked between them

c023dd2

oriolgual force-pushed the feature/basic_meeting_close branch from 718694f to ba85bfb Compare January 23, 2017 10:35

oriolgual removed request for ItsGenis, andreslucena, divins and lastpotion January 23, 2017 10:36

Close a meeting and link it to proposals

f6d79a2

oriolgual force-pushed the feature/basic_meeting_close branch from ba85bfb to f6d79a2 Compare January 23, 2017 11:27

mrcasals approved these changes Jan 23, 2017

View reviewed changes

josepjaume approved these changes Jan 23, 2017

View reviewed changes

josepjaume merged commit d93fdf1 into master Jan 23, 2017

josepjaume deleted the feature/basic_meeting_close branch January 23, 2017 13:01

arnaumonty mentioned this pull request Feb 10, 2017

Two problems when you close a meeting #884

Closed

2 tasks

aitorlb pushed a commit to CodiTramuntana/decidim that referenced this pull request Aug 8, 2019

🐛 fix author_presenter for official debates decidim#525

7b6f2e5

aitorlb pushed a commit to CodiTramuntana/decidim that referenced this pull request Aug 8, 2019

🐛 better fix for decidim#525

25bd9a4

- revert 7b6f2e5 for `author_event` - add conditional checks for `UserPresenter` on Organization class

aitorlb pushed a commit to CodiTramuntana/decidim that referenced this pull request Aug 8, 2019

🐛 ⏪ Better fixes for decidim#525

cf9a7ea

- reverting 25bd9a4 - reverting 7b6f2e5 - better fix comes from decidim@7372946

Uh oh!

Conversation

oriolgual commented Jan 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

👻 GIF

Uh oh!

mention-bot commented Jan 17, 2017

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!

codecov-io commented Jan 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 97.26% (diff: 98.76%)

Uh oh!

mention-bot commented Jan 17, 2017

Uh oh!

oriolgual commented Jan 20, 2017

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!

mrcasals commented Jan 23, 2017

Uh oh!

beagleknight 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

oriolgual commented Jan 17, 2017 •

edited

Loading

codecov-io commented Jan 17, 2017 •

edited

Loading