Survey-style poll prototype #5540
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were using three different variable names for the same thing, in consecutite tests.
Not sure about when we stopped needing them, but all usages of require_dependency definitely become obsolete after we started using Zeitwerk in commit 5f24ee9. We're also going to rename `Poll::Question::Answer` to `Poll::Question::Option`, so the conflict of having two `Answer` classes, that made us add this code in the first place, will not even exist.
Just like we do every else (even on that very same file), we use the method instead of the instance variable.
Having a class named `Poll::Question::Answer` and another class named `Poll::Answer` was so confusing that no developer working on the project has ever been capable of remembering which is which for more than a few seconds. Furthermore, we're planning to add open answers to polls, and we might add a reference from the `poll_answers` table to the `poll_question_answers` table to property differentiate between open answers and closed answers. Having yet another thing named answer would be more than what our brains can handle (we know it because we did this once in a prototype). So we're renaming `Poll::Question::Answer` to `Poll::Question::Option`. Hopefully that'll make it easier to remember. The name is also (more or less) consistent with the `Legislation::QuestionOption` class, which is similar. We aren't changing the table or columns names for now in order to avoid possible issues when upgrading (old code running with the new database tables/columns after running the migrations but before deployment has finished, for instance). We might do it in the future. At first I tried not to change the internationalization keys either so existing translations would still be valid. However, since we have to change the keys in `activerecord.yml`, since it uses class names and we're changing the class names, we're changing them everywhere for consistency. Note that it isn't clear whether we should use `option` or `question_option` in some cases. In order to keep things simple, we're using `option` where we were using `answer` and `question_option` where we were using `question_answer`.
We were getting `undefined method `lock!' for nil:NilClass` when the question allowed multiple answers.
Since we were only using it in one place, it made the code harder to follow. We'll extract it again if we ever find a way to reuse it.
We were checking we didn't have more votes than allowed in the case of questions with multiple answers, but we weren't checking it in the case of questions with a single answer. This made it possible to create more than one answer to the same question. This could happen because the method `find_or_initialize_user_answer` might initialize two answers in different threads, due to a race condition.
That way the record is only locked while necessary.
Note that, when taking votes from an erased user, since poll answers don't belong to poll voters, we were not migrating them in the `take_votes_from` method (and we aren't migrating them now either).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,2 @@ | |||
| .poll-form { | |||
There was a problem hiding this comment.
Unexpected empty block (block-no-empty)
javierm
force-pushed
the
poll_form
branch
2 times, most recently
from
a5dfc35
to
e1457e9
Compare
|
|
||
| user.with_lock do | ||
| questions.each do |question| | ||
| question.answers.where(author: user).each(&:destroy!) |
There was a problem hiding this comment.
Rails/FindEach: Use find_each instead of each. (https://rails.rubystyle.guide#find-each)
Suggested change
| question.answers.where(author: user).each(&:destroy!) | |
| question.answers.where(author: user).find_each(&:destroy!) |
javierm
force-pushed
the
add_option_id_to_poll_answers
branch
2 times, most recently
from
60014d9
to
9a004e6
Compare
Note that, since poll answers belong to a user and not to a voter, we aren't doing anything regarding poll answers. This is a separate topic that might be dealt with in a separate pull request. Also note that, since there are no records belonging to poll voters, and poll voters don't use `acts_as_paranoia` and don't have any callbacks on destroy, it doesn't really matter whether we call `destroy!` or `delete`. We're using `delete` so there are no unintended side-effects that might affect voters with the same `user_id` and `poll_id` on Consul Democracy installations customizing this behavior.
It might be interesting in some cases to check the information related to those records.
The routes for poll questions were accidentally deleted in commit 5bb831e when deleting the `:show` action, and restored in commit 9871503. However, the deleted code was: ``` resources :questions, only: [:show], controller: 'polls/questions' (...) ``` While the restored code was: ``` resources :questions, controller: 'polls/questions' (...) ``` Meaning we forgot to add the `only: []` option when restoring the routes. We also forgot to remove the `before_action` code when deleting the `:show` action, so we're removing it now.
It was confusing to have the action to create an answer in `QuestionsController#answer` while the action to destroy it was `AnswersController#destroy`.
javierm
force-pushed
the
add_option_id_to_poll_answers
branch
from
9a004e6
to
6424024
Compare
javierm
force-pushed
the
add_option_id_to_poll_answers
branch
7 times, most recently
from
3c2920c
to
9376db4
Compare
Until now, we've stored the text of the answer somebody replied to. The idea was to handle the scenarios where the user voters for an option but then that option is deleted and restored, or the texts of the options are accidentally edited and so the option "Yes" is now "Now" and vice versa. However, since commit 3a6e99c, options can no longer be edited once the poll starts, so there's no risk of the option changing once somebody has voted. This means we can now store the ID of the option that has been voted. That'll also help us deal with a bug introduced int 673ec07, since answers in different locales are not counted as the same answer. Note we aren't dealing with this bug right now. We're still keeping (and storing) the answer as well. There are two reasons for that. First, we might add an "open answer" type of questions in the future and use this column for it. Second, we've still got logic depending on the answer, and we need to be careful when changing it because there are existing installations where the answer is present but the option_id is not. Note that we're using `dependent: nullify`. The reasoning is that, since we're storing both the option_id and the answer text, we can still use the answer text when removing the option. In practice, this won't matter much, though, since we've got a validation rule that makes it impossible to destroy options once the poll has started. Also note we're still allowing duplicate records when the option is nil. We need to do that until we've removed every duplicate record in the database.
Note: to avoid confusion, "answer" will mean a row in the poll_answers table and "choice" will mean whatever is in the "answer" column of that table (I'm applying the same convention in the code of the task). In order make this task perform reasonably on installations with millions of votes, we're using `update_all` to update all the answers with the same choice at once. In order to do that, we first need to check the existing choices and what are the possible option_ids for those choices. Note that, in order for this task to work, we need to remote the duplicate answers first. Otherwise, we will run into a RecordNotUnique exception when trying to add the same option_id to two duplicate answers. So we're making this task depend on the one that removes duplicate answers. That means we no longer need to specify the task to remove duplicate answers in the release tasks; it will automatically be executed when running the task to add an option_id.
javierm
force-pushed
the
add_option_id_to_poll_answers
branch
from
9376db4
to
237e12e
Compare
This code isn't used since commit 5fdbc7b.
This is consistent with the way we've got partials to render debates, proposals and legislation processes on their index pages. Note that, while adding the tests for the status icon, we're keeping one system test because it also tests the process of voting. We're adding a new, similar component test, where the voter is created in the database, so all possible statuses are tested in the component.
We were using a redundant `elsif` instead of an `else`. We were also using a negative condition in the main `if`, which made the code a bit harder to read. Since we usually use `current_user` instead of `user_signed_in?`, we're also changing that for consistency.Extract component to render the status of a poll
We accidentally introduced a typo in commit f497227 which caused the dates to be rendered outside the element where the dates styles are applied.
The rows in these tables were using the styles from the `.poll` selector, and the `position: relative` property defined there caused the inner borders to disappear. So we're adding the `public` class to the selector; this way, it doesn't affect elements in the admin section. Even though it's only necessary to add the `.public` prefix to the `.poll` selector in one place in order to fix this issue, we're doing it everywhere for consistency.
When this code was added, in commit 1a20a3c, we had no validation rules checking the presence of the start and end dates of a poll. Now we do, so we don't have to check this condition in the view.
Since we were using `I18n.t`, our monkey-patch of the `t` helper wasn't being applied. Note that polls with no start or end date are invalid, so I don't think it makes sense to write a test for the `polls.no_dates` scenario. I wonder whether we should remove the code for this condition.
Name: because we already have Ballot, and because Vote might be mistaken with other vote classes
javierm
force-pushed
the
add_option_id_to_poll_answers
branch
5 times, most recently
from
df1b744
to
091b247
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Objectives