Remove results and add accountability to decidim #1926

beagleknight · 2017-09-25T14:54:24Z

🎩 What? Why?

Remove decidim-results from the decidim gem and includes decidim-assemblies and decidim-accountability.

📌 Related Issues

Closes Decidim-accountability Core Integration #1823

📋 Subtasks

📷 Screenshots (optional)

N/A

👻 GIF

Nope.

codecov · 2017-09-25T14:54:31Z

Codecov Report

Merging #1926 into master will decrease coverage by 0.01%.
The diff coverage is 98.38%.

@@            Coverage Diff             @@
##           master    #1926      +/-   ##
==========================================
- Coverage   98.56%   98.55%   -0.02%     
==========================================
  Files        1149     1167      +18     
  Lines       25941    26499     +558     
==========================================
+ Hits        25570    26117     +547     
- Misses        371      382      +11

josepjaume · 2017-09-26T08:00:48Z

@beagleknight what deprecation strategy will we use? I'd warn the users and provide a script (pure SQL?) to migrate from plain results to accountability results. Maybe even just a migration?

beagleknight · 2017-09-26T08:17:25Z

@josepjaume I think it's better to provide some kind of documentation like we did with the followable resources. I agree using plain SQL to migrate those results. We can use this as a starting point:
https://github.com/AjuntamentdeBarcelona/decidim-barcelona/blob/master/lib/tasks/migrate_results_to_accountability.rake

josepjaume · 2017-09-26T10:24:52Z

Oh, nice @beagleknight !

mrcasals · 2017-09-29T10:32:57Z

decidim-accountability/app/controllers/decidim/accountability/results_controller.rb

@@ -9,8 +9,7 @@ class ResultsController < Decidim::Accountability::ApplicationController

      helper_method :results, :result, :stats_calculator, :first_class_categories, :category, :progress_calculator, :count_calculator, :current_scope, :template_texts

-      def home
-      end
+      def home; end


This is not needed.

mrcasals · 2017-09-29T10:33:36Z

decidim-accountability/app/helpers/decidim/accountability/application_helper.rb

@@ -19,7 +19,7 @@ def display_count(count)
      end

      def active_class_if_current(scope)
-        %Q{class=active} if scope.to_s == current_scope.to_s
+        %(class=active) if scope.to_s == current_scope.to_s


use normal string?

mrcasals · 2017-09-29T10:36:41Z

decidim-accountability/app/commands/decidim/accountability/admin/update_result.rb

@@ -26,7 +26,7 @@ def call

          transaction do
            update_result
-            result.touch if form_proposal_ids != current_proposal_ids
+            result.save! if form_proposal_ids != current_proposal_ids


unneeded? (update_result is already saving the result)

deivid-rodriguez · 2017-09-29T17:02:04Z

decidim-accountability/spec/shared/manage_results_examples.rb

@@ -1,6 +1,7 @@
+# -*- coding: utf-8 -*-


deivid-rodriguez · 2017-09-29T17:02:40Z

decidim-accountability/spec/shared/manage_results_examples.rb

 # frozen_string_literal: true

-shared_examples "manage results" do
+RSpec.shared_examples "manage results" do


We don't use RSpec.shared_examples but just shared_examples.

deivid-rodriguez · 2017-09-29T17:03:13Z

decidim-accountability/spec/shared/manage_results_examples.rb

@@ -91,7 +93,7 @@
      end

      within "table" do
-        expect(page).to have_no_content(translated(result2.title))
+        expect(page).not_to have_content(translated(result2.title))


Better the previous way since it waits for the content to disappear.

deivid-rodriguez · 2017-09-29T17:03:40Z

decidim-accountability/spec/shared/manage_statuses_examples.rb

@@ -0,0 +1,89 @@
+# -*- coding: utf-8 -*-


RuboCop.... 🤔

deivid-rodriguez · 2017-09-29T17:05:02Z

decidim-core/decidim-core.gemspec

@@ -57,5 +57,4 @@ Gem::Specification.new do |s|
  s.add_development_dependency "decidim-participatory_processes", Decidim.version


Should we add decidim-assemblies and/or decidim-accountability here?

To be honest, decidim-core shouldn't depend on neither of them, and we should probably remove all the other dependencies as well, except for decidim-dev.

Seems sensible.

In reality, decidim-core might not work by itself. But it's a first step to detect unnecessary coupling.

deivid-rodriguez · 2017-09-29T17:05:10Z

decidim-core/spec/factories.rb

@@ -5,4 +5,4 @@
 require "decidim/participatory_processes/test/factories"
 require "decidim/proposals/test/factories"
 require "decidim/meetings/test/factories"
-require "decidim/results/test/factories"
+# require "decidim/results/test/factories"


Remove this, I guess.

deivid-rodriguez · 2017-09-29T17:05:42Z

decidim-meetings/spec/factories.rb

@@ -4,7 +4,7 @@
 require "decidim/participatory_processes/test/factories"
 require "decidim/assemblies/test/factories"
 require "decidim/proposals/test/factories"
-require "decidim/results/test/factories"
+# require "decidim/results/test/factories"


Remove this I guess.

deivid-rodriguez · 2017-09-29T17:05:59Z

decidim-proposals/spec/factories.rb

@@ -5,7 +5,7 @@
 require "decidim/assemblies/test/factories"
 require "decidim/comments/test/factories"
 require "decidim/meetings/test/factories"
-require "decidim/results/test/factories"
+# require "decidim/results/test/factories"


Remove this I guess.

deivid-rodriguez · 2017-09-29T17:07:45Z

decidim-core/config/locales/en.yml

@@ -157,6 +157,7 @@ en:
    filters:
      linked_classes:
        all: All
+        dummyresource: Dummy resources


Is this key correctly not including a hyphen? Nice decoupling of the decidim-results dependency for tests, by the way.

Looks like it is!

deivid-rodriguez · 2017-09-29T17:14:08Z

@beagleknight Awesome and tough work! 💪

I did a bit of CI-style reviewing, quite a lot of comments but easy to fix!

mrcasals

Can we address the isssues found by @deivid-rodriguez please?

This reverts commit 337d857.

josepjaume · 2017-10-16T11:08:57Z

@mrcasals @deivid-rodriguez can I ask for a re-review?

mrcasals

Looks good to me!

beagleknight self-assigned this Sep 25, 2017

ghost added the in-progress label Sep 26, 2017

beagleknight mentioned this pull request Sep 27, 2017

Deprecate this repository decidim-archive/decidim-accountability#64

Open

beagleknight added in-review and removed in-progress labels Sep 28, 2017

beagleknight force-pushed the decidim-gem-changes branch 5 times, most recently from da09af4 to d4fd753 Compare September 28, 2017 13:02

beagleknight added in-progress in-review and removed in-review in-progress labels Sep 28, 2017

beagleknight force-pushed the decidim-gem-changes branch from 559fba9 to 98acb07 Compare September 29, 2017 07:43

mrcasals reviewed Sep 29, 2017

View reviewed changes

mrcasals previously approved these changes Sep 29, 2017

View reviewed changes

beagleknight added 7 commits September 29, 2017 14:12

Remove decidim-results files

098b276

Decidim results is no longer a dependency of decidim-core

7c4ece1

Remove results dependency from proposals and meetings

d41681c

Sanitize locales

56f9ace

Include decidim-assemblies inside decidim gem

337d857

Add decidim-accountability code

4990a92

Accountability commmands specs revision

25a69b6

deivid-rodriguez reviewed Sep 29, 2017

View reviewed changes

mrcasals suggested changes Oct 2, 2017

View reviewed changes

josepjaume changed the title ~~Remove results and add assemblies and accountability to decidim~~ Remove results and add accountability to decidim Oct 4, 2017

josepjaume added 3 commits October 6, 2017 12:05

Revert "Include decidim-assemblies inside decidim gem"

919c811

This reverts commit 337d857.

Merge remote-tracking branch 'origin/master' into decidim-gem-changes

79ce6ff

Improve code

c42fdef

ghost assigned josepjaume Oct 6, 2017

josepjaume added 9 commits October 6, 2017 15:25

Fix build

2161c3f

Fix translation

55906e5

Merge remote-tracking branch 'origin/master' into decidim-gem-changes

efbef4b

Merge remote-tracking branch 'origin/master' into decidim-gem-changes

a96fedc

Fix job enqueueing

347b017

Fix stuff

4ab5653

Fix job enququeing

895bbb9

Fix accountability

c4e14b5

Fix tests

c6d1aac

mrcasals approved these changes Oct 16, 2017

View reviewed changes

josepjaume merged commit 6564fa6 into master Oct 16, 2017

josepjaume deleted the decidim-gem-changes branch October 16, 2017 12:19

ghost removed the in-review label Oct 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove results and add accountability to decidim #1926

Remove results and add accountability to decidim #1926

beagleknight commented Sep 25, 2017 •

edited

codecov bot commented Sep 25, 2017 •

edited

josepjaume commented Sep 26, 2017

beagleknight commented Sep 26, 2017

josepjaume commented Sep 26, 2017

mrcasals Sep 29, 2017

mrcasals Sep 29, 2017

mrcasals Sep 29, 2017

deivid-rodriguez Sep 29, 2017

deivid-rodriguez Sep 29, 2017

deivid-rodriguez Sep 29, 2017

deivid-rodriguez Sep 29, 2017

deivid-rodriguez Sep 29, 2017

josepjaume Oct 6, 2017

deivid-rodriguez Oct 6, 2017

josepjaume Oct 6, 2017

deivid-rodriguez Sep 29, 2017

deivid-rodriguez Sep 29, 2017

deivid-rodriguez Sep 29, 2017

deivid-rodriguez Sep 29, 2017

josepjaume Oct 16, 2017

deivid-rodriguez commented Sep 29, 2017

mrcasals left a comment

josepjaume commented Oct 16, 2017

mrcasals left a comment

		@@ -57,5 +57,4 @@ Gem::Specification.new do \|s\|
		s.add_development_dependency "decidim-participatory_processes", Decidim.version

Remove results and add accountability to decidim #1926

Remove results and add accountability to decidim #1926

Conversation

beagleknight commented Sep 25, 2017 • edited

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

👻 GIF

codecov bot commented Sep 25, 2017 • edited

Codecov Report

josepjaume commented Sep 26, 2017

beagleknight commented Sep 26, 2017

josepjaume commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivid-rodriguez commented Sep 29, 2017

mrcasals left a comment

Choose a reason for hiding this comment

josepjaume commented Oct 16, 2017

mrcasals left a comment

Choose a reason for hiding this comment

beagleknight commented Sep 25, 2017 •

edited

codecov bot commented Sep 25, 2017 •

edited