Add content pages #190

oriolgual · 2016-11-08T15:30:52Z

🎩 What? Why?

Adds a feature so admins can manage static content pages.

📌 Related Issues

Related to Content pages editor #161

📋 Subtasks

📷 Screenshots (optional)

Not yet.

👻 GIF

houndci-bot · 2016-11-08T15:31:05Z

decidim-core/lib/decidim/page_finder.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+module Decidim
+  class PageFinder < HighVoltage::PageFinder


Missing top-level class documentation comment.

houndci-bot · 2016-11-08T15:31:07Z

decidim-core/app/models/decidim/page.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+module Decidim
+  class Page < ApplicationRecord


Missing top-level class documentation comment.

houndci-bot · 2016-11-08T15:31:07Z

decidim-admin/app/forms/decidim/admin/page_form.rb

+# frozen_string_literal: true
+module Decidim
+  module Admin
+    class PageForm < Rectify::Form


Missing top-level class documentation comment.

codecov-io · 2016-11-09T09:21:02Z

Current coverage is 97.60% (diff: 0.00%)

Merging #190 into master will increase coverage by 0.23%

@@             master       #190   diff @@
==========================================
  Files           246        263    +17   
  Lines          3794       4293   +499   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3694       4190   +496   
- Misses          100        103     +3   
  Partials          0          0

Powered by Codecov. Last update ebd0654...6540ea1

josepjaume · 2016-11-10T16:25:29Z

decidim-system/app/commands/decidim/system/create_default_pages.rb

+      # Returns nothing.
+      def call
+        Decidim::Page::DEFAULT_PAGES.map do |slug|
+          Decidim::Page.create!(


Maybe you should do this in an idempotent way? So if the page already exists it doesn't blow up.

josepjaume · 2016-11-10T16:26:26Z

decidim-system/app/commands/decidim/system/register_organization.rb

+        end
+      end
+
+      def localized_attribute(attribute)


localized_attribute is duplicated in https://github.com/AjuntamentdeBarcelona/decidim/pull/190/files#diff-9d9bec53e0e6a2b136da17b1ab2119a3R33 . Maybe we should extract this into a module?

I forgot to temove the code from RegisterOrganization, it shouldn't be there.

josepjaume · 2016-11-10T16:27:22Z

decidim-system/config/locales/ca.yml

@@ -26,6 +26,10 @@ ca:
        update:
          error: S'ha produït un error en actualitzar aquest administrador..
          success: Administrador actualitzat correctament
+      default_pages:
+        placeholders:
+          content: Si us plau, afegir contingut significatiu a aquesta pàgina afegir el panell de control de l'administrador.


Grammar nazi: "Si us plau, afegeix contingut significatiu a aquesta pàgina des del panell d'administració."

It was auto-translated, it wasn't review it yet 😛

josepjaume · 2016-11-10T16:27:54Z

decidim-system/config/locales/en.yml

@@ -26,6 +26,10 @@ en:
        update:
          error: There was an error when updating this admin.
          success: Admin updated successfully
+      default_pages:
+        placeholders:
+          content: Please add meaningful content to this page add the admin dashboard.


"at the" o "on the"

LOL I actually wrote add xD

josepjaume · 2016-11-10T16:28:11Z

decidim-system/config/locales/es.yml

@@ -26,6 +26,10 @@ es:
        update:
          error: Se ha producido un error al actualizar este administrador ..
          success: Administrador actualizado correctamente
+      default_pages:
+        placeholders:
+          content: Por favor, añadir contenido significativo a esta página añadir el panel de control del administrador.


"Por favor, añade contenido significativo..."

I've updated everything

mrcasals · 2016-11-15T10:09:49Z

decidim-admin/app/commands/decidim/admin/update_static_page.rb

+# frozen_string_literal: true
+module Decidim
+  module Admin
+    # A command with all the business logic when creating a new participatory


s/creating/updating/

mrcasals · 2016-11-15T10:10:11Z

decidim-admin/app/commands/decidim/admin/update_static_page.rb

+        @form = form
+      end
+
+      # Executes the command. Braodcasts these events:


s/Braodcasts/Broadcasts/

mrcasals · 2016-11-15T10:12:18Z

decidim-admin/config/locales/ca.yml

+        actions:
+          view: Visualitzar la pàgina pública
+        create:
+          error: Error


Missing i18n

Alternatively, you can remove these keys and wait until #225 is merged and add these translations via Crowdin.

mrcasals · 2016-11-15T10:13:30Z

decidim-admin/config/locales/es.yml

+        actions:
+          view: Ver la página pública
+        create:
+          error: Error


Same with the Catalan translation

mrcasals · 2016-11-15T10:15:24Z

decidim-core/db/migrate/20161108093802_create_decidim_static_pages.rb

+class CreateDecidimStaticPages < ActiveRecord::Migration[5.0]
+  def change
+    create_table :decidim_static_pages do |t|
+      t.hstore :title, null: false


Use jsonb?

The display_for helper depends on the column being hstore. I'd create another issue to migrate existing hstore columns to jsonb and then update the helper.

I've created an issue to fix this: https://github.com/AjuntamentdeBarcelona/decidim/issues/228

mrcasals · 2016-11-15T10:15:41Z

decidim-core/db/migrate/20161108093802_create_decidim_static_pages.rb

+    create_table :decidim_static_pages do |t|
+      t.hstore :title, null: false
+      t.string :slug, null: false
+      t.hstore :content, null: false


oriolgual · 2016-11-15T10:37:55Z

Code Climate is complaining about the TODO at the Readme. Since the TODO refers to documenting the particpatory process dashboard at the admin, it's outisde the scope of this PR.

josepjaume

Nice! Check my comments to see if you agree with me - I consider this mergeable in its current state. Anyway, good work.

josepjaume · 2016-11-15T10:55:03Z

decidim-admin/README.md

-Short description and motivation.
+
+This library adds and administration dashboard so users can manage their
+organization, particpatory processes and all other entities.


Learn to write

josepjaume · 2016-11-15T11:01:35Z

decidim-admin/app/controllers/decidim/admin/static_pages_controller.rb

+    #
+    class StaticPagesController < ApplicationController
+      def index
+        authorize! :index, Decidim::StaticPage


I think Decidim is redundant here. Applies to all other controllers as well (outside of the scope of this PR)

josepjaume · 2016-11-15T11:04:20Z

decidim-admin/app/models/decidim/admin/abilities/admin_user.rb

@@ -14,6 +14,10 @@ def initialize(user)
          can :manage, ParticipatoryProcess
          can :manage, ParticipatoryProcessStep
          can :manage, ParticipatoryProcessUserRole
+          can [:create, :update, :index, :new, :read], StaticPage
+          can [:update_slug, :destroy], [StaticPage, StaticPageForm] do |page|
+            !StaticPage.default?(page.slug)


josepjaume · 2016-11-15T11:05:19Z

decidim-admin/app/views/decidim/admin/static_pages/_form.html.erb

+<% end %>
+
+<div class="field">
+  <%= form.translated :text_area, :content %>


We need to change this when https://github.com/AjuntamentdeBarcelona/decidim/pull/227 is merged (NO HURRIES @beagleknight)

josepjaume · 2016-11-15T11:10:55Z

decidim-core/lib/decidim/page_finder.rb

+  # to render. We need this because we allow rendering pages with content from
+  # the database (with Decidim::Page) but also fallback to a template if it
+  # exists.
+  class PageFinder < HighVoltage::PageFinder


Are you entirely sure using HighVoltage is justified? In the end, it only ends up doing something like render template: "pages/#{page_name}" in the controller.

We're including a dependency that doesn't add much value and that can lead to issues in the future (being locked to a Rails version, for example).

HighVoltage was already in the project, if we want to discuss the need about it we can do it in another issue/PR.

josepjaume · 2016-11-15T11:11:26Z

decidim-core/spec/factories.rb

+  factory :static_page, class: Decidim::StaticPage do
+    slug { Faker::Internet.slug(nil, '-') }
+    title { Decidim::Faker::Localized.sentence(3) }
+    content { Decidim::Faker::Localized.wrapped("<p>", "</p>") { Decidim::Faker::Localized.sentence(4) } }


oriolgual self-assigned this Nov 8, 2016

houndci-bot reviewed Nov 8, 2016

View reviewed changes

oriolgual force-pushed the feature/content-pages-editor branch 2 times, most recently from cdc3149 to d577d80 Compare November 9, 2016 09:21

oriolgual force-pushed the feature/content-pages-editor branch 5 times, most recently from 32bd3b0 to 7f5b453 Compare November 10, 2016 11:48

josepjaume previously requested changes Nov 11, 2016

View reviewed changes

mrcasals mentioned this pull request Nov 14, 2016

Create a WYSIWYG editor field #106

Closed

oriolgual force-pushed the feature/content-pages-editor branch 3 times, most recently from 6b8786a to 46ded59 Compare November 15, 2016 10:17

mrcasals suggested changes Nov 15, 2016

View reviewed changes

mrcasals approved these changes Nov 15, 2016

View reviewed changes

josepjaume approved these changes Nov 15, 2016

View reviewed changes

oriolgual force-pushed the feature/content-pages-editor branch from 5f3873e to d165938 Compare November 15, 2016 12:10

oriolgual added 6 commits November 15, 2016 14:44

Add content pages

d4a7aec

Default pages

91ae66c

Rename to StaticPage to avoid confusions with the pages component

19c1ddf

Add docs

c7b4714

Typos

8d13a34

Typos

6540ea1

oriolgual force-pushed the feature/content-pages-editor branch from d165938 to 6540ea1 Compare November 15, 2016 13:44

oriolgual merged commit 8092b0c into master Nov 15, 2016

oriolgual deleted the feature/content-pages-editor branch November 15, 2016 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add content pages #190

Add content pages #190

oriolgual commented Nov 8, 2016 •

edited

houndci-bot Nov 8, 2016

houndci-bot Nov 8, 2016

houndci-bot Nov 8, 2016

codecov-io commented Nov 9, 2016 •

edited

josepjaume Nov 10, 2016

josepjaume Nov 10, 2016

oriolgual Nov 14, 2016

josepjaume Nov 10, 2016

oriolgual Nov 14, 2016

josepjaume Nov 10, 2016

oriolgual Nov 14, 2016

josepjaume Nov 10, 2016

mrcasals Nov 15, 2016

mrcasals Nov 15, 2016

mrcasals Nov 15, 2016

mrcasals Nov 15, 2016

mrcasals Nov 15, 2016

mrcasals Nov 15, 2016

oriolgual Nov 15, 2016

oriolgual Nov 15, 2016

mrcasals Nov 15, 2016

oriolgual Nov 15, 2016

oriolgual commented Nov 15, 2016

josepjaume left a comment

josepjaume Nov 15, 2016

josepjaume Nov 15, 2016

josepjaume Nov 15, 2016

josepjaume Nov 15, 2016

josepjaume Nov 15, 2016

oriolgual Nov 15, 2016

josepjaume Nov 15, 2016

Add content pages #190

Add content pages #190

Conversation

oriolgual commented Nov 8, 2016 • edited

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

👻 GIF

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 9, 2016 • edited

Current coverage is 97.60% (diff: 0.00%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oriolgual commented Nov 15, 2016

josepjaume left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oriolgual commented Nov 8, 2016 •

edited

codecov-io commented Nov 9, 2016 •

edited