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.

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

[Feature] [WEBSITE-1239] 404 page redesign #2253

Merged
merged 17 commits into from
Feb 21, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions app/assets/stylesheets/application_redesign.css.scss
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ z-index levels
@import 'redesign/style_session';
@import 'redesign/tables';
@import 'redesign/pages';
@import 'redesign/error';

// Checkout
@import 'redesign/pages/page-checkout';
Expand Down
26 changes: 26 additions & 0 deletions app/assets/stylesheets/redesign/_error.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Error Pages (Errors 404, 500 etc)
.error-page {
margin: 100px auto;

h1 {
font-size: 3.8em;
margin-top: 0;
}

h2 {
font-size: 3.2rem;
margin: 50px 0;
}

p {
font-size: 2rem;
font-weight: 300;
line-height: 150%;
margin: 0;
}

a {
@extend .link-underline;
}

}
16 changes: 16 additions & 0 deletions app/assets/stylesheets/redesign/generic/_helpers.scss
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,22 @@

}

.thin-line {

&--bottom::after {
content: "";
padding-bottom: 50px;
border-bottom: 1px solid $c-neutral-black;
width: 90px;
display: block;
}

&--alignCenter::after {
margin: 0 auto;
}

}

@media (min-width: $bootstrap-md-width) {

.no-margin-md {
Expand Down
8 changes: 2 additions & 6 deletions app/assets/stylesheets/redesign/pages/_why-us.scss
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,8 @@
}

.heading-thin-line-bottom::after {
content: "";
padding-bottom: 40px;
@extend .thin-line--bottom;
margin-bottom: 40px;
border-bottom: 1px solid $c-neutral-black;
width: 90px;
display: block;
}

@media (max-width: $bootstrap-md-width - 1px) {
Expand All @@ -63,7 +59,7 @@
}

}

}

}
1 change: 1 addition & 0 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class ApplicationController < ActionController::Base
include Concerns::UserCampaignable
include Concerns::AutomaticDiscount
include Concerns::Moodboards
include Concerns::Errors

# Marketing related concerns
include Marketing::Gtm::Controller::Container
Expand Down
49 changes: 49 additions & 0 deletions app/controllers/concerns/errors.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
module Concerns
module Errors
extend ActiveSupport::Concern

MYSTERIOUS_ROUTES = [ 'undefined', '/au/undefined', '1000668' ].freeze

included do
rescue_from ActiveRecord::RecordInvalid, with: -> { render_error(code: 422) }
rescue_from ActiveRecord::RecordNotFound, with: -> { render_error(code: 404) }
rescue_from ActionController::UnknownController, with: -> { render_error(code: 404) }
rescue_from ActionController::RoutingError, with: -> { render_error(code: 404) }
rescue_from AbstractController::ActionNotFound, with: -> { render_error(code: 404) }
end

def non_matching_request
path = params.fetch(:path, '')
format = params.fetch(:format, '')

if path.match(/\.php$/) || format.eql?('php')
head :not_acceptable, layout: false

elsif MYSTERIOUS_ROUTES.any? { |v| v.match(path) }
data = request.env.select {|key,_| key.upcase == key }
NewRelic::Agent.record_custom_event('UndefinedURL', data)
render text: 'Not Found', status: :not_found

else
# NOTE: Alexey Bobyrev 14 Jan 2017
# Raise error here to make it visible for application#rescue_from
# Ref: https://github.com/rails/rails/issues/671
raise ActionController::RoutingError.new(path)
end
end

protected

def render_error(code:)
if Rails.application.config.consider_all_requests_local
raise
else
respond_to do |format|
format.html { render "errors/#{code}", status: code }
format.all { render nothing: true, status: code }
end
end
end

end
end
9 changes: 0 additions & 9 deletions app/controllers/errors/invalid_format_controller.rb

This file was deleted.

14 changes: 0 additions & 14 deletions app/controllers/errors/mysterious_route_controller.rb

This file was deleted.

2 changes: 1 addition & 1 deletion app/controllers/products/collections_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ def collection_template
if page.page_is_lookbook? || @collection_options
page.template_path
else
{file: 'public/404', layout: false, status: :not_found}
raise ActiveRecord::RecordNotFound
end
end

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/spree/omniauth_callbacks_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ def failure
end

def passthru
render :file => "#{Rails.root}/public/404", :formats => [:html], :status => 404, :layout => false
raise ActiveRecord::RecordNotFound
end

def auth_hash
Expand Down
15 changes: 15 additions & 0 deletions app/views/errors/404.html.slim
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
- @title = "The page you were looking for doesn't exist (404)"

= render 'errors/analytics_javascript', locals: { code: 404, message: 'not found'}

.container
.col-xs-12
.error-page.text-center
h1
| Looking for something?
h2
| <em>Sorry,</em> this page doesn't exist.
p
| Try our #{link_to "homepage", root_path} to see what's new and start customizing.
p.thin-line--bottom.thin-line--alignCenter
| Still can't find what you're looking for? #{link_to "E-mail us", 'mailto:team@fameandpartners.com'} at team@fameandpartners.com.
15 changes: 15 additions & 0 deletions app/views/errors/422.html.slim
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
- @title = "The change you wanted was rejected (422)"

= render 'errors/analytics_javascript', locals: { code: 422, message: 'unprocessable entity'}

.container
.col-xs-12
.error-page.text-center
h1
| The change you wanted was rejected.
h2
| Maybe you tried to change something you didn't have access to.
p
| Try our #{link_to "homepage", root_path} to see what's new and start customizing.
p.thin-line--bottom.thin-line--alignCenter
| Still can't find what you're looking for? #{link_to "E-mail us", 'mailto:team@fameandpartners.com'} at team@fameandpartners.com.
18 changes: 18 additions & 0 deletions app/views/errors/_analytics_javascript.html.slim
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
- code = local_assigns[:code]
- message = local_assigns[:message]

javascript:
(function (i, s, o, g, r, a, m) {
i['GoogleAnalyticsObject'] = r;
i[r] = i[r] || function () {
(i[r].q = i[r].q || []).push(arguments)
}, i[r].l = 1 * new Date();
a = s.createElement(o),
m = s.getElementsByTagName(o)[0];
a.async = 1;
a.src = g;
m.parentNode.insertBefore(a, m)
})(window, document, 'script', '//www.google-analytics.com/analytics.js', 'ga');
ga('create', 'UA-41247818-1', 'auto');
ga('send', 'pageview');
ga('send', 'event', "#{code}", "#{message}");
17 changes: 7 additions & 10 deletions config/routes.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
FameAndPartners::Application.routes.draw do

############################
# Devise Omniauth Workaround
############################
Expand Down Expand Up @@ -503,14 +502,6 @@
end
end

#################
# Mysterious URLs
#################

get '/undefined', to: 'errors/mysterious_route#undefined'
get '/au/undefined', to: 'errors/mysterious_route#undefined'
get '/1000668', to: 'errors/mysterious_route#undefined'

#########
# Widgets
#########
Expand Down Expand Up @@ -624,6 +615,12 @@
mount Revolution::Engine => '/'

mount WeddingAtelier::Engine, at: '/wedding-atelier'
end

match '*path', to: 'errors/invalid_format#capture_php', constraints: lambda { |request| request.path[/\.php$/] }
# NOTE: Alexey Bobyrev 14 Feb 2017
# Method append used here to handle all request directly right after defined ones (including engines)
FameAndPartners::Application.routes.append do
# NOTE: Alexey Bobyrev 14 Jan 2017
# Any other routes are handled here (as ActionDispatch prevents RoutingError from hitting ApplicationController#rescue_action)
match '*path', to: 'application#non_matching_request', as: 'routing_error'
end
57 changes: 0 additions & 57 deletions public/404.html

This file was deleted.

26 changes: 0 additions & 26 deletions public/422.html

This file was deleted.

27 changes: 0 additions & 27 deletions spec/controllers/mysterious_route_controller_spec.rb

This file was deleted.

3 changes: 2 additions & 1 deletion spec/controllers/products/collections_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@
describe 'returns a 404 status' do
before(:each) do
allow(controller).to receive(:parse_permalink).and_return(nil)
allow(Rails.application.config).to receive(:consider_all_requests_local).and_return(false)
end

it 'when querying a inexistent permalink' do
get :show, permalink: 'nothing'
expect(response).to render_template(file: 'public/404', layout: false)
expect(response).to render_template(file: 'errors/404', layout: 'redesign/application')
expect(response).to have_http_status(:not_found)
end
end
Expand Down