Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add notes to history #85

Merged
merged 7 commits into from

2 participants

bishboria Jake Benilov
bishboria

Don't merge until is alphagov/gds-api-adapters#125 is accepted and version bumped, and the version of gds-api-adapters updated in the Gemfile.

app/controllers/notes_controller.rb
@@ -0,0 +1,18 @@
+require 'gds_api/need_api'
+require 'plek'
+require 'json'
+
+class NotesController < ApplicationController
+ def create
+ text = params["notes"]["text"]
+ need_id = params["need_id"]
+ @note = Note.new(text, need_id, current_user)
+
+ if @note.save
+ flash[:notice] = "Note saved"
+ else
+ flash[:error] = "Error saving note"
Jake Benilov Collaborator

This is a bit cryptic; now that the API returns specific errors, could these be displayed to the user?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jake Benilov benilovj commented on the diff
test/fixtures/needs/101350.json
@@ -67,7 +89,18 @@
"goal": [ "apply for a school place", "apply for a secondary school place" ],
"role": [ "grandparent", null ]
},
- "created_at": "2013-01-01T13:00:00+00:00"
+ "created_at": "2013-01-01T13:00:00+00:00",
+ "notes": [
+ {
+ "text": "oops",
+ "author": {
+ "name": "Goofy",
+ "email": null,
+ "uid": null
+ },
+ "created_at": "2013-01-02T13:00:00+00:00"
+ }
+ ]
Jake Benilov Collaborator

I'm really not a fan of using fixtures for test data; this ought to be created dynamically by builder code. But that's a bigger change and not part of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
bishboria

@benilovj Updated the code and tests based on exposed errors from Need API

Jake Benilov benilovj merged commit ff706a4 into from
Jake Benilov benilovj deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
2  Gemfile
View
@@ -45,5 +45,5 @@ gem 'formtastic-bootstrap', '2.1.3'
if ENV['API_DEV']
gem 'gds-api-adapters', path: '../gds-api-adapters'
else
- gem 'gds-api-adapters', '8.4.0'
+ gem 'gds-api-adapters', '9.2.0'
end
4 Gemfile.lock
View
@@ -83,7 +83,7 @@ GEM
formtastic-bootstrap (2.1.3)
formtastic (~> 2.2)
fssm (0.2.10)
- gds-api-adapters (8.4.0)
+ gds-api-adapters (9.2.0)
link_header
lrucache (~> 0.1.1)
null_logger
@@ -237,7 +237,7 @@ DEPENDENCIES
factory_girl_rails (= 4.2.1)
formtastic (= 2.2.1)
formtastic-bootstrap (= 2.1.3)
- gds-api-adapters (= 8.4.0)
+ gds-api-adapters (= 9.2.0)
gds-sso (= 9.2.2)
jquery-rails
kaminari (= 0.14.1)
18 app/controllers/notes_controller.rb
View
@@ -0,0 +1,18 @@
+require 'gds_api/need_api'
+require 'plek'
+require 'json'
+
+class NotesController < ApplicationController
+ def create
+ text = params["notes"]["text"]
+ need_id = params["need_id"]
+ @note = Note.new(text, need_id, current_user)
+
+ if @note.save
+ flash[:notice] = "Note saved"
+ else
+ flash[:error] = "Note couldn't be saved: #{@note.errors}"
+ end
+ redirect_to revisions_need_path(need_id)
+ end
+end
34 app/models/note.rb
View
@@ -0,0 +1,34 @@
+class Note
+
+ attr_reader :text, :need_id, :author
+ attr_reader :errors
+
+ def initialize(text, need_id, author)
+ @text = text
+ @need_id = need_id
+ @author = author
+ end
+
+ def save
+ note_atts = {
+ "text" => text,
+ "need_id" => need_id,
+ "author" => author_atts(author)
+ }
+ Maslow.need_api.create_note(note_atts)
+ true
+ rescue GdsApi::HTTPErrorResponse => err
+ @errors = err.error_details["errors"].first
+ false
+ end
+
+ private
+
+ def author_atts(author)
+ {
+ "name" => author.name,
+ "email" => author.email,
+ "uid" => author.uid
+ }
+ end
+end
2  app/views/needs/_need_header.html.erb
View
@@ -17,7 +17,7 @@
"View" => need_path(@need),
"Edit" => edit_need_path(@need),
"Actions" => actions_need_path(@need),
- "History" => revisions_need_path(@need)
+ "History & Notes" => revisions_need_path(@need)
}
tabs.delete("Edit") if @need.duplicate_of.present?
bootstrap_flavour_tabs(tabs, active: active) %>
12 app/views/needs/_revision.html.erb
View
@@ -1,3 +1,15 @@
+<% revision.notes.each do |n| %>
+ <li class="revision">
+ <h3 class="revision-title">
+ <%= Time.parse(n.created_at).strftime("%-d %B %Y, %H:%M") %> &mdash;
+ Note by <%= n.author.name %>
+ <% if n.author.email.present? %>
+ &lt;<%= n.author.email %>&gt;
+ <% end %>
+ </h3>
+ <%= n.text %>
+ </li>
+<% end %>
<li class="revision">
<h3 class="revision-title">
31 app/views/needs/revisions.html.erb
View
@@ -1,11 +1,11 @@
-<% content_for :page_title, "History" %>
+<% content_for :page_title, "History & Notes" %>
<article class="need">
<nav class="add-top-margin">
<ul class="breadcrumb">
<li><%= link_to "All needs", needs_path %> <span class="divider">/</span></li>
<li><%= link_to "#{@need.need_id}: #{format_need_goal @need.goal}", need_path(@need) %> <span class="divider">/</span></li>
- <li class="active">History</li>
+ <li class="active">History &amp; Notes</li>
</ul>
</nav>
@@ -15,13 +15,30 @@
need_path(@need.duplicate_of) %>
</p>
<% else %>
- <%= render :partial => "need_header", locals: { active: "History" } %>
+ <%= render :partial => "need_header", locals: { active: "History & Notes" } %>
<% end %>
- <h3>Recent changes</h3>
- <ul class="revisions">
- <%= render partial: "revision", collection: @need.revisions %>
- </ul>
+ <div class="row-fluid">
+ <div id="notes" class="span6">
+ <%= semantic_form_for(:notes, url: notes_url) do |f| %>
+ <%= f.inputs do %>
+ <%= hidden_field_tag :need_id, @need.need_id %>
+ <%= f.input :text, :label => 'Note', :as => :text, :hint => "Don't forget to save your note when you have finished.", input_html: { class: "span12" } %>
+ <% end %>
+ <%= f.action :submit,
+ :button_html => {
+ :value => "Add note",
+ :class => "btn btn-primary"
+ } %>
+ <% end %>
+ </div>
+ <div id="revision-history" class="span6 accordion">
+ <h3>Recent changes</h3>
+ <ul class="revisions">
+ <%= render partial: "revision", collection: @need.revisions %>
+ </ul>
+ </div>
+ </div>
<%= render partial: "workflow_buttons", locals: { edit_page: false } %>
</article>
2  config/routes.rb
View
@@ -4,6 +4,8 @@
get :bookmarklet, controller: :bookmarklet, path: 'bookmarklet'
+ resources :notes, only: [:create]
+
resources :needs, except: [:destroy], constraints: { id: /[0-9]+/ } do
member do
get :revisions
39 test/fixtures/needs/101350.json
View
@@ -46,7 +46,18 @@
"goal": [ "apply for a secondary school place" ,"apply for a primary school place" ],
"role": [ null, "parent" ]
},
- "created_at": "2013-05-01T13:00:00+00:00"
+ "created_at": "2013-05-01T13:00:00+00:00",
+ "notes": [
+ {
+ "text": "hello",
+ "author": {
+ "name": "Donald Duck",
+ "email": null,
+ "uid": null
+ },
+ "created_at": "2013-05-02T13:00:00+00:00"
+ }
+ ]
},
{
"action_type": "update",
@@ -54,7 +65,18 @@
"changes": {
"legislation": [ "foo", "bar" ]
},
- "created_at": "2013-04-01T13:00:00+00:00"
+ "created_at": "2013-04-01T13:00:00+00:00",
+ "notes": [
+ {
+ "text": "goodbye",
+ "author": {
+ "name": "Minnie Mouse",
+ "email": null,
+ "uid": null
+ },
+ "created_at": "2013-04-03T13:00:00+00:00"
+ }
+ ]
},
{
"action_type": "create",
@@ -67,7 +89,18 @@
"goal": [ "apply for a school place", "apply for a secondary school place" ],
"role": [ "grandparent", null ]
},
- "created_at": "2013-01-01T13:00:00+00:00"
+ "created_at": "2013-01-01T13:00:00+00:00",
+ "notes": [
+ {
+ "text": "oops",
+ "author": {
+ "name": "Goofy",
+ "email": null,
+ "uid": null
+ },
+ "created_at": "2013-01-02T13:00:00+00:00"
+ }
+ ]
Jake Benilov Collaborator

I'm really not a fan of using fixtures for test data; this ought to be created dynamically by builder code. But that's a bigger change and not part of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
]
}
46 test/functional/notes_controller_test.rb
View
@@ -0,0 +1,46 @@
+require_relative '../integration_test_helper'
+require 'gds_api/test_helpers/need_api'
+
+class NotesControllerTest < ActionController::TestCase
+ include GdsApi::TestHelpers::NeedApi
+
+ setup do
+ login_as stub_user
+ @note_atts = {
+ "notes" => {
+ "text" => "test"
+ },
+ "need_id" => "100001"
+ }
+ end
+
+ context "POST create" do
+ should "be successful" do
+ stub_create_note(
+ "text" => "test",
+ "need_id" => "100001",
+ "author" => {
+ "name" => stub_user.name,
+ "email" => stub_user.email,
+ "uid" => stub_user.uid
+ }
+ )
+
+ post :create, @note_atts
+
+ assert_redirected_to revisions_need_path("100001")
+ assert_equal "Note saved", @controller.flash[:notice]
+ refute @controller.flash[:error]
+ end
+
+ should "return an error message if the save fails" do
+ Note.any_instance.expects(:save).returns(false)
+ Note.any_instance.expects(:errors).returns("Text can't be blank")
+
+ post :create, @note_atts
+
+ assert_equal "Note couldn't be saved: Text can't be blank", @controller.flash[:error]
+ refute @controller.flash[:notice]
+ end
+ end
+end
30 test/integration/view_a_need_test.rb
View
@@ -40,7 +40,7 @@ class ViewANeedTest < ActionDispatch::IntegrationTest
within ".nav-tabs" do
assert page.has_link?("Edit", href: "/needs/101350/edit")
- assert page.has_link?("History", href: "/needs/101350/revisions")
+ assert page.has_link?("History & Notes", href: "/needs/101350/revisions")
end
within ".the-need" do
@@ -92,12 +92,12 @@ class ViewANeedTest < ActionDispatch::IntegrationTest
visit "/needs"
click_on "101350"
- click_on "History"
+ click_on "History & Notes"
within ".breadcrumb" do
assert page.has_link?("All needs", href: "/needs")
assert page.has_link?("101350: Book a driving test", href: "/needs/101350")
- assert page.has_content?("History")
+ assert page.has_content?("History & Notes")
end
within ".need" do
@@ -119,9 +119,15 @@ class ViewANeedTest < ActionDispatch::IntegrationTest
end
within ".revisions" do
- assert_equal 3, page.all(".revision").count
+ assert_equal 6, page.all(".revision").count
within ".revision:nth-child(1)" do
+ assert page.has_content?("Note by Donald Duck")
+ assert page.has_content?("2 May 2013, 13:00")
+ assert page.has_content?("hello")
+ end
+
+ within ".revision:nth-child(2)" do
assert page.has_content?("Update by Mickey Mouse <mickey.mouse@test.com>")
assert page.has_content?("1 May 2013, 13:00")
@@ -133,13 +139,25 @@ class ViewANeedTest < ActionDispatch::IntegrationTest
end
end
- within ".revision:nth-child(2)" do
+ within ".revision:nth-child(3)" do
+ assert page.has_content?("Note by Minnie Mouse")
+ assert page.has_content?("3 April 2013, 13:00")
+ assert page.has_content?("goodbye")
+ end
+
+ within ".revision:nth-child(4)" do
assert page.has_content?("Update by unknown author")
assert page.has_no_content?("<>") # catch missing email
assert page.has_content?("1 April 2013, 13:00")
end
- within ".revision:nth-child(3)" do
+ within ".revision:nth-child(5)" do
+ assert page.has_content?("Note by Goofy")
+ assert page.has_content?("2 January 2013, 13:00")
+ assert page.has_content?("oops")
+ end
+
+ within ".revision:nth-child(6)" do
assert page.has_content?("Create by Donald Duck")
assert page.has_no_content?("<>") # catch an empty email string
assert page.has_content?("1 January 2013, 13:00")
39 test/unit/note_test.rb
View
@@ -0,0 +1,39 @@
+require_relative '../test_helper'
+require 'gds_api/test_helpers/need_api'
+
+class NoteTest < ActiveSupport::TestCase
+ include GdsApi::TestHelpers::NeedApi
+
+ setup do
+ @author = OpenStruct.new(
+ name: "Winston Smith-Churchill",
+ email: "winston@alphagov.co.uk",
+ uid: "win5t0n"
+ )
+ end
+
+ should "send note data to the need api" do
+ stub_create_note(
+ "text" => "test",
+ "need_id" => "100001",
+ "author" => {
+ "name" => "Winston Smith-Churchill",
+ "email" => "winston@alphagov.co.uk",
+ "uid" => "win5t0n"
+ }
+ )
+
+ Note.new("test", "100001", @author).save
+ end
+
+ should "have errors set if the note couldn't be saved" do
+ GdsApi::NeedApi.any_instance.expects(:create_note).raises(
+ GdsApi::HTTPErrorResponse.new(422, {"errors" => ["error"]})
+ )
+
+ note = Note.new("", "100001", @author)
+ note.save
+
+ assert_equal "error", note.errors
+ end
+end
Something went wrong with that request. Please try again.