Add position to steps by mrcasals · Pull Request #137 · decidim/decidim

mrcasals · 2016-10-20T14:28:59Z

🎩 What? Why?

We need to reorder steps. This can be done now with a drag&drop of table rows.

📋 Subtasks

👻 GIF

codecov-io · 2016-10-20T14:34:32Z

Current coverage is 97.70% (diff: 100%)

Merging #137 into master will increase coverage by 0.04%

@@             master       #137   diff @@
==========================================
  Files           154        156     +2   
  Lines          2257       2348    +91   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2204       2294    +90   
- Misses           53         54     +1   
  Partials          0          0

Powered by Codecov. Last update 4c266f1...cf9cb74

oriolgual · 2016-10-21T14:27:37Z

decidim-admin/app/commands/decidim/admin/reorder_participatory_process_steps.rb

Wrong docs?

oriolgual · 2016-10-21T14:28:04Z

decidim-admin/app/commands/decidim/admin/reorder_participatory_process_steps.rb

Either the docs or the var name are wrong.

I don't get the comment. Names match :/

I was confused because the var name says order_stirng, but then it's an Array of IDs, I didn't get it was a JSON string or similar (maybe it could be added)

order_string should be something like: "[2, 1, 3, 5, 7, 9, 8]", so a String representing an Array of IDs. Each element of the array is an ID of a step, and its index is the position field value.

This is weird. Coercing that string into an array should be the transport's responsibility (ujs via the controller).

Makes sense

I didn't want to rescue a JSON parsing in the controller but yeah, feels weird to me too. Changing it!

oriolgual · 2016-10-21T14:28:23Z

decidim-admin/app/commands/decidim/admin/reorder_participatory_process_steps.rb

Missing new line after return

oriolgual · 2016-10-21T14:28:31Z

decidim-admin/app/commands/decidim/admin/reorder_participatory_process_steps.rb

Missing new line before return

oriolgual · 2016-10-21T14:29:27Z

decidim-admin/app/commands/decidim/admin/reorder_participatory_process_steps.rb

I'd move this to a method (order). This way you can do return broadcast(:invalid) unless order and encapsulate all the JSON and presence logic in a method.

oriolgual · 2016-10-21T14:31:34Z

decidim-admin/app/commands/decidim/admin/reorder_participatory_process_steps.rb

I don't understand this method. Also:

order.each_with_index.inject({}) do |data, (id, index)| data.update(id: { position: index }) end

Your version does an update for every single element in the collection. My version uses a single update for all the collection:

https://github.com/rails/rails/blob/a419a4d9ade48e777166ff956dd7bb24e37b2181/activerecord/lib/active_record/relation.rb#L363

Edit: OK checking the code it actually does an update for every element in the array :/

That's why I said I didn't understand, what is this doing?

oriolgual · 2016-10-21T14:34:48Z

decidim-admin/app/controllers/decidim/admin/participatory_process_steps_controller.rb

Not sure if we should create another controller or not.

Probably, with a create action. Will do.

oriolgual · 2016-10-21T14:35:04Z

decidim-admin/app/controllers/decidim/admin/participatory_process_steps_controller.rb

What about on(:valid) ?

We don't do anything. If I put a render or a redirect here, then page refreshes and misses all the point.

We could render some JS here and then do something in the layout, but if we need to show a success message every time the user drags and drops... I think it's too much.

What you should do is put a spinner when you start the request, and delete that spinner when it finishes. But hey, ujs is a hack, this won't last long as we'll replace this with React+GraphQL (already working on it).

oriolgual · 2016-10-21T14:35:50Z

decidim-admin/app/views/decidim/admin/participatory_processes/show.html.erb

Should we move this to a JS file?

I won't accept any JS inside html templates... 👎

oriolgual · 2016-10-21T14:37:15Z

decidim-core/db/migrate/20161020080756_add_position_to_steps.rb

We should add some validations to the model (numericality, minimum, maximum, etc). Should we set the position on creation or do we allow nil values?

Right now, steps are created with position: nil, thus positioning themselves in the beginning of the list. I don't know if this is the expected or we'd rather have them in the end. If we want to have them in the end, I can create the steps with a really high position (position: 9999 for example). Once the user reorders this, position will go back to a "normal" number

We can add validations allowing blank values. Regarding default values, I'd expect new steps to be added to the end, not the begining. Maybe the default value can be the previous + 1 (resort solves this 😝)

Validations are in the model. As for the default value, I don't know if resort would be too much for just this (could be a before_save method or similar), and I don't know if I'll need other resort features by now :/

Agreed offline to just add a before_create action to set the value when it's nil.

beagleknight · 2016-10-24T07:39:14Z

I recommend adding a javascript linter before it is too late. I recommend eslint (https://github.com/appfolio/eslint-rails) and adding this as a CI step.

mrcasals · 2016-10-24T07:59:14Z

decidim-admin/app/controllers/decidim/admin/participatory_process_step_ordering_controller.rb

See previous discussion in https://github.com/codegram/decidim/pull/137#discussion_r84491443

I'd go with "No news good news" for now.

josepjaume · 2016-10-24T14:12:55Z

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

Protip: You can run rubocop -a and will probably auto-fix this.

josepjaume · 2016-10-24T18:22:57Z

decidim-admin/app/commands/decidim/admin/reorder_participatory_process_steps.rb

This is weird. Coercing that string into an array should be the transport's responsibility (ujs via the controller).

oriolgual · 2016-10-25T07:49:19Z

decidim-admin/app/controllers/decidim/admin/participatory_process_step_ordering_controller.rb

+      def create
+        authorize ParticipatoryProcessStep
+
+        byebug


oriolgual · 2016-10-25T07:50:47Z

decidim-admin/spec/commands/reorder_participatory_process_steps_spec.rb

+      expect { subject.call }.to broadcast(:ok)
+    end
+
+    it "deactivates it" do


wrong text?

oriolgual · 2016-10-25T07:52:18Z

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

+      return if position.present?
+      return self.position = 0 if participatory_process.steps.empty?
+
+      self.position = participatory_process.steps.last.position + 1


What about participatory_process.steps.pluck(:position).last ? (I know we'll load more rows, but we'll load less data, not sure which is best though.)

oriolgual · 2016-10-25T07:54:21Z

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

+    # manually, it will be set to 0, and when it is not the only one it will be
+    # set to the last step's position + 1.
+    #
+    # Note: This allows manual positioning, so it can cause different steps to


Shouldn't we add a unique validation then? (And also to the index)

oriolgual · 2016-10-25T07:54:52Z

decidim-core/db/migrate/20161020080756_add_position_to_steps.rb

+class AddPositionToSteps < ActiveRecord::Migration[5.0]
+  def change
+    add_column :decidim_participatory_process_steps, :position, :integer
+    add_index :decidim_participatory_process_steps, [:decidim_participatory_process_id, :position], order: { position: :desc }, name: "index_check_position_for_steps"


This order doesn't match the one at the relation scope

oriolgual suggested changes Oct 21, 2016

View reviewed changes

mrcasals commented Oct 24, 2016

View reviewed changes

oriolgual approved these changes Oct 24, 2016

View reviewed changes

josepjaume reviewed Oct 24, 2016

View reviewed changes

mrcasals force-pushed the add_position_to_steps branch from 9593bb6 to d85c3a3 Compare October 24, 2016 14:14

josepjaume suggested changes Oct 24, 2016

View reviewed changes

mrcasals added 12 commits October 25, 2016 09:09

Add position

3d38869

Reorder steps in the forntend

19a08a1

Move logic to a command

ae7d996

I18n and auto save changes after reordering

80416e0

Add specs for command

a378642

Fix style

b4579d6

Move action to a separate controller

5641dba

Move JS code to a separate file

717f85a

Fix styles

bb3105d

Set position automatically when not set manually

31e9aee

Don't parse the string, send an array directly

bcb6bd6

Add index to steps position

1cf6bab

mrcasals force-pushed the add_position_to_steps branch from d85c3a3 to 1cf6bab Compare October 25, 2016 07:46

oriolgual reviewed Oct 25, 2016

View reviewed changes

mrcasals added 2 commits October 25, 2016 09:51

Improve style & ignore files in HoundCI

df77879

HoundCI: Ignore all JS vendor files

a146543

oriolgual suggested changes Oct 25, 2016

View reviewed changes

mrcasals added 5 commits October 25, 2016 10:20

Fix code style

42b7aa1

Add missing spec

8ace267

Fix JS code style

8eccfa7

Improve docs

856b322

Improve params handling

cf9cb74

oriolgual approved these changes Oct 25, 2016

View reviewed changes

josepjaume approved these changes Oct 25, 2016

View reviewed changes

mrcasals merged commit 2dc8195 into master Oct 25, 2016

mrcasals deleted the add_position_to_steps branch October 25, 2016 10:46

beagleknight mentioned this pull request May 18, 2017

Survey questions can be sorted using drag and drop #1373

Merged

Uh oh!

Conversation

mrcasals commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📋 Subtasks

👻 GIF

Uh oh!

codecov-io commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 97.70% (diff: 100%)

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!

mrcasals Oct 24, 2016 • 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!

beagleknight commented Oct 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oriolgual Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mrcasals commented Oct 20, 2016 •

edited

Loading

codecov-io commented Oct 20, 2016 •

edited

Loading

mrcasals Oct 24, 2016 •

edited

Loading

oriolgual Oct 24, 2016 •

edited

Loading