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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow the home page to have a path other than '/' #3368

Merged
merged 1 commit into from
May 23, 2018
Merged

Allow the home page to have a path other than '/' #3368

merged 1 commit into from
May 23, 2018

Conversation

anitagraham
Copy link
Contributor

No description provided.

@anitagraham
Copy link
Contributor Author

Strange: errors in a place this PR doesn't touch.

Failures:
  1) TranslatePages a page with a single locale can have a second locale added to it
     Failure/Error: expect(Refinery::Page.count).to eq(2)
     
       expected: 2
            got: 5
     
       (compared using ==)
     # ./pages/spec/features/refinery/admin/pages_spec.rb:827:in `block (3 levels) in <module:Admin>'
     # -e:1:in `<main>'

Reran rake a few times locally and on some runs saw other errors, each of which could be triggered if the three pages created by pages/spec/controllers/refinery/pages_controller_spec.rb remained in the db.

bricesanchez
bricesanchez requested changes May 18, 2018
View reviewed changes
Copy link
Member

@bricesanchez bricesanchez left a comment

Choose a reason for hiding this comment

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

Thanks @anitagraham ! It's a great and simple feature. Could you fix the specs?

pages/app/controllers/refinery/pages_controller.rb Outdated
@@ -101,7 +104,7 @@ def action_page_finder
module Finders
class Home
def self.call(_params)
Refinery::Page.find_by link_url: "/"
Refinery::Page.find_by link_url: Refinery::Pages.home_page_path || "/"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a fallback to "/" if the Refinery::Pages.home_page_path config has "/" as default ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Just making sure it happens somewhere.

pages/spec/controllers/refinery/pages_controller_spec.rb Outdated
@@ -2,21 +2,32 @@

module Refinery
describe PagesController, :type => :controller do
before do
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the before block ?

pages/spec/features/refinery/admin/pages_spec.rb Outdated
@@ -811,20 +813,19 @@ module Admin
describe "a page with a single locale" do
before do
allow(Refinery::I18n).to receive(:frontend_locales).and_return([:en, :lv])
Page.create :title => 'First Page'
# Page.create :title => 'First Page'
Copy link
Member

Choose a reason for hiding this comment

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

This code is commented

pages/app/controllers/refinery/pages_controller.rb
@@ -10,6 +10,9 @@ class PagesController < ::ApplicationController

# This action is usually accessed with the root path, normally '/'
def home
if page.link_url.present? && page.link_url != "/"
Copy link
Member

Choose a reason for hiding this comment

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

Do you also allow a redirect of the homepage if there is a path in link_url ? It's nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed as recommended

@anitagraham anitagraham force-pushed the master branch 2 times, most recently from 4fd892b to d779dba Compare May 22, 2018 04:43
@bricesanchez bricesanchez requested a review from parndt May 22, 2018 15:28
parndt
parndt reviewed May 23, 2018
View reviewed changes
pages/spec/controllers/refinery/pages_controller_spec.rb Outdated
end
end

describe "#show" do
render_views
Copy link
Member

Choose a reason for hiding this comment

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

how come this needs render_views now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because when I ran the tests it came back from this (and another similar test) saying the view was rendered with []. (from memory).

Copy link
Member

Choose a reason for hiding this comment

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

When I remove render_views from the file completely the tests still pass. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did the same (removed the 'render_views' and also got no failures. So out they go.

@anitagraham
Copy link
Contributor Author

anitagraham commented May 23, 2018 via email •
edited

@parndt parndt merged commit e618e14 into refinery:master May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parndt parndt parndt left review comments

@bricesanchez bricesanchez bricesanchez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@anitagraham @parndt @bricesanchez