Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Relate processes and scopes #1017

Merged
merged 20 commits into from
Feb 28, 2017
Merged

Relate processes and scopes #1017

merged 20 commits into from
Feb 28, 2017

Conversation

mrcasals
Copy link
Contributor

@mrcasals mrcasals commented Feb 21, 2017 •
edited
Loading

🎩 What? Why?

Relate processes and scopes, and revert part of the changes done for #835.

📌 Related Issues

📋 Subtasks

  • Relate process and scopes
  • Remove toggle from proposals
  • Proposals: remove filter when process has related scope
  • Proposals: relate proposals to process scope when process has related scope
  • Results: remove filter when process has related scope
  • Results: relate results to process scope when process has related scope
  • Meetings: remove filter when process has related scope
  • Meetings: relate meetings to process scope when process has related scope
  • Projects: remove filter when process has related scope
  • Projects: relate budgets to process scope when process has related scope

@mrcasals mrcasals self-assigned this Feb 21, 2017
@mrcasals mrcasals mentioned this pull request Feb 21, 2017
Closed
@mrcasals mrcasals force-pushed the relate_process_with_scopes branch from a0e5ef2 to 975835b Compare February 21, 2017 13:51
@codecov-io
Copy link

codecov-io commented Feb 21, 2017 •
edited
Loading

Codecov Report

Merging #1017 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1017      +/-   ##
==========================================
+ Coverage   97.16%   97.19%   +0.02%     
==========================================
  Files         395      398       +3     
  Lines        6602     6663      +61     
==========================================
+ Hits         6415     6476      +61     
  Misses        187      187
Impacted Files Coverage Δ
decidim-proposals/lib/decidim/proposals/feature.rb 100% <ø> (ø) ✅
decidim-core/db/seeds.rb 100% <ø> (ø) ✅
...ands/decidim/admin/create_participatory_process.rb 100% <ø> (ø) ✅
...ecidim/admin/participatory_processes_controller.rb 92.15% <100%> (+0.15%) ✅
...osals/app/forms/decidim/proposals/proposal_form.rb 100% <100%> (ø) ✅
...lts/app/forms/decidim/results/admin/result_form.rb 100% <100%> (ø) ✅
...ts/app/forms/decidim/budgets/admin/project_form.rb 100% <100%> (ø) ✅
...m-core/app/models/decidim/participatory_process.rb 100% <100%> (ø) ✅
.../forms/decidim/admin/participatory_process_form.rb 100% <100%> (ø) ✅
...pp/controllers/decidim/features/base_controller.rb 100% <100%> (ø) ✅
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3f983f...80a799d. Read the comment docs.

@oriolgual
Copy link
Contributor

@mrcasals this also applies to projects (Budgets)

@mrcasals
Copy link
Contributor Author

@oriolgual thanks, description updated!

oriolgual
oriolgual suggested changes Feb 23, 2017
View reviewed changes
decidim-core/db/migrate/20170221094835_add_scopes_to_processes.rb Outdated
@@ -0,0 +1,5 @@
class AddScopesToProcesses < ActiveRecord::Migration[5.0]
def change
add_column :decidim_participatory_processes, :scope_ids, :integer, array: true, default: []
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I hate HABTM, and we won't need to find the relation from the other end (eg scope.participatory_processes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did the same in SurveyResults in L'Hospitalet BTW

Copy link
Contributor

@oriolgual oriolgual Feb 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

But that shouldn't be a HABTM, it's a belong_to (from process to scope)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oriolgual didn't we agree to support multiple scopes for a process in the backend, but in fact only relate to one by now? I was sure we agreed to this, but I might have dreamt it :/

Copy link
Contributor

Choose a reason for hiding this comment

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

LOL, I remember the conversation and I think I was against this. I think it's better to don't allow multiple scopes since this opens too many options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, it's an easy change

decidim-core/app/models/decidim/participatory_process.rb Outdated
@@ -32,7 +32,7 @@ class ParticipatoryProcess < ApplicationRecord

has_many :features, foreign_key: "decidim_participatory_process_id"

attr_readonly :active_step
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering too, actually

@mrcasals mrcasals force-pushed the relate_process_with_scopes branch from 849edcc to 2abea0b Compare February 27, 2017 08:28
@mention-bot
Copy link

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

@mrcasals mrcasals force-pushed the relate_process_with_scopes branch from f152b35 to b278c39 Compare February 28, 2017 11:29
oriolgual
oriolgual suggested changes Feb 28, 2017
View reviewed changes
decidim-admin/app/commands/decidim/admin/create_participatory_process.rb
@@ -46,6 +46,8 @@ def create_participatory_process
hero_image: form.hero_image,
banner_image: form.banner_image,
promoted: form.promoted,
meta_scope: form.meta_scope,
Copy link
Contributor

Choose a reason for hiding this comment

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

You should rename the keys at the locales files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, true

decidim-core/lib/decidim/core/test/factories.rb
@@ -58,7 +58,7 @@
banner_image { test_file("city2.jpeg", "image/jpeg") }
published_at { Time.current }
organization
scope { Decidim::Faker::Localized.word }
meta_scope { Decidim::Faker::Localized.word }
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we add the scope too? Or a trait

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A trait it is.

@mrcasals mrcasals merged commit e8cfa7a into master Feb 28, 2017
@mrcasals mrcasals deleted the relate_process_with_scopes branch February 28, 2017 14:09
Quentinchampenois added a commit to Quentinchampenois/decidim that referenced this pull request Nov 12, 2020
@Quentinchampenois
disable admin TOS for iniatives authors (decidim#1017)
6c71d85
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beagleknight beagleknight beagleknight approved these changes

@oriolgual oriolgual oriolgual approved these changes

Assignees

@mrcasals mrcasals

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mrcasals @codecov-io @oriolgual @mention-bot @beagleknight