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.

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

Validate presence 'poll_id' attribute on Poll::Question model #1868

Conversation

aitbw
Copy link
Collaborator

@aitbw aitbw commented Sep 14, 2017

Where

Fixes #1831

What

Validates presence of poll_id attribute on Poll::Question model, which previously allowed a question to be created with no association to a poll

How

Added the necessary validation on the Poll::Question model

Screenshots

screenshot-2017-9-14 admin
Here's how the form looks when a poll is not selected

Test

Added tests for both valid and invalid scenarios, also did a small refactor to improve DRYness for the related spec

Copy link
Collaborator

@bertocq bertocq left a comment

Choose a reason for hiding this comment

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

Nice spec cleanup!

Copy link
Contributor

@MariaCheca MariaCheca left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@bertocq
Copy link
Collaborator

bertocq commented Sep 14, 2017

@aitbw only one problem with tests, I've run three times locally spec/features/admin/poll/polls_spec.rb and it always fails at:

  1) Admin polls Questions Poll show Add question to poll
     Failure/Error: question = create(:poll_question, poll: nil, title: 'Should we rebuild the city?')

     ActiveRecord::RecordInvalid:
       Validation failed: Poll can't be blank
     # ./spec/features/admin/poll/polls_spec.rb:186:in `block (4 levels) in <top (required)>'

Because of the poll: nil at https://github.com/wairbut-m2c/consul/blob/aperez-validate-poll-question-is-selected/spec/features/admin/poll/polls_spec.rb#L186

@aitbw
Copy link
Collaborator Author

aitbw commented Sep 14, 2017

I'll fix it right away. Should I fix the issue, rebase and update the PR or a new commit would be better, @bertocq?

@bertocq
Copy link
Collaborator

bertocq commented Sep 14, 2017

@aitbw I think in the same commit that you were expecting poll not to be nil that fix could be as well in context of the rest of the commit, right? "Same commit" I mean :)

Fixes consuldemocracy#1831

On branch aperez-validate-poll-question-is-selected
  Changes to be committed:
    modified:   app/models/poll/question.rb
    modified:   spec/models/poll/question_spec.rb
    modified:   spec/features/admin/poll/polls_spec.rb
@zamuro zamuro force-pushed the aperez-validate-poll-question-is-selected branch from a2082d6 to 5b5f6cf Compare September 14, 2017 15:58
@aitbw
Copy link
Collaborator Author

aitbw commented Sep 14, 2017

The failing spec is fixed, so you shouldn't have any problems with it, @bertocq :D

Copy link
Member

@voodoorai2000 voodoorai2000 left a comment

Choose a reason for hiding this comment

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

👍

On branch aperez-validate-poll-question-is-selected
  Changes to be committed:
    modified:   spec/models/poll/question_spec.rb
@zamuro zamuro force-pushed the aperez-validate-poll-question-is-selected branch from 5b5f6cf to 15ccef8 Compare September 19, 2017 11:08
@bertocq bertocq merged commit 5d6c4f0 into consuldemocracy:master Sep 19, 2017
@aitbw aitbw deleted the aperez-validate-poll-question-is-selected branch January 15, 2018 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants