Iagirre question ansers ordenable #2037
Conversation
app/models/poll/question/answer.rb
Outdated
| ordered_array.each_with_index do |answer_id, order| | ||
| answer = find(answer_id) | ||
| answer.update_attribute(:given_order, (order + 1)) | ||
| answer.save |
There was a problem hiding this comment.
update_attribute saves the record already. I would refactor this 3 lines into just find(answer_id).update_attribute(:given_order, (order + 1))
app/models/poll/question/answer.rb
Outdated
| end | ||
|
|
||
| def set_order | ||
| last_position = Poll::Question::Answer.where(question_id: question_id).maximum("given_order") || 0 |
There was a problem hiding this comment.
I think we don't need Poll::Question::Answer self reference
| @@ -0,0 +1,5 @@ | |||
| class AddGivenOrderToPollQuestionAnswers < ActiveRecord::Migration | |||
| def change | |||
| add_column :poll_question_answers, :given_order, :integer | |||
There was a problem hiding this comment.
Nice one! I started a quick refactor to add this but found myself in the pitfall of creating a column named order that raised many issues 🙈 this name is better
Although by reading all code seems to me that the given_order values will always start from 1, right? Would it make sense to set a default: 1 just as a reminder on the code and as well as a security measure?
Also I think if given_order is an attribute needed there should be a presence validation on the model (as well as one that checks its unique for the question_id (only one Answer on a particular order for its Question)
| @@ -24,9 +24,27 @@ | |||
| expect(page).to have_content(description) | |||
| end | |||
|
|
|||
| scenario 'Create second answer and place after the first one' do | |||
| @@ -9,6 +9,7 @@ def new | |||
|
|
|||
| def create | |||
| @answer = ::Poll::Question::Answer.new(answer_params) | |||
| @answer.set_order | |||
There was a problem hiding this comment.
I think since there might be other ways to create an answer (think through rake task, console, etc...) and a value at the given_order attribute is always needed. This set_order method shouldn't be called here, but on a before_create on the Poll Question Answer model
|
Really nice the default order in the relation between questions and answers 👏 cool trick :) There's an additional place where we'll need answers ordered where that default order won't help sadly :( https://github.com/consul/consul/blob/master/app/views/polls/show.html.erb#L82 on Madrid's fork we did this "hack" sorting in Ruby but that's not really a nice way to do it either :D https://github.com/ayuntamientomadrid/consul/blob/master/app/views/polls/show.html.erb#L98. I think the trick here would be to instead of We can fix this in a second PR if you think it will be too much for this one (tell me and I'll open an Issue) |
zamuro
force-pushed
the
iagirre-question-ansers-ordenable
branch
from
1d6644b
to
644d09e
Compare
| @@ -10,8 +10,26 @@ class Poll::Question::Answer < ActiveRecord::Base | |||
| has_many :videos, class_name: 'Poll::Question::Answer::Video' | |||
|
|
|||
| validates :title, presence: true | |||
| validates :given_order, presence: true, uniqueness: { scope: :question_id } | |||
|
|
|||
| before_validation :set_order, on: :create | |||
There was a problem hiding this comment.
I finally used before_validation on: :create to set the order, because if it was set before_create, the validation run before all of that (so, before set_order method) and it failled.
For the 1st there was no problem, but for the 2nd it couldn't pass the validation and it couldn't create the record.
| sequence(:title) { |n| "Question title #{n}" } | ||
| sequence(:description) { |n| "Question description #{n}" } | ||
| sequence(:title) { |n| "Answer title #{n}" } | ||
| sequence(:description) { |n| "Answer description #{n}" } |
There was a problem hiding this comment.
I changed the name for the poll_question_answer because it made conflicts with the poll_question name when running specs (they had the same name, but, as I understand, the answers should be named 'Answer title X' instead of 'Question title X').
…esults_links Don't show links to disabled budget results
Where
What
Make the answers sortable from the admin page, so that they appear in the given order when user enters the poll.
How
given_orderorderto the relation between question and answers, so that every time we usequestion.question_answersthey appear ordered bygiven_orderfield.Screenshots
Drag and drop to change the order
The order has been changed
User sees the new order
Test
I increased the test coverage to test if:
Deployment
Not needed.
Warnings
There is already an answer with given_order = 1. The next will be given_order = 2, and so on. If it's the first one, it will be given_order = 1.