Themes as features #765

aviav · 2017-05-19T15:35:52Z

Integrate themes with feature flags

houndci-bot · 2017-05-19T15:36:08Z

spec/models/pageflow/theming_spec.rb

+          config.themes.register(:named_theme)
+        end
+
+        theming = build(:theming, :theme_name => 'named_theme')


Use the new Ruby 1.9 hash syntax.

houndci-bot · 2017-05-19T15:36:08Z

app/models/pageflow/revision.rb

@@ -44,6 +44,10 @@ class Revision < ActiveRecord::Base
    validate :published_until_unchanged, :if => :published_until_was_in_past?
    validate :published_until_blank, :if => :published_at_blank?

+    validates_inclusion_of(:theme_name, in: lambda do |revision|
+                             Pageflow.config_for(revision.entry.account).themes.names
+                           end)


end at 49, 27 is not aligned with lambda do |revision| at 47, 44 or validates_inclusion_of(:theme_name, in: lambda do |revision| at 47, 4.

houndci-bot · 2017-05-19T15:36:08Z

app/helpers/pageflow/themes_helper.rb

+    include RenderJsonHelper
+
+    def themes_options_json_seed(config = Pageflow.config)
+      config.themes.each_with_object({}) do |theme, options|


Prefer {...} over do...end for multi-line chained blocks.

tf

Looks like the migration is missing.

coveralls · 2017-05-22T13:39:03Z

Coverage increased (+0.02%) to 99.053% when pulling 6b317c6 on aviav:themes-as-features into 81a3276 on codevise:master.

tf

I think the commit which adds logic to copy the theme name from the theming to the revision is still missing.

- also move setting home_button_enabled to theming - rename copy_default_meta_tags to reflect this change and make it private - test the public API method instead of the now private helper

aviav · 2017-05-28T08:55:54Z

Is it the reasoning for including the commit that we create entries from the admin?

coveralls · 2017-05-28T09:08:25Z

Coverage increased (+0.03%) to 99.058% when pulling 0191ddb on aviav:themes-as-features into 81a3276 on codevise:master.

tf · 2017-05-29T08:17:54Z

spec/models/pageflow/home_button_spec.rb

@@ -5,15 +5,17 @@ module Pageflow
    describe '#url' do
      it 'returns home_url of revision' do
        revision = Revision.new(home_url: 'http://example.com')
-        theming = Theming.new
+        theming = Theming.new(account: create(:account))


Using the :theming factory would be easier since it would automatically create the account.

tf · 2017-05-29T08:18:13Z

app/models/pageflow/revision.rb

@@ -44,6 +44,11 @@ class Revision < ActiveRecord::Base
    validate :published_until_unchanged, :if => :published_until_was_in_past?
    validate :published_until_blank, :if => :published_at_blank?

+    validates_inclusion_of(:theme_name,
+                           in: lambda do |revision|
+                             Pageflow.config_for(revision.entry.account).themes.names


Didn't we mean to allow entry level theme feature flags?

I think I had that in some other branch. Yes, that is what we want here

tf · 2017-05-29T08:31:47Z

The PR includes a migration that copies the theme names to the revisions. Without the mentioned commit there is no way to create new entries with theme names in their revision, though.

I hadn't noticed it before, but I think it would even make sense to include the changes to entry_theme_stylesheet_link_tag ([1], [2]) since the PR already goes have the way of moving theme_name to Revision. With these changes the moved data would also be used.

The other PR would then mainly be "allow switching themes inside the editor".

tf · 2017-05-29T08:39:49Z

Alternatively, this validation should not have been part of the PR in the first place. Then the migration would not have been required and the PR would really just have been "Themes as (account) features".

coveralls · 2017-05-29T14:31:13Z

Coverage increased (+0.03%) to 99.058% when pulling ed9779d on aviav:themes-as-features into 81a3276 on codevise:master.

coveralls · 2017-05-30T08:19:38Z

Coverage increased (+0.03%) to 99.058% when pulling 9829d74 on aviav:themes-as-features into b452c34 on codevise:master.

coveralls · 2017-05-30T08:19:38Z

Coverage increased (+0.03%) to 99.058% when pulling 9829d74 on aviav:themes-as-features into b452c34 on codevise:master.

coveralls · 2017-05-30T08:49:12Z

Coverage increased (+0.03%) to 99.058% when pulling 49c183c on aviav:themes-as-features into b452c34 on codevise:master.

coveralls · 2017-05-30T10:39:47Z

Coverage increased (+0.03%) to 99.058% when pulling 4f57385 on aviav:themes-as-features into b452c34 on codevise:master.

coveralls · 2017-05-30T10:39:47Z

Coverage increased (+0.03%) to 99.058% when pulling 4f57385 on aviav:themes-as-features into b452c34 on codevise:master.

coveralls · 2017-05-30T11:00:59Z

Coverage increased (+0.03%) to 99.058% when pulling 9aef442 on aviav:themes-as-features into a1cba5a on codevise:master.

coveralls · 2017-05-30T11:00:59Z

Coverage increased (+0.03%) to 99.058% when pulling 9aef442 on aviav:themes-as-features into a1cba5a on codevise:master.

tf

Why have those preview image changes now been included in the PR? Isn't that more related to "switching themes in editor"? On the other hand the stylesheet helper changes mentioned in the previous comment are still not part of the commits.

If it helps move things forward, I'd be open to merging this PR. Regarding the preview images though, I'm wondering if they really need to be that large (especially the preview thumbnail). As noted in the internal ticket system, preview images take quite long to load inside the editor.

coveralls · 2017-05-31T16:26:13Z

Coverage decreased (-0.7%) to 98.382% when pulling 1d77154 on aviav:themes-as-features into 4f2a9d9 on codevise:master.

tf · 2017-06-01T06:54:08Z

Specs are failing

houndci-bot · 2017-06-01T10:35:50Z

spec/models/pageflow/theming_spec.rb

@@ -60,33 +94,52 @@ module Pageflow
      end
    end

-    describe '#copy_default_meta_tags' do
+    describe '#copy_defaults_to' do


Block has too many lines. [29/25]

coveralls · 2017-06-01T10:45:34Z

Coverage increased (+0.03%) to 99.058% when pulling 48cf002 on aviav:themes-as-features into 4f2a9d9 on codevise:master.

coveralls · 2017-06-01T10:45:34Z

Coverage increased (+0.03%) to 99.058% when pulling 48cf002 on aviav:themes-as-features into 4f2a9d9 on codevise:master.

2017-11-07 [Compare changes](codevise/pageflow@12-0-stable...v12.1.0) - Include HLS in output presences of legacy files ([#872](codevise/pageflow#872), [#867](codevise/pageflow#867)) The database migration for Pageflow 12.0 which updates output presences of existing video files is missing the HLS variant. This causes HLS urls of existing video files to be rendered as `undefined`. To apply the fix, install migrations and migrate your database. This fix has previously been released as part of version 12.0.2. - Add Resque::Server to the generated routes ([#871](codevise/pageflow#871)) Mounting the Resque web server makes it easier to inspect background workers and restart jobs that have failed. See the issue description of [#856](codevise/pageflow#856) on how to add this to existing apps. - The theme configured on account level now only acts as a default for new entries. After enabling the `selectable_themes` feature, a theme selection dialog is available inside the editor from the "Title and Options > Appearance" tab. The dialog allows configuring the theme on a per revision basis. ([#781](codevise/pageflow#781), [#897](codevise/pageflow#897)) - Use page from url hash es landing page ([#832](codevise/pageflow#832)) - Do not record history when changing pages via scrolling ([#831](codevise/pageflow#831)) - Improve text tracks and info box display logic ([#826](codevise/pageflow#826)) - Bug fix: Fix order of public i18n fallback ([#883](codevise/pageflow#883)) - Bug fix: Prevent display of NaN duration in video controls ([#878](codevise/pageflow#878)) - Bug fix: Prevent 404 when share image has been deleted ([#816](codevise/pageflow#816)) - Use searchable select boxes in admin forms ([#888](codevise/pageflow#888)) - Remove sensitive data from active admin downloads ([#899](codevise/pageflow#899)) - Add config option to prevent multi account users ([#848](codevise/pageflow#848), [#868](codevise/pageflow#868)) - Add config options to restrict account manager permissions ([#849](codevise/pageflow#849)) - Fix N+1 query in account admin users tab ([#877](codevise/pageflow#877)) - Bug fix: Hide restore link and snapshot button for entry previewers ([#853](codevise/pageflow#853)) - Bug fix: Use copy of current_user in profile form ([#850](codevise/pageflow#850)) - Bug fix: Ensure new entry button is hidden for editors ([#847](codevise/pageflow#847)) - Bug fix: Show folder edit button only for publishers and above ([#838](codevise/pageflow#838)) - Bug fix: Allow :create entry only for publishers on accounts, not on entries ([#836](codevise/pageflow#836)) - Widget configuration ([#694](codevise/pageflow#694), [#809](codevise/pageflow#809)) - Bug fix: Ensure page order in editor preview stays up to date ([#898](codevise/pageflow#898)) - Bug fix: Switch off file meta data edit links when uploading ([#857](codevise/pageflow#857)) - Bug fix: Improve polling for HostedFile state ([#822](codevise/pageflow#822)) - Bug fix: Handle undefined page title. ([#763](codevise/pageflow#763)) - Use relative urls inside dash and hls playlists ([#842](codevise/pageflow#842)) - Use web audio api for volume fading if available ([#800](codevise/pageflow#800), [#863](codevise/pageflow#863)) - Add panorama_mask image file style ([#830](codevise/pageflow#830)) - Bug fix: Make url template generation more robust ([#876](codevise/pageflow#876)) - Themes can now be guarded by feature flags ([#765](codevise/pageflow#765)) - Extend EncodingConfirmation by public account attribute ([#817](codevise/pageflow#817)) - Extend query interface ([#815](codevise/pageflow#815)) - Accept options for accounts admin menu via config ([#811](codevise/pageflow#811)) - Add placeholder options to textareainputview ([#807](codevise/pageflow#807)) - Call hook on entry publication ([#806](codevise/pageflow#806)) - Add rake task and resque job to delete old auto snapshots ([#861](codevise/pageflow#861), [#882](codevise/pageflow#882)) - Generate a secure password in the seeds ([#775](codevise/pageflow#775)) - Allow specifying opacity of image variant logo ([#799](codevise/pageflow#799)) - Allow setting size of custom loading spinner background ([#798](codevise/pageflow#798)) - Add theme option to right align logo in desktop layout ([#797](codevise/pageflow#797)) - Add theme option to hide scroll indicator ([#790](codevise/pageflow#790)) - Make content text max width configurable ([#780](codevise/pageflow#780), [#804](codevise/pageflow#804)) - Import nginx-upload-module guide from wiki ([#814](codevise/pageflow#814), [#821](codevise/pageflow#821)) - Add documentation for versioning policy ([#866](codevise/pageflow#866)) - Fix small typos ([#787](codevise/pageflow#787)) - Precompile assets in Travis run ([#873](codevise/pageflow#873)) - Test workflow improvements ([#803](codevise/pageflow#803)) - Generate admins with account membership in specs ([#840](codevise/pageflow#840)) - Use rails 4.2.9 in tests ([#837](codevise/pageflow#837)) - Clean up memberships admin code ([#835](codevise/pageflow#835)) - Basic tests for UsersTab add user button ([#805](codevise/pageflow#805)) - Use codevise/teaspoon for logging backtraces on console ([#786](codevise/pageflow#786)) - Split vendored code from our own ([#885](codevise/pageflow#885)) - Introduce ApplicationRecord ([#889](codevise/pageflow#889)) - Move configuration into a concern ([#794](codevise/pageflow#794)) - Read the attribute instead of super ([#810](codevise/pageflow#810)) - Destroy widgets when parent subject is destroyed ([#834](codevise/pageflow#834)) - Fix dummy app generation on Travis for Rubygems 2.7 ([#893](codevise/pageflow#893)) - Update contents of gemspec ([#891](codevise/pageflow#891)) - Bug fix: Work around breaking change in resque_mailer 2.4.3 ([#894](codevise/pageflow#894)) See [12-0-stable](https://github.com/codevise/pageflow/blob/12-0-stable/CHANGELOG.md) branch for previous changes.

aviav added 4 commits May 19, 2017 17:32

Integrate themes with features

1610f04

Use more specific config where needed

c750128

Adjust specs to themes as features

3ff4194

Test that themes registered in disabled features cannot be used

490d835

houndci-bot reviewed May 19, 2017

View reviewed changes

Placate Hound

805bdeb

tf requested changes May 22, 2017

View reviewed changes

Add theme name to revisions on database

6b317c6

aviav assigned tf May 22, 2017

tf requested changes May 23, 2017

View reviewed changes

tf removed their assignment May 24, 2017

On creating an entry, set theme_name on its revision

0191ddb

- also move setting home_button_enabled to theming - rename copy_default_meta_tags to reflect this change and make it private - test the public API method instead of the now private helper

aviav assigned tf May 28, 2017

tf requested changes May 29, 2017

View reviewed changes

aviav changed the title ~~Themes as features~~ [WIP] Themes as features May 29, 2017

aviav unassigned tf May 29, 2017

aviav force-pushed the themes-as-features branch from ed9779d to 9829d74 Compare May 30, 2017 08:10

tf modified the milestone: v12.1 May 30, 2017

aviav assigned tf May 30, 2017

aviav changed the title ~~[WIP] Themes as features~~ Themes as features May 30, 2017

aviav unassigned tf May 30, 2017

aviav force-pushed the themes-as-features branch from 4f57385 to 9aef442 Compare May 30, 2017 10:51

aviav assigned tf and unassigned tf May 30, 2017

tf requested changes May 30, 2017

View reviewed changes

tf assigned aviav and unassigned tf May 30, 2017

aviav force-pushed the themes-as-features branch from 9aef442 to 1d77154 Compare May 31, 2017 16:17

tf approved these changes Jun 1, 2017

View reviewed changes

aviav added 4 commits June 1, 2017 12:34

Add themes to editor seeds

1858e22

Add theme_name to PublishedEntry

57ff9ae

Make themes as features configurable per entry

213e08c

Use theming factory in updated specs

48cf002

aviav force-pushed the themes-as-features branch from 1d77154 to 48cf002 Compare June 1, 2017 10:35

houndci-bot reviewed Jun 1, 2017

View reviewed changes

aviav assigned tf and unassigned aviav Jun 1, 2017

tf merged commit da119ad into codevise:master Jun 1, 2017

tilsammans mentioned this pull request Nov 12, 2017

upgrade Pageflow scrollytelling/app#23

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Themes as features #765

Themes as features #765

aviav commented May 19, 2017

houndci-bot May 19, 2017

houndci-bot May 19, 2017

houndci-bot May 19, 2017

tf left a comment

coveralls commented May 22, 2017

tf left a comment

aviav commented May 28, 2017

coveralls commented May 28, 2017 •

edited

Loading

tf May 29, 2017

tf May 29, 2017

aviav May 30, 2017

tf commented May 29, 2017

tf commented May 29, 2017

coveralls commented May 29, 2017 •

edited

Loading

coveralls commented May 30, 2017

coveralls commented May 30, 2017

coveralls commented May 30, 2017 •

edited

Loading

coveralls commented May 30, 2017

coveralls commented May 30, 2017

coveralls commented May 30, 2017

coveralls commented May 30, 2017

tf left a comment

coveralls commented May 31, 2017 •

edited

Loading

tf commented Jun 1, 2017

houndci-bot Jun 1, 2017

coveralls commented Jun 1, 2017

coveralls commented Jun 1, 2017

Themes as features #765

Themes as features #765

Conversation

aviav commented May 19, 2017

houndci-bot May 19, 2017

Choose a reason for hiding this comment

houndci-bot May 19, 2017

Choose a reason for hiding this comment

houndci-bot May 19, 2017

Choose a reason for hiding this comment

tf left a comment

Choose a reason for hiding this comment

coveralls commented May 22, 2017

tf left a comment

Choose a reason for hiding this comment

aviav commented May 28, 2017

coveralls commented May 28, 2017 • edited Loading

tf May 29, 2017

Choose a reason for hiding this comment

tf May 29, 2017

Choose a reason for hiding this comment

aviav May 30, 2017

Choose a reason for hiding this comment

tf commented May 29, 2017

tf commented May 29, 2017

coveralls commented May 29, 2017 • edited Loading

coveralls commented May 30, 2017

coveralls commented May 30, 2017

coveralls commented May 30, 2017 • edited Loading

coveralls commented May 30, 2017

coveralls commented May 30, 2017

coveralls commented May 30, 2017

coveralls commented May 30, 2017

tf left a comment

Choose a reason for hiding this comment

coveralls commented May 31, 2017 • edited Loading

tf commented Jun 1, 2017

houndci-bot Jun 1, 2017

Choose a reason for hiding this comment

coveralls commented Jun 1, 2017

coveralls commented Jun 1, 2017

coveralls commented May 28, 2017 •

edited

Loading

coveralls commented May 29, 2017 •

edited

Loading

coveralls commented May 30, 2017 •

edited

Loading

coveralls commented May 31, 2017 •

edited

Loading