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

Hide irrelevant URL fields in Mapping form [57569274] #36

Merged
merged 6 commits into from Oct 8, 2013

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Oct 7, 2013

  • Use JavaScript to toggle visibility of archive and redirect form fields
  • Upgrade jQuery gem and jQuery to 1.10.2
  • Add Jasmine gem for JavaScript testing
  • Add Jasmine CI task to default rake command
Paul Hayes added 5 commits Oct 3, 2013
* Upgrade from 2.0.2 to 3.0.4
* Start from a newer jQuery version (1.10.2)
* Supported by Bootstrap 2.3.2
* We aren't using these features yet
* Add 2.0.0 rc3 gem and run 'rails g jasmine:install'
* Jasmine will be used for JavaScript feature testing, with a
dependency on PhantomJS
* Create a GOVUK.Mappings.edit function which hides/shows the
appropriate form fields on load, and whenever the http status changes
* This simplifies the form and clarifies which fields are needed for
which http status type
* Use fieldsets to determine which fields to hide/show, using js
prefixed class names
* Add Jasmine test spec
* Run JavaScript tests by default
@@ -4,7 +4,7 @@ source 'https://BnrJb6FZyzspBboNJzYZ@gem.fury.io/govuk/'
gem 'rails', '3.2.13'
gem 'unicorn', '4.6.2'
gem 'mysql2', '0.3.13'
gem 'jquery-rails', '2.0.2' # TODO: Newer versions break publisher sortable parts. Will need attention.

This comment has been minimized.

@jamiecobbett

jamiecobbett Oct 7, 2013
Contributor

Death to obsolete comments! :)

@ghost ghost assigned rgarner Oct 7, 2013
@rgarner

This comment has been minimized.

Copy link
Contributor

@rgarner rgarner commented on spec/javascripts/spec/mappings.spec.js in 39af5ee Oct 8, 2013

This worries me. The markup in the view ERB could change and these tests will continue to pass. This makes these tests partly design-time tests which are only suitable for regression if the Javascript is edited, not the markup. Is there a way to render the ERB here?

@rgarner
Copy link
Contributor

@rgarner rgarner commented Oct 8, 2013

My main concern above still stands - these tests will continue to pass and may give the impression that JS on mappings editing is working when they may not be due to a markup change. @fofr says he'll introduce erb fixtures later as part of bringing in jasmine-query - @jamiecobbett , if you want to merge on the basis that this is coming and we'll be coupling mappings.spec.js to the mappings/_form.html.erb partial in due course, please go ahead.

@jamiecobbett
Copy link
Contributor

@jamiecobbett jamiecobbett commented Oct 8, 2013

I think @rgarner is correct. These tests are good as unit tests of the Javascript, but we need an integration test probably in Cucumber and using a headless browser to test whether or not the Javascript successfully integrates.

@jamiecobbett
Copy link
Contributor

@jamiecobbett jamiecobbett commented Oct 8, 2013

There's an example of us using Jasmine to unit test and rspec/Capybara (for this app we should use Cucumber) to provide an integration test in Frontend here:
https://github.com/alphagov/frontend/blob/master/test/javascripts/unit/foreign-travel-advice-test.js and https://github.com/alphagov/frontend/blob/master/test/integration/travel_advice_test.rb#L57-L87

For context, it's related to this page: https://www.gov.uk/foreign-travel-advice

* Include poltergeist to allow cucumber to run PhantomJS (and hence
JavaScript)
* Completes integration testing for dynamic mapping fields
@@ -27,6 +27,7 @@ group :assets do
end

group :test do
gem 'poltergeist'

This comment has been minimized.

@jamiecobbett

jamiecobbett Oct 8, 2013
Contributor

Please pin the version as per our styleguide.

jamiecobbett added a commit that referenced this pull request Oct 8, 2013
Hide irrelevant URL fields in Mapping form [57569274]
@jamiecobbett jamiecobbett merged commit 3b2da64 into master Oct 8, 2013
@jamiecobbett jamiecobbett deleted the dynamic-mapping-fields branch Oct 8, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.