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

Add html-validation to detect malformed markup #15433

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ language: ruby
cache: bundler
before_install:
- echo -e "machine github.com\n login $GITHUB_ACCESS_TOKEN" >> ~/.netrc
addons:
apt:
packages:
- cmake
before_install:
- scripts/install_tidy.sh
script:
- bundle exec rake test:all
- bundle exec rake rubocop
Expand All @@ -15,4 +21,5 @@ notifications:
secure: GVD9d+kwR5hzab5ZnWugbCkp9QSYyheSrABWkD+LmpMcWcx7jijajSn4LLvDi/zHYn1MdOBcPe08hSygmpm7ViUApp0EJcSzE4BLU/5oAs+ANV0Qq6jsssMlyo3v8eRAqHNiLxAiAsz+lc0EZWfQnSW8kHzzbO3NeYq1NRL5CgQ=
env:
global:
secure: cwq0zTDtALPr4nm29EgvQ6v5oIzYsE+DEP3Vt4unrTuv84JywWu2mry+YiXLCmcermD433BG5Fy/E+MUXAXiGjMtJr6wqkVe8HlDH3xOCJ4LBzR/hrE1x5Ufke6UmHCdpcWlSTNO8eQDP1k/X2Pz6Zng2TlmenLwGwUNZ82O5vM=
- secure: cwq0zTDtALPr4nm29EgvQ6v5oIzYsE+DEP3Vt4unrTuv84JywWu2mry+YiXLCmcermD433BG5Fy/E+MUXAXiGjMtJr6wqkVe8HlDH3xOCJ4LBzR/hrE1x5Ufke6UmHCdpcWlSTNO8eQDP1k/X2Pz6Zng2TlmenLwGwUNZ82O5vM=
- PATH="$PATH:$HOME/bin"
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ group :test do
gem 'rack-test'
gem 'webmock'
gem 'bundler-audit'
gem 'html_validation'
end

group :quality do
Expand Down
2 changes: 2 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ GEM
ruby_parser (~> 3.1, > 3.1.0)
sexp_processor (~> 4.4)
hashdiff (0.3.0)
html_validation (1.1.3)
ice_nine (0.11.2)
iso_country_codes (0.7.1)
json (1.8.2)
Expand Down Expand Up @@ -121,6 +122,7 @@ DEPENDENCIES
everypolitician (~> 0.18.0)!
everypolitician-popolo!
flog
html_validation
iso_country_codes
json
minitest
Expand Down
26 changes: 17 additions & 9 deletions public/javascript/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -305,15 +305,23 @@ $(function(){
});

// http://baymard.com/labs/country-selector
$('.js-select-to-autocomplete').selectToAutocomplete().on('change', function(){
var v = $(this).val();
if (v) {
// Assumes the <option>'s `value` attribute is a URL slug for the country
window.location.href = '/' + v + '/';
}
}).on('focus', function(){
$(this).next().trigger("focus");
});
$('.js-select-to-autocomplete')
.attr({
placeholder: 'Search by country name',
autocorrect: 'off',
autocomplete: 'off'
})
.selectToAutocomplete()
.on('change', function(){
var v = $(this).val();
if (v) {
// Assumes the <option>'s `value` attribute is a URL slug for the country
window.location.href = '/' + v + '/';
}
})
.on('focus', function(){
$(this).next().trigger("focus");
});

// Fix the incorrect default autocomplete width, which meant that
// autocomplete menu was longer than the search input it's linked to.
Expand Down
12 changes: 12 additions & 0 deletions scripts/install_tidy.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#!/usr/bin/env bash

set -eo pipefail

cd /tmp
wget https://github.com/htacg/tidy-html5/archive/5.2.0.zip
unzip 5.2.0.zip
cd tidy-html5-5.2.0/build/cmake
cmake ../..
make
mkdir ~/bin
mv tidy ~/bin/tidy
6 changes: 6 additions & 0 deletions t/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
require 'pry'
require 'webmock/minitest'
require 'everypolitician'
require 'html_validation'

module Minitest
class Spec
Expand Down Expand Up @@ -55,6 +56,11 @@ def stub_github_api
stub_json('https://api.github.com/repos/everypolitician/everypolitician-data/issues?labels=New%20Country,3%20-%20WIP&per_page=100')
end

def last_response_must_be_valid
validation = PageValidations::HTMLValidation.new.validation(last_response.body, last_request.url)
assert validation.valid?, validation.exceptions
end

private

def ep_repo
Expand Down
9 changes: 9 additions & 0 deletions t/web/basic.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,4 +66,13 @@
last_response.status.must_equal 404
end
end

describe 'HTML validation' do
before { get '/' }

it 'has no errors in the home page' do
skip if `which tidy`.empty?
last_response_must_be_valid
end
end
end
13 changes: 13 additions & 0 deletions t/web/countries.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# frozen_string_literal: true
require 'test_helper'
require_relative '../../app'

describe 'Countries' do
describe 'HTML validation' do
it 'has no errors in the countries page' do
skip if `which tidy`.empty?
get '/countries.html'
last_response_must_be_valid
end
end
end
8 changes: 8 additions & 0 deletions t/web/country.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,12 @@
links.last.text.must_equal '/united-states-of-america/senate/download.html'
end
end

describe 'HTML validation' do
it 'has no errors in the country page' do
skip if `which tidy`.empty?
get '/estonia/'
last_response_must_be_valid
end
end
end
7 changes: 7 additions & 0 deletions t/web/house.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,11 @@
subject.css('.avatar-unit p').last.text.must_equal '1981-01-06 - 1983-01-03'
end
end

describe 'HTML validation' do
it 'has no errors in the house page' do
skip if `which tidy`.empty?
last_response_must_be_valid
end
end
end
7 changes: 7 additions & 0 deletions t/web/house_download.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,11 @@
subject.css('#term-senate-113 p').text.strip.must_include '2013-01-06 - 2015-01-03'
end
end

describe 'HTML validation' do
it 'has no errors in the house/download page' do
skip if `which tidy`.empty?
last_response_must_be_valid
end
end
end
9 changes: 9 additions & 0 deletions t/web/term_table/nz.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,13 @@
subject.css('#term h1').text.must_include '51st Parliament'
end
end

describe 'HTML validation' do
before { get '/new-zealand/house/term-table/51.html' }

it 'has no errors in the term-table page' do
skip if `which tidy`.empty?
last_response_must_be_valid
end
end
end
6 changes: 3 additions & 3 deletions views/country.erb
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,10 @@

<a href="http://gender-balance.org" class="gender-balance-promo__demo">
<div class="screenshot-wrapper">
<img src="/images/screencast.gif" width="232" height="330">
<img src="/images/screencast.gif" width="232" height="330" alt="Gender-Balance app preview">
</div>
</div>
</a>
</a>
</div>

</div>
</div>
2 changes: 1 addition & 1 deletion views/country_selector.erb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<select class="field js-select-to-autocomplete" placeholder="Search by country name" name="Country" id="country-selector" autofocus="autofocus" autocorrect="off" autocomplete="off">
<select class="field js-select-to-autocomplete" name="Country" id="country-selector" autofocus="autofocus">
<option value="" selected="selected">Select Country</option>
<% @page.all_countries.each do |country| %>
<option value="<%= country.slug %>" data-alternative-spellings="<%= country.names %>" title="<%= commify(country.total_people) %> <% if country.total_people == 1 %>person<% else %>people<% end %>"><%= country.name %></option>
Expand Down
6 changes: 3 additions & 3 deletions views/house.erb
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@

<a href="http://gender-balance.org" class="gender-balance-promo__demo">
<div class="screenshot-wrapper">
<img src="/images/screencast.gif" width="232" height="330">
<img src="/images/screencast.gif" width="232" height="330" alt="Gender-Balance app preview">
</div>
</div>
</a>
</a>
</div>

</div>
</div>
8 changes: 4 additions & 4 deletions views/layout.erb
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
<a href="/"><span class="header-logo">EveryPolitician</span></a>
<% end %>
</h2>
<% if @house %><h3><a href="/<%= @country.slug.downcase %>/<%= @house.slug.downcase %>/"><%= @house.name %></h3><% end %>
<% if @house %><h3><a href="/<%= @country.slug.downcase %>/<%= @house.slug.downcase %>/"><%= @house.name %></a></h3><% end %>
</div>
<div class="site-header__navigation site-header__navigation--primary">
<nav role="navigation">
Expand Down Expand Up @@ -115,7 +115,7 @@
<div class="column-one-quarter">
<div class="mysoc-footer__donate">
<p>Your donations keep this site and others like it running</p>
<a href="https://www.mysociety.org/donate?utm_source=everypolitician.org&utm_content=footer+donate+now&utm_medium=link&utm_campaign=mysoc_footer" class="mysoc-footer__donate__button">Donate now</a>
<a href="https://www.mysociety.org/donate?utm_source=everypolitician.org&amp;utm_content=footer+donate+now&amp;utm_medium=link&amp;utm_campaign=mysoc_footer" class="mysoc-footer__donate__button">Donate now</a>
</div>
</div>

Expand All @@ -127,14 +127,14 @@
<div class="mysoc-footer__orgs">
<p class="mysoc-footer__org">
Built by
<a href="https://www.mysociety.org?utm_source=everypolitician.org&utm_content=footer+logo&utm_medium=link&utm_campaign=mysoc_footer" class="mysoc-footer__org__logo mysoc-footer__org__logo--mysociety">mySociety</a>
<a href="https://www.mysociety.org?utm_source=everypolitician.org&amp;utm_content=footer+logo&amp;utm_medium=link&amp;utm_campaign=mysoc_footer" class="mysoc-footer__org__logo mysoc-footer__org__logo--mysociety">mySociety</a>
</p>
</div>
</div>

<div class="column-one-third">
<div class="mysoc-footer__legal">
<p>mySociety Limited is a project of UK Citizens Online Democracy, a registered charity in England and Wales. For full details visit <a href="https://www.mysociety.org?utm_source=everypolitician.org&utm_content=footer+full+logo+details&utm_medium=link&utm_campaign=mysoc_footer">mysociety.org</a>.</p>
<p>mySociety Limited is a project of UK Citizens Online Democracy, a registered charity in England and Wales. For full details visit <a href="https://www.mysociety.org?utm_source=everypolitician.org&amp;utm_content=footer+full+logo+details&amp;utm_medium=link&amp;utm_campaign=mysoc_footer">mysociety.org</a>.</p>
</div>
</div>

Expand Down
9 changes: 6 additions & 3 deletions views/term_table.erb
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,13 @@
<img src="/images/person-placeholder-108px.png"
data-src="<%= person.proxy_image %>"
style="display: none"
class="person-card__image">
<noscript><img src="<%= person.proxy_image %>" class="person-card__image"></noscript>
class="person-card__image"
alt="Member headshot">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a plain "Member headshot" sufficient, when there might be hundreds of these across the page? Perhaps we should include the member's name here?

<noscript><img src="<%= person.proxy_image %>" class="person-card__image"
alt="Member headshot"></noscript>
<% else %>
<img src="/images/person-placeholder-108px.png" class="person-card__image">
<img src="/images/person-placeholder-108px.png" class="person-card__image"
alt="Placeholder image">
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto here. "Placeholder image for Freddy McFreedy" seems like it might be more useful.

<% end %>

<h3 class="person-card__name"><%= person.name %></h3>
Expand Down