Conversation
Codecov Report
@@ Coverage Diff @@
## master #1342 +/- ##
=========================================
- Coverage 95.48% 94.98% -0.5%
=========================================
Files 439 466 +27
Lines 7574 7980 +406
=========================================
+ Hits 7232 7580 +348
- Misses 342 400 +58 |
37fb172 to
e1f6ec3
Compare
|
|
||
| @survey.update_attributes!(attributes) | ||
|
|
||
| unless @survey.published? |
There was a problem hiding this comment.
Use a guard clause instead of wrapping the code inside a conditional expression.
| factory :survey_answer, class: Decidim::Surveys::SurveyAnswer do | ||
| body { Decidim::Faker::Localized.sentence } | ||
| survey | ||
| question { create(:survey_question, survey: survey )} |
There was a problem hiding this comment.
Space inside parentheses detected.
Space missing inside }.
2152877 to
ba4933a
Compare
|
|
||
| visit_feature | ||
|
|
||
| expect(page).to have_content("This is the first question"); |
There was a problem hiding this comment.
Do not use semicolons to terminate expressions.
| visit_feature_admin | ||
|
|
||
| within "form.edit_survey" do | ||
| expect(page).to have_selector('.survey-question', count: 0) |
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
| visit_feature_admin | ||
|
|
||
| within "form.edit_survey" do | ||
| expect(page).to have_selector('.survey-question', count: 1) |
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
| visit_feature_admin | ||
|
|
||
| within "form.edit_survey" do | ||
| expect(page).to have_selector('.survey-question', count: 1) |
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
| end | ||
|
|
||
| describe "when a survey has an existing question" do | ||
| let(:body) { |
There was a problem hiding this comment.
Avoid using {...} for multi-line blocks.
| it "creates a new survey with the same name as the feature" do | ||
| expect(Survey).to receive(:new).with({ | ||
| feature: feature | ||
| }).and_call_original |
There was a problem hiding this comment.
Indent the right brace the same as the first position after the preceding left parenthesis.
|
|
||
| it "creates a new survey with the same name as the feature" do | ||
| expect(Survey).to receive(:new).with({ | ||
| feature: feature |
There was a problem hiding this comment.
Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.
| end | ||
|
|
||
| it "creates a new survey with the same name as the feature" do | ||
| expect(Survey).to receive(:new).with({ |
There was a problem hiding this comment.
Redundant curly braces around a hash parameter.
| end | ||
|
|
||
| it "creates a survey answer for each question answered" do | ||
| expect { |
There was a problem hiding this comment.
Avoid using {...} for multi-line blocks.
| end | ||
|
|
||
| it "doesn't create survey answers" do | ||
| expect { |
There was a problem hiding this comment.
Avoid using {...} for multi-line blocks.
| private | ||
|
|
||
| def have_questions_before_publishing | ||
| errors.add(:published_at, :invalid) if published? && questions.length === 0 |
There was a problem hiding this comment.
Avoid the use of the case equality operator ===.
|
|
||
| private | ||
|
|
||
| def have_questions_before_publishing |
There was a problem hiding this comment.
Rename have_questions_before_publishing to questions_before_publishing?.
| private | ||
|
|
||
| def have_questions_before_publishing | ||
| errors.add(:published_at, :invalid) if published? && questions.length === 0 |
There was a problem hiding this comment.
Avoid the use of the case equality operator ===.
|
|
||
| private | ||
|
|
||
| def have_questions_before_publishing |
There was a problem hiding this comment.
Rename have_questions_before_publishing to questions_before_publishing?.
18171e5 to
d057802
Compare
| private | ||
|
|
||
| def questions_before_publishing? | ||
| errors.add(:published_at, :invalid) if published? && questions.length == 0 |
There was a problem hiding this comment.
Use questions.length.zero? instead of questions.length == 0.
Use empty? instead of length == 0.
| private | ||
|
|
||
| def questions_before_publishing? | ||
| errors.add(:published_at, :invalid) if published? && questions.length == 0 |
There was a problem hiding this comment.
Use questions.length.zero? instead of questions.length == 0.
Use empty? instead of length == 0.
|
@beagleknight, thanks for your PR! By analyzing the history of the files in this pull request, we identified @josepjaume and @deivid-rodriguez to be potential reviewers. |
decidim-surveys/README.md
Outdated
| @@ -0,0 +1,24 @@ | |||
| # Decidim::Surveys | |||
|
|
|||
| The Surveys module adds one of the main features of Decidim: allows users to contribute to a particiaptory process by creating surveys. | |||
There was a problem hiding this comment.
s/particiaptory/participatory
And the description is wrong, admins can create surveys ad users can participate in them.
| toc: @form.toc | ||
| } | ||
|
|
||
| if @form.published_at.present? |
There was a problem hiding this comment.
Can't you use Rails dirty attributes?
| return broadcast(:invalid) if @form.invalid? | ||
|
|
||
| update_survey | ||
| update_survey_questions unless survey_was_published? |
There was a problem hiding this comment.
wrap these in a transaction
| if form_question.id.present? | ||
| question = @survey.questions.where(id: form_question.id).first | ||
| if form_question.deleted? | ||
| question.destroy |
| question.update_attributes!(body: form_question.body) | ||
| end | ||
| else | ||
| @survey.questions.create(body: form_question.body) |
| @user = user | ||
| @context = context | ||
|
|
||
| # can :manage, SomeResource |
There was a problem hiding this comment.
Shouldn't we add proper permissions?
There was a problem hiding this comment.
Yeah, I totally forgot about that.
| @user = user | ||
| @context = context | ||
|
|
||
| # can :manage, SomeResource if authorized?(:some_action) |
| @user = user | ||
| @context = context | ||
|
|
||
| # can :manage, SomeResource |
| end | ||
|
|
||
| feature.on(:destroy) do |instance| | ||
| Decidim::Surveys::DestroySurvey.call(instance) do |
There was a problem hiding this comment.
We shouldn't allow destroying a survey feature if there are surveys
| Decidim::Faker::Localized.paragraph(3) | ||
| end, | ||
| toc: Decidim::Faker::Localized.wrapped("<p>", "</p>") do | ||
| Decidim::Faker::Localized.paragraph(2) |
| let(:user) { build(:user, organization: organization) } | ||
| let(:participatory_process) { build(:participatory_process, organization: organization) } | ||
| let(:surveys_feature) { build(:surveys_feature, participatory_process: participatory_process) } | ||
| let(:context) { |
There was a problem hiding this comment.
Avoid using {...} for multi-line blocks.
josepjaume
left a comment
There was a problem hiding this comment.
Nice work! I don't quite like the styles of the survey though. Maybe we can use the surveys on https://github.com/HospitaletdeLlobregat/decidim-hospitalet as an inspiration.
|
Also coverage doesn't seem that great. Are we missing critical paths? |
e55a586 to
29be570
Compare
|
I will reopen this as soon as #1364 is merged 😄 |
cd7b800 to
2255609
Compare
* Add engine skeleton * Add feature test * An admin can update a survey without questions * Admin can add n questions to a survey * Admin can edit survey questions * An admin can remove questions from surveys * Admin can publish a survey * Minor refactor * Users answer surveys * Fix minor errors * Rubocop * Add model validations * Fix minor things * Fix hound complains * Fix more complains * Fix broken specs * Add some feedback * Add some permissions * Add missing translation * Fix rubocop issues * Add tests to the CI * Fix broken test suite * Normalize locales
* Add basic drag and drop features * Refactor JS and minor adjustments * Add missing documentation * Minor label adjustments * Fix rubocop complains * Add missing index * Add missing feature test * Fix rubocop issues * Fix lint errors * Fix broken specs
* Add custom styles * Change from toc to tos * Fix broken tests * Fix typo * Fix spec * Fix rubocop issues * Add Surveys engine MVP (#1364) * Add engine skeleton * Add feature test * An admin can update a survey without questions * Admin can add n questions to a survey * Admin can edit survey questions * An admin can remove questions from surveys * Admin can publish a survey * Minor refactor * Users answer surveys * Fix minor errors * Rubocop * Add model validations * Fix minor things * Fix hound complains * Fix more complains * Fix broken specs * Add some feedback * Add some permissions * Add missing translation * Fix rubocop issues * Add tests to the CI * Fix broken test suite * Normalize locales * Admin can mark question as mandatory (#1386) * Admin can mark question as mandatory * Users must respond mandatory questions * Fix problems with poltergeist * Fix rubocop complains * Fix i18n-issues * Add feedback * Fix rubocop complains * Huge refactor templates * Fix rubocop issues * Admins can select question type * Factor out auto label position code * Moar refactor * Huge JS refactor * Add dynamic answer options for a question * Admin survey form working with dynamic answer options * Single and multiple options working * Fixed rubocop issues * Fix i18n problems * Ignore unused i18n * Add ignore unused * Add answer option default value * Fix rectify problem * Fix minor issues * Fix a few bad merges * Fix broken specs
* Add user answers serializer * Finish export and serializer * Add admin feature specs * Fix rubocop issues
* Admin cannot modify questions if already answered * Add step setting to allow/disallow answers * Fix rubocop issues * Replace answered? with questions_editable?
2255609 to
57d5db9
Compare
|
@beagleknight, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mrcasals and @josepjaume to be potential reviewers. |
|
I just deployed a review app: https://decidim-barcelona-pr-127.herokuapp.com |
🎩 What? Why?
Add Surveys engine to Decidim. An admin should be able to add this new feature to any participatory process.
A survey has the following fields:
An admin must publish a survey before users can answer it. A survey cannot be published without questions and they cannot be edit when the survey is published.
A survey question has the following fields:
Finally, a user should be able to answer a survey but just one time.
📌 Related Issues
📋 Subtasks