Content blocks: use settings and images by mrcasals · Pull Request #3968 · decidim/decidim

mrcasals · 2018-08-06T08:25:16Z

🎩 What? Why?

This PR lets admins change the content blocks settings and images from the admin section.

📌 Related Issues

Related to Add ContentBlocks #3839

📋 Subtasks

Move sortable content blocks rows to cells
Add form to edit the content blocks settings
Add settings_form_cell to content blocks. This cell will receive a form object wrapping the content block, and should render the settings fields for the content block. This allows devs to add help texts and customize how to form looks.
Add images
Move "hero" content block data to settings and add migration
Add/modify seeds
Add system specs
Add specs for UpdateContentBlock command
Add documentation regarding the feature

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

+      # Public: Initializes the command.
+      #
+      # form    - The form from which the data in this component comes from.
+      # component - The component to update.


oriolgual · 2018-08-07T12:26:04Z

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

+
+module Decidim
+  module Admin
+    # This command gets called when a contnt block is updated from the admin panel.


s/contnt/content

oriolgual · 2018-08-07T12:26:12Z

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

+        @scope = scope
+      end
+
+      # Public: Creates the Component.


oriolgual · 2018-08-07T12:26:31Z

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

+      def call
+        return broadcast(:invalid) if form.invalid?
+
+        update_content_block_settings


Wrap these in a transaction?

oriolgual · 2018-08-07T12:26:39Z

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

+      def update_content_block_images
+        content_block.image_names.each do |image_name|
+          attachment = content_block.images.send(image_name)
+          byebug


Remove this

oriolgual · 2018-08-07T12:26:47Z

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

+          elsif form.images[image_name]
+            update_image(attachment, image_name, form.images[image_name])
+          end
+


Remove extra line

oriolgual · 2018-08-07T12:28:25Z

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

+        @content_blocks ||= Decidim::ContentBlock.for_scope(:homepage, organization: current_organization)
+      end
+
+      def used_manifests


Maybe rename used_manifests to current_manifests and unused_manifests to available_manifests?

Although it's a bit weird to talk about manifests when we seem to want content blocks

oriolgual · 2018-08-07T12:30:27Z

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

+      def define_methods
+        block.manifest.image_names.each do |image_name|
+          self.class.define_method image_name do
+            Attachment.find_by(attached_to:@block, title: { name: image_name })


Should this be memoized?

oriolgual · 2018-08-07T12:30:58Z

decidim-core/lib/decidim/content_block_manifest.rb

+    # Public: Registers the cell this content block will use to render the
+    # settings form in the admin section. Use `#settings_form_cell_name` to
+    # retrieve it.
+    def settings_form_cell(cell_name)


Shouldn't this be settings_form_cell=?

No, to unify the API. This is the same strategy I followed for cell/cell_name

Now that I think of it, I can change the API to use = so that I can remove all these weird-looking and confusing methods.

Options are hard-coded by now

oriolgual · 2018-08-10T12:27:47Z

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

+        end
+      end
+
+      def content_block_from_manifest


Isn't this a bit weird? Creating a content block at the controller in this way its' kind of unexpected 🤨

I need to modify the DB here, any suggestion on how to improve this?

Not really, I don't know how to solve it.

oriolgual · 2018-08-10T12:29:47Z

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

    validates :default_locale, inclusion: { in: :available_locales }

-    mount_uploader :homepage_image, Decidim::HomepageImageUploader
+    mount_uploader :homepage_image, Decidim::HomepageImageUploader # DEPRECATED


Why isn't this removed?

Because I need to access this uploader from the migration. Maybe I can remove this from here and reopen the class in the migration, add the mount_uploader part there and keep this clean?

oriolgual · 2018-08-10T12:30:31Z

decidim-core/app/views/layouts/decidim/_application.html.erb

  url: request.original_url,
  twitter_handler: current_organization.twitter_handler,
-  image_url: current_organization.homepage_image.url
+  image_url: Decidim::ContentBlock.published.find_by(


Maybe we should add this logic to the organization, so the external api doesn't change.

oriolgual · 2018-08-10T12:31:07Z

decidim-core/db/migrate/20180810092428_move_organization_fields_to_hero_content_block.rb

+      settings = welcome_text.inject(settings) { |acc, (k, v)| acc.update("welcome_text_#{k}" => v) }
+
+      content_block.settings = settings
+      content_block.images_container.background_image = organization.homepage_image.file


Have you tried this with "real" data?

I tried it with the development app, which had this data. Does this count as "real data"?

oriolgual · 2018-08-10T12:31:48Z

decidim-core/lib/decidim/content_block_manifest.rb

    def image_names_are_unique
-      errors.add(:image_names, :invalid) if image_names.count != image_names.uniq.count
+      image_names = images.map { |image| image[:name] }
+      errors.add(:images, "names must be unique per manifest") if image_names.count != image_names.uniq.count


Not needed, this will be executed on application startup, the error is not for end users.

oriolgual · 2018-08-10T12:31:54Z

decidim-core/lib/decidim/content_block_manifest.rb

-    class ImageNameCannotBeBlank < StandardError; end
+    def images_have_an_uploader
+      uploaders = images.map { |image| image[:uploader].presence }
+      errors.add(:images, "must have an uploader") if uploaders.compact.count != images.count


oriolgual · 2018-08-10T12:32:55Z

decidim-core/lib/decidim/core/test/factories.rb

    sequence(:host) { |n| "#{n}.lvh.me" }
    description { Decidim::Faker::Localized.wrapped("<p>", "</p>") { Decidim::Faker::Localized.sentence(2) } }
-    welcome_text { Decidim::Faker::Localized.wrapped("<p>", "</p>") { Decidim::Faker::Localized.sentence(2) } }
-    homepage_image { Decidim::Dev.test_file("city.jpeg", "image/jpeg") }


Can't we create content blocks at the factory? Or maybe we shouldn't?

We can, but I don't really think it's needed in most cases

oriolgual · 2018-08-10T12:33:42Z

decidim-generators/lib/decidim/generators/install_generator.rb

-          rails "db:migrate"
-        end
-
+        rails "db:migrate"


Why? If you had to change this maybe you forgot to define a class inside a migration.

In the migration I need to access elements defined in the ContentBlock model (I need to access the images_container dynamic class). If I redefine the class in the migration then I cannot access that dynamic class, and makes life a lot harder.

I've talked to @deivid-rodriguez and agreed to change this file.

mrcasals added the lot: core label Aug 6, 2018

mrcasals self-assigned this Aug 6, 2018

ghost added the status: WIP label Aug 6, 2018

mrcasals force-pushed the content-blocks/use-options branch 3 times, most recently from f1e497f to f5e3cf7 Compare August 6, 2018 10:24

mrcasals added module: pages and removed module: pages labels Aug 6, 2018

mrcasals force-pushed the content-blocks/use-options branch from c79c3d8 to b3029d6 Compare August 6, 2018 14:16

oriolgual suggested changes Aug 7, 2018

View reviewed changes

mrcasals changed the title ~~[WIP] Content blocks: use settings~~ [WIP] Content blocks: use settings and images Aug 7, 2018

mrcasals force-pushed the content-blocks/use-options branch from 2634979 to b8dff39 Compare August 7, 2018 13:10

mrcasals added 16 commits August 9, 2018 10:52

Move content blocks admin to cells

e2626a7

Add pencil icon

04217a9

Add content block configuration links

feecda5

Only show links if content blocks have settings

98fb769

Update content blocks

7e35ed5

Options are hard-coded by now

Remove unused settings & use content block settings in view

b3d9eaf

Fix linter issues

b3f97a3

Add settings form cell to content blocks

42f6547

Ensure the hero content block uses settings in homepage

3bf2f39

Let content blocks use images

a73900a

Fix linter erros

0a95f63

Address feedback

3444599

Simplify API to register content blocks

ee4d620

Improve method naming

784305d

Add tests for images support

efa499f

Add specs for command

07a0451

mrcasals force-pushed the content-blocks/use-options branch 2 times, most recently from 6c25f78 to edd9b32 Compare August 9, 2018 10:47

mrcasals added 2 commits August 9, 2018 12:56

Move content block images frmo attachments to custom solution

68a8969

Fix docs

ec779c2

mrcasals force-pushed the content-blocks/use-options branch from edd9b32 to ec779c2 Compare August 9, 2018 11:08

mrcasals added 5 commits August 9, 2018 14:19

Fix specs

b24d0f4

Add system specs to test content blocks can be configured

a5eaf29

Fix specs

997667f

Migrate data from organization to hero content block

4cdf7ad

Update seeds

f1c0f0c

mrcasals force-pushed the content-blocks/use-options branch from ab08010 to f1c0f0c Compare August 10, 2018 10:17

Fix generators tests

79b9d23

mrcasals added in-review and removed status: WIP labels Aug 10, 2018

mrcasals changed the title ~~[WIP] Content blocks: use settings and images~~ Content blocks: use settings and images Aug 10, 2018

oriolgual suggested changes Aug 10, 2018

View reviewed changes

Move uploader from model to mgiration

cf5567a

ghost added the status: WIP label Aug 10, 2018

mrcasals added 2 commits August 13, 2018 11:15

Cleanup

dd24086

Fix migration

9148fe9

oriolgual approved these changes Aug 27, 2018

View reviewed changes

mrcasals removed the status: WIP label Aug 30, 2018

mrcasals mentioned this pull request Aug 30, 2018

Add ContentBlocks #3839

Merged

18 tasks

mrcasals merged commit e06bef7 into master Aug 30, 2018

mrcasals deleted the content-blocks/use-options branch August 30, 2018 10:09

mrcasals mentioned this pull request Aug 30, 2018

Fix hero content block migration when no welcome_text is set #4061

Merged

1 task

deivid-rodriguez mentioned this pull request Sep 21, 2018

Normalize content block migration #4152

Merged

5 tasks

oriolgual mentioned this pull request Nov 22, 2018

Fix update content block image #4549

Merged

2 tasks

mrcasals added this to the CDP4 milestone Dec 10, 2018

Uh oh!

Conversation

mrcasals commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎩 What? Why?

📌 Related Issues

📋 Subtasks

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mrcasals commented Aug 6, 2018 •

edited

Loading