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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduces an option to toggle the header snippets setting #1923

Merged
merged 3 commits into from
Sep 26, 2017

Conversation

jkraemer
Copy link
Contributor

馃帺 What? Why?

As discussed in #1868 , here's a PR that introduces a setting to toggle the header_snippets feature.
I made it default to true (enabled) to not break existing applications, but new apps will have it disabled by default through the generated initializer.

- defaults to true (enabled) to not break existing applications
- new apps will have it disabled by default through the generated initializer.
expect(organization.header_snippets).to_not be_present
end

it "saves header snippets if configured" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you wrap this it inside a context?

context "when header snippets are configured" do
  before do
    allow(Decidim).to receive(:enable_html_header_snippets).and_return(true)
  end

  it "saves header snippets" do
    ...
  end
end

@mrcasals
Copy link
Contributor

Also, can you check the tests please? 馃槃

@codecov
Copy link

codecov bot commented Sep 26, 2017

Codecov Report

Merging #1923 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1923      +/-   ##
==========================================
+ Coverage   98.48%   98.48%   +<.01%     
==========================================
  Files        1127     1127              
  Lines       25330    25351      +21     
==========================================
+ Hits        24947    24968      +21     
  Misses        383      383

@mrcasals
Copy link
Contributor

Thank you! 馃帀 Let's wait for the tests and we'll merge this 馃槃

@xabier
Copy link
Contributor

xabier commented Sep 26, 2017

Nice one, thanks @jkraemer

@mrcasals mrcasals merged commit d2f23df into decidim:master Sep 26, 2017
@mrcasals
Copy link
Contributor

Argh, I forgot to ask you for a CHANGELOG entry 馃槶

@mrcasals
Copy link
Contributor

See #1927 for the CHANGELOG entry.

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

3 participants