Updates translatable custom pages #2913

papayalabs · 2018-09-20T13:19:05Z

References

Visual Changes

Editing with many languages

Notes

In the migration of translation tables of custom pages ( 20180802221830_add_translate_pages.rb ) we add a change_column to allow null in title. Title is not used anymore as index in table and now is used in many languages title_en,title_es,title_fr, etc. Give NullConstraint from database if do not make this change.

class AddTranslatePages < ActiveRecord::Migration
  def self.up
    SiteCustomization::Page.create_translation_table!({
      title: :string,
      subtitle: :string,
      content: :text
    })
    change_column :site_customization_pages, :title, :string, :null => true
  end

  def self.down
    SiteCustomization::Page.drop_translation_table!
    change_column :site_customization_pages, :title, :string, :null => false
  end
end

papayalabs · 2018-09-20T13:48:35Z

Hi @mlovic, all the functionality works fine, the only detail is when you click other translation tag in the Custom Page form the CKEDITOR goes down one line and keeps like that if you continue navigating. I try to fix this from the beginning but I could not find how.

javierm

Thanks @papayalabs! I've made some comments; could you have a look? Feel free to disagree with any of the comments, of course 😉.

I haven't tested the functionality properly yet; I'll do it after I hear back from you. Thanks again!

javierm · 2018-09-21T10:37:55Z

app/controllers/admin/site_customization/pages_controller.rb

@@ -30,19 +31,25 @@ def destroy
 notice = t('admin.site_customization.pages.destroy.notice')
 redirect_to admin_site_customization_pages_path, notice: notice
 end
+
+ def resource


IMHO this method should be private.

javierm · 2018-09-21T10:43:21Z

app/controllers/admin/site_customization/pages_controller.rb

 :title,
 :subtitle,
 :content,
 :more_info_flag,
 :print_content_flag,
 :status,
- :locale
+ :locale]
+


Would you mind checking trailing whitespaces? We're currently in the process of adding a rubocop rule for trailing whitespaces. In the meantime, you might want to add it manually and check for trailing whitespaces using tools like rubocop-git or (even better IMHO) configure your text editor to highlight trailing whitespaces or (probably useful as well) add a pre-commit git hook which checks whitespace issues.

javierm · 2018-09-21T10:46:23Z

app/models/site_customization/page.rb


 def url
 "/#{slug}"
 end
 end
+


We're also about to add a newline character to rubocop rules. Would you mind checking for trailing blank lines as well, or configuring your editor so it automatically adds a newline character? This way we don't accidentally change files just to add/remove blank lines at the end.

app/views/admin/site_customization/pages/_form.html.erb

javierm · 2018-09-21T10:51:53Z

app/views/admin/site_customization/pages/_form.html.erb

+ data: { locale: locale },
+ style: display_translation?(locale) do %>
+ <%= f.cktext_area "content_#{locale.to_s.downcase.underscore.to_sym}",
+ class: "js-globalize-attribute",


I think there a few tabs in these lines. Could you check we use whitespaces for indentation instead of tabs? Again, configuring your text editor to highlight tab characters makes the job way easier.

spec/features/site_customization/custom_pages_spec.rb

javierm · 2018-09-21T11:07:11Z

spec/features/site_customization/custom_pages_spec.rb

+ expect(page).not_to have_link "Español"
+ end
+
+ context "Globalize javascript interface" do


It looks like some of these tests could be replaced by it_behaves_like "translatable". I'm not sure, though. @mlovic @papayalabs What do you think?

Using it_behaves_like "translatable" will use test "Add a translation scenario" with "Français" and I think is important to test with "Português" because for this language has some particularities that may trigger some errors that will not trigger with other languages. But again is my perspective, if you guys prefer with "translatable" code I will changed ;)

I agree it's important to test Brazilian Portuguese 👍! In the translatable shared examples there's a scenario for that case, called "Add a translation for a locale with non-underscored name". Would that be enough, or do we need something specific to custom pages? If we need something specific, we could add it_behaves_like "translatable" and then add tests for specific cases, like in collaborative legislation specs. What do you think?

Sorry, I had not seen that scenario in the translatable shared. I think that would be enough, Thanks for the clarification @javierm! I am going to updated with translatable shared

Hi @javierm! in the translatable shared class is there a way to pass some field that are not translatable? For example in cutom pages the default for status is draft and we need to pass status published for tests.

How about adding a new factory? In spec/factories/administration.rb:

factory :site_customization_page, class: 'SiteCustomization::Page' do slug "example-page" # (...) trait :published do status "published" end # (...) factory :published_site_customization_page, traits: [:published] # <---- Add this line end

Maybe that way we could pass published_site_customization_page as the factory name to translatable.

You might run into issues using the translatable behaviour because (so far) some of the specs there don't work well with CKEditor. If that happens, it's OK to leave the tests where they are for now.

Thanks so much for the tip!!

I get confused, was not necessary to create published_site_customization_page, the thing was that in translatable class it uses a update_button_text and for default uses the name Save changesbut the button in Custom Pages is Update Custom pageand I thought that the page was not published. I already add Update Custom page name in translatable class. But you are right does not recognize CKEditor fields nevertheless it works fine with the others fields and the tests now does not check CKEditor fields either. So how I proceed? Leave tests as they are or use translatable with (title,subtitle) only? Thanks!!

Leaving tests as they are is fine 👌. We'll try to better integrate CKEditor with Capybara in the future.

Ok @javierm! So I thing all the requested changes are ready. I unified everything in one commit. Thanks!

javierm · 2018-09-21T11:08:22Z

db/schema.rb

@@ -116,6 +116,8 @@
 t.datetime "updated_at", null: false
 t.text "background_color"
 t.text "font_color"
+ t.string "banner_position"
+ t.text "headings"


I think these lines aren't related to this pull request.

javierm · 2018-09-21T11:09:19Z

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.

-ActiveRecord::Schema.define(version: 20180813141443) do
+ActiveRecord::Schema.define(version: 20180831000000) do


This schema version doesn't seem to be related to any migration files. I wonder how it got there 😕.

javierm · 2018-09-21T11:10:42Z

db/schema.rb

 t.string "subtitle"
 t.text "content"
 t.boolean "more_info_flag"
 t.boolean "print_content_flag"
 t.string "status", default: "draft"
 t.datetime "created_at", null: false
 t.datetime "updated_at", null: false
- t.string "locale"


I didn't find a migration file where this column was dropped. Did I miss it?

papayalabs · 2018-09-24T06:13:07Z

Hi @javierm! Yes you are right that I need to setup everything to be more aware of indentation and other rubocop rules. I fix all you tell me about this. I also change schema.rb ( hope I do it right). I only comment the changes in "spec/features/site_customization/custom_pages_spec.rb" with my perspective but is not fixed. Please tell me if you need more changes.

papayalabs · 2018-09-24T07:51:52Z

@javierm almost all the changes you request were made but I unified everything in one commit ( first one ) so it looks that were not made

javierm

This is looking good. Thank you so much! I've just left a couple of comments, and since I'm not sure about the slug usage I've mentioned @decabeza who will probably know better.

javierm · 2018-09-24T14:19:29Z

app/views/admin/site_customization/pages/_form.html.erb

+ class: "js-globalize-attribute",
+ ckeditor: { language: locale },
+ data: { locale: locale },
+ label: false %>


I think we need (somehow) a label here. Maybe you could use the code @microweb10 hast just implemented (still not merged, though): https://github.com/consul/consul/pull/2914/files#diff-4d169e27f1a8740fdcd328f352bf82d2R38

Yes, our functionality with ckeditor is similar. Will work just fine anything that @microweb10 implement. 👍

javierm · 2018-09-24T14:29:43Z

app/views/admin/site_customization/pages/edit.html.erb

@@ -1,11 +1,11 @@
-<% provide :title do %>
- <%= t("admin.header.title") %> - <%= t("admin.menu.site_customization.pages") %> - <%= @page.title %>
+<% provide :slug do %>


Actually, we need to use provide :title 😉, since the parameter here is used to generate the HTML <title> tag (see app/views/layouts/_common_head.html.erb, the line saying <title><%= content_for?(:title) ? yield(:title) : default_title %></title>).

Ok I will check this. I took the daring to replace slugwith titlebut I am not sure is the best way to go. 😉 I take this way because slug like status and created,updated at are not translatable in many languages but I could change everything to use titleagain. Just confirm me!

javierm · 2018-09-24T14:33:24Z

app/views/admin/site_customization/pages/edit.html.erb

-<% provide :title do %>
- <%= t("admin.header.title") %> - <%= t("admin.menu.site_customization.pages") %> - <%= @page.title %>
+<% provide :slug do %>
+ <%= t("admin.header.title") %> - <%= t("admin.menu.site_customization.pages") %> - <%= @page.slug %>


I don't have a strong preference about which title the browser should show, which is what we're changing here. Which one do you think it's better?

I don´t have preference either 😉. Just confirm me what to use. :)

javierm · 2018-09-24T14:34:34Z

app/views/admin/site_customization/pages/edit.html.erb

 <% end %>

 <%= back_link_to admin_site_customization_pages_path %>

 <div class="small-12 column">
- <h2 class="inline-block"><%= t("admin.site_customization.pages.edit.title", page_title: @page.title) %></h2>
+ <h2 class="inline-block"><%= t("admin.site_customization.pages.edit.title", page_title: @page.slug) %></h2>


Similar comment as the previous one. Should admins see "Editing My Page" or "Editing my-page"? Maybe @decabeza can tell better.

Hi, @papayalabs and @javierm! I think it's better always to use the page title. Usually, the slugs will have a dash or underscore and it'll a little more uncomprehensive. 😌

I think is easier to read
Administration - Editing My custom page
instead of
Administration - Editing my-custom-page

Hi @decabeza! Ok clear! I will return everything with title instead slug. 👍

javierm · 2018-09-24T14:36:39Z

app/views/admin/site_customization/pages/index.html.erb

 <th class="small-4"><%= t("admin.actions.actions") %></th>
 </tr>
 </thead>
 <tbody>
 <% @pages.each do |page| %>
 <tr id="<%= dom_id(page) %>">
 <td>
- <%= link_to page.title, edit_admin_site_customization_page_path(page) %>
+ <%= link_to page.slug, edit_admin_site_customization_page_path(page) %>


In all other translatable sections we show the title. @decabeza @papayalabs Which one do you guys think is more convenient here?

@papayalabs, @javierm, in this case, I think it would a good idea show the both, something like

<th><%= t("admin.site_customization.pages.page.title") %></th> <th><%= t("admin.site_customization.pages.page.slug") %></th> <td><%= link_to page.title, edit_admin_site_customization_page_path(page) %></td> <td><%= page.slug %></td>

So... the admin can see the slug info in the index table without have to edit the page:

Title Slug My custom page my-custom-page

Moreover, in other sections we always use the title as a link, consistency is the key! 😌 😉

Ok @decabeza, show both and title as the key link 😉👍

javierm · 2018-09-24T14:44:58Z

spec/features/site_customization/custom_pages_spec.rb

+ expect(page).not_to have_link "Español"
+ end
+
+ context "Globalize javascript interface" do


I agree it's important to test Brazilian Portuguese 👍! In the translatable shared examples there's a scenario for that case, called "Add a translation for a locale with non-underscored name". Would that be enough, or do we need something specific to custom pages? If we need something specific, we could add it_behaves_like "translatable" and then add tests for specific cases, like in collaborative legislation specs. What do you think?

javierm · 2018-09-24T14:54:14Z

app/views/pages/help/_other.html.erb

@@ -3,7 +3,7 @@
 <ul class="features">
 <li><%= link_to t("pages.help.other.how_to_use", org_name: setting['org_name']), how_to_use_path %></li>

- <% SiteCustomization::Page.with_more_info_flag.with_same_locale.each do |custom_page| %>
+ <% SiteCustomization::Page.with_more_info_flag.each do |custom_page| %>


This one is tricky. IMHO we should only show pages which have a translation for the current locale. How about keeping this line the way it was and changing the scope to:

scope :with_same_locale, -> { joins(:translations).where("site_customization_page_translations.locale": I18n.locale) }

Feel free to simplify the condition! This is only an example.

I am agree. Will add again with_same_local scope and check to simplify the condition.

Done. But I dont find how simplified more.

javierm · 2018-09-26T08:45:28Z

db/migrate/20180924071722_add_translate_pages.rb

+ SiteCustomization::Page.drop_translation_table!
+ change_column :site_customization_pages, :title, :string, :null => false
+ end
+end


I think this missing newline character is the only spacing issue left.

Yes, sorry. Updated.

javierm · 2018-09-26T13:30:04Z

app/helpers/translatable_form_helper.rb

+ data: { locale: locale }
+ else
+ @template.concat send(field_type, localized_attr_name, final_options)
+ end


We've already merged #2914 which incorporated these changes, in case you'd like to rebase against master.

javierm · 2018-09-27T11:41:19Z

app/views/admin/site_customization/pages/_form.html.erb

- <%= f.submit class: "button success expanded" %>
- </div>
+ <div class="small-12 column">
+ <hr> 


Could you remove the trailing whitespaces in this line? By the way, IMHO this line should be either indented or before the previous line (I'm not sure which one is right; you probably know it better).

javierm · 2018-09-27T11:43:17Z

app/views/admin/site_customization/pages/_form.html.erb

+ <div class="small-12 medium-6 large-3">
+ <%= f.submit class: "button success expanded" %>
+ </div>
+ </div>


Could you move this line one space to the left 😉? Sorry if I'm being a bit annoying about whitespace issues. Hope you understand we've got many contributors and sometimes we need to try to keep the codebase as consistent as possible.

javierm · 2018-09-27T11:55:04Z

app/views/admin/site_customization/pages/_form.html.erb

- <%= f.submit class: "button success expanded" %>
- </div>
+ <div class="small-12 column">
+ <hr> 


Thanks for the indentation fix! Would you mind removing the trailing whitespaces here 🙏? Thank you so much!

Sure! No problem. I revised with an option of the editor the trailing whitespaces but I think I am doing something wrong. I am going to checked again.

Ready! Thanks @javierm for your patience 🙏

javierm

Thank you for this one @papayalabs 🎉!

decabeza · 2018-09-27T16:18:57Z

spec/features/admin/site_customization/pages_spec.rb

@@ -11,7 +11,7 @@
 custom_page = create(:site_customization_page)
 visit admin_site_customization_pages_path

- expect(page).to have_content(custom_page.title)


As we decided to keep the title and the slug on /admin/site_customization/pages/index.html.erb could you keep both also in the spec? 🙏 😌

Thanks!

Of course @decabeza! I forgot to update the spec🐰I already added in all scenarios the changes. Thanks!!

decabeza · 2018-09-27T16:19:25Z

spec/features/admin/site_customization/pages_spec.rb

@@ -22,19 +22,18 @@
 click_link "Custom pages"
 end

- expect(page).not_to have_content "An example custom page"


Same the previous 😉

Ready ✌️

decabeza · 2018-09-27T16:19:38Z

spec/features/admin/site_customization/pages_spec.rb


 click_button "Create Custom page"

- expect(page).to have_content "An example custom page"


Same the previous 😉

Ready ✌️

javierm · 2018-10-03T11:23:24Z

Thank you so much @papayalabs 😉.

papayalabs · 2018-10-03T11:33:15Z

My pleasure! Thank you guys to make so nice to contribute in this project!

papayalabs mentioned this pull request Sep 20, 2018

Updates translatable custom pages #2839

Closed

javierm requested changes Sep 21, 2018

View reviewed changes

papayalabs force-pushed the 2741-updates-translatable-custom-pages-reloaded branch from 57e0971 to 110549d Compare September 24, 2018 06:32

javierm reviewed Sep 24, 2018

View reviewed changes

javierm reviewed Sep 26, 2018

View reviewed changes

papayalabs force-pushed the 2741-updates-translatable-custom-pages-reloaded branch from f8e592c to db79982 Compare September 26, 2018 14:40

javierm reviewed Sep 27, 2018

View reviewed changes

Updates translatable custom pages

914bfa6

papayalabs force-pushed the 2741-updates-translatable-custom-pages-reloaded branch from 18c340c to 914bfa6 Compare September 27, 2018 11:46

javierm reviewed Sep 27, 2018

View reviewed changes

Remove trailing whitespaces

5a4b9b0

javierm approved these changes Sep 27, 2018

View reviewed changes

decabeza reviewed Sep 27, 2018

View reviewed changes

Check title and slug in spec

3f9dea0

decabeza approved these changes Sep 28, 2018

View reviewed changes

javierm mentioned this pull request Sep 28, 2018

Migrate translatable data AyuntamientoMadrid/consul#1657

Merged

decabeza mentioned this pull request Sep 28, 2018

Improve admin custom pages #2937

Open

voodoorai2000 approved these changes Oct 3, 2018

View reviewed changes

javierm merged commit 132fea9 into consuldemocracy:master Oct 3, 2018

This was referenced Oct 3, 2018

[Upstream] Update translatable custom pages AyuntamientoMadrid/consul#1662

Merged

Updates translatable custom pages #2741

Closed

javierm mentioned this pull request Oct 5, 2018

[Backport] Update custom pages translations #2952

Merged

This was referenced Oct 17, 2018

Bring back CKEditor images button AyuntamientoMadrid/consul#1675

Merged

[Backport] Bring back CKEditor images button #2977

Merged

decabeza added this to Release 0.17 in Roadmap Oct 20, 2018


		click_button "Create Custom page"

		expect(page).to have_content "An example custom page"

Updates translatable custom pages #2913

Updates translatable custom pages #2913

Conversation

papayalabs commented Sep 20, 2018 • edited Loading

References

Visual Changes

Notes

papayalabs commented Sep 20, 2018

javierm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javierm Sep 21, 2018 • edited Loading

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

papayalabs Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

papayalabs Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papayalabs Sep 26, 2018 • edited Loading

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

papayalabs commented Sep 24, 2018

papayalabs commented Sep 24, 2018

javierm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papayalabs Sep 25, 2018 • edited Loading

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

decabeza Sep 25, 2018 • edited Loading

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

papayalabs Sep 25, 2018 • edited Loading

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

javierm Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

javierm Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javierm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papayalabs Sep 28, 2018 • edited Loading

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

javierm commented Oct 3, 2018

papayalabs commented Oct 3, 2018

papayalabs commented Sep 20, 2018 •

edited

Loading

javierm Sep 21, 2018 •

edited

Loading

papayalabs Sep 25, 2018 •

edited

Loading

papayalabs Sep 26, 2018 •

edited

Loading

papayalabs Sep 26, 2018 •

edited

Loading

papayalabs Sep 25, 2018 •

edited

Loading

decabeza Sep 25, 2018 •

edited

Loading

papayalabs Sep 25, 2018 •

edited

Loading

javierm Sep 27, 2018 •

edited

Loading

javierm Sep 27, 2018 •

edited

Loading

papayalabs Sep 28, 2018 •

edited

Loading