Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add notes to history #54

Merged
merged 10 commits into from

4 participants

@bishboria

No description provided.

@bishboria bishboria referenced this pull request in alphagov/gds-api-adapters
Merged

Add create_note method for need api #125

@fatbusinessman

I may have misinterpreted, or missed out on some extra context, but I thought these notes were going to be more analogous to commit messages, so they get attached to a revision when the need is saved.

@bishboria

After discussing the details of the feature with @wryobservations it became clear that it isn't generally necessary that a comment needs to be given on an edit but it may spark further conversation. If users are forced to enter a comment on every action, it may reduce the quality of the comment made.

Me and @vinayvinay were discussed it and this seemed like a reasonable solution: a note could be part of a bigger discussion around edits/decisions that have been made about a need.

app/controllers/notes_controller.rb
@@ -0,0 +1,16 @@
+class NotesController < ApplicationController
+ def create
+ note = Note.new(filtered_params)
+ if note.save
+ render nothing: true, status: 201

IMHO head :created seems more explicit and also encapsulates the magic number.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
app/models/changeset.rb
((5 lines not shown))
- def initialize(current, previous)
+ def initialize(current, previous, notes)

you can save on passing the nil value for notes as a parameter if you give notes a default value of [] and make it optional. i see, you're doing the same a bit later on. why not here ?

def initialize(current, previous, notes=[])

not all changesets have notes, that's also a good reason for it to be an optional parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vinayvinay vinayvinay commented on the diff
test/unit/presenters/changeset_presenter_test.rb
@@ -34,5 +45,12 @@ class ChangesetPresenterTest < ActiveSupport::TestCase
assert_equal Time.parse("2013-01-01"), response[:created_at]
assert_equal ["user", "home owner"], response[:changes][:role]
+
+ note = response[:notes][0]
+ assert_equal "test", note[:text]
+ assert_equal "Sir John Anderson", note[:author][:name]
+ assert_equal "jock@alphagov.co.uk", note[:author][:email]
+ assert_equal "j0hn", note[:author][:uid]
+ assert_equal Time.parse("2013-01-02"), note[:created_at]

can you split these note assertions into a separate should block ? i feel notes cannot be considered as basic attributes of a need.

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

LGTM. i'll just wait for @fatbusinessman to confirm if he's okay with the notes functionality as is implemented.

app/models/note.rb
@@ -0,0 +1,17 @@
+class Note
+ include Mongoid::Document
+ include Mongoid::Timestamps
+
+ field :text, type: String
+ field :need_id, type: Integer
+ field :author, type: Hash
+ field :revision, type: String
+
+ default_scope order_by([:_id, :desc])

Is this going to do what you want? I can’t see where this would be assigned anything other than the default Mongo ID, which isn’t strictly ordered.

_id contains an embedded timestamp of when the item was generated… So this does order by decreasing time.

I'm happy to change it to use the created_at field if you think it'd be clearer.

@fatbusinessman - i had a similar concerns, but it turns out mongoid also relies on _id field for ordering: http://mongoid.org/en/mongoid/docs/querying.html#queries mentions using _id.

I think it’s better to be explicit and use created_at, since we’ve got it already. Or we might be able to sidestep the problem entirely: see my other comment.

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

I wonder, would it make sense to embed the notes within the revisions themselves? They don’t really make much sense outside the revisions they’re made on, and it would save us making a query for every revision on a need.

@bishboria

I'm not so sure that one query on every revision is such a big deal, considering the small amount of data…

Also, having a separate note entity allows us to use it in other features with no further changes to the revision code.

app/controllers/notes_controller.rb
@@ -0,0 +1,16 @@
+class NotesController < ApplicationController
+ def create
+ note = Note.new(filtered_params)
+ if note.save
+ head :created
+ else
+ error 422, message: "Something went wrong"
@benilovj Collaborator

it'd be good to return the validation errors here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@benilovj benilovj commented on the diff
app/models/note.rb
@@ -0,0 +1,17 @@
+class Note
@benilovj Collaborator

I think that this class should have some validations

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

@benilovj I've pushed those changes

@benilovj benilovj merged commit 5deb520 into master
@benilovj benilovj deleted the add-notes-to-history 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.
View
16 app/controllers/notes_controller.rb
@@ -0,0 +1,16 @@
+class NotesController < ApplicationController
+ def create
+ note = Note.new(filtered_params)
+ if note.save
+ head :created
+ else
+ error 422, message: :invalid_attributes, errors: note.errors.full_messages
+ end
+ end
+
+ private
+
+ def filtered_params
+ params.except(:action, :controller)
+ end
+end
View
3  app/decorators/need_with_changesets.rb
@@ -6,7 +6,8 @@ class NeedWithChangesets < SimpleDelegator
def changesets
(need.revisions + [nil]).each_cons(2).map {|revision, previous|
- Changeset.new(revision, previous)
+ notes = Note.where(revision: revision.id)
+ Changeset.new(revision, previous, notes)
}
end
View
4 app/models/changeset.rb
@@ -2,10 +2,12 @@
class Changeset
attr_reader :current, :previous
+ attr_reader :notes
- def initialize(current, previous)
+ def initialize(current, previous, notes=[])
@current = current
@previous = previous
+ @notes = notes
end
# This method returns the changes between the current revision and its
View
29 app/models/note.rb
@@ -0,0 +1,29 @@
+class Note
@benilovj Collaborator

I think that this class should have some validations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ include Mongoid::Document
+ include Mongoid::Timestamps
+
+ field :text, type: String
+ field :need_id, type: Integer
+ field :author, type: Hash
+ field :revision, type: String
+
+ default_scope order_by([:created_at, :desc])
+
+ validates_presence_of :text, :need_id, :author
+ validate :validate_need_id
+
+ def save
+ need = Need.where(need_id: need_id).first
+ self.revision = need.revisions.first.id if need
+ super
+ end
+
+ private
+
+ def validate_need_id
+ need = Need.where(need_id: need_id).first
+ if need.nil?
+ errors.add(:need_id, "A note must have a valid need_id")
+ end
+ end
+end
View
9 app/presenters/changeset_presenter.rb
@@ -8,7 +8,14 @@ def present
action_type: @changeset.current.action_type,
author: @changeset.current.author,
changes: @changeset.changes,
- created_at: @changeset.current.created_at
+ created_at: @changeset.current.created_at,
+ notes: @changeset.notes.map { |note|
+ {
+ text: note.text,
+ author: note.author,
+ created_at: note.created_at
+ }
+ }
}
end
end
View
2  config/routes.rb
@@ -1,6 +1,8 @@
GovukNeedApi::Application.routes.draw do
resources :organisations, :only => :index
resources :needs, :except => [:new, :edit]
+ resources :notes, :only => [:create]
+
put '/needs/:id/closed', to: 'needs#closed'
delete '/needs/:id/closed', to: 'needs#reopen'
View
39 test/functional/notes_controller_test.rb
@@ -0,0 +1,39 @@
+require_relative '../test_helper'
+
+class NotesControllerTest < ActionController::TestCase
+
+ setup do
+ login_as_stub_user
+ @need = FactoryGirl.create(:need)
+ @note = {
+ text: "test",
+ need_id: @need.need_id,
+ author: {
+ name: stub_user.name,
+ email: stub_user.email
+ }
+ }
+ end
+
+ context "POST create" do
+ should "save the note" do
+ post :create, @note
+
+ note = Note.first
+
+ assert_equal 201, response.status
+ assert_equal "test", note.text
+ assert_equal @need.need_id, note.need_id
+ assert_equal stub_user.name, note.author["name"]
+ assert_equal stub_user.email, note.author["email"]
+ end
+
+ should "show errors if a note is not valid" do
+ post :create, @note.except(:text)
+
+ body = JSON.parse(response.body)
+ assert_equal 422, response.status
+ assert_equal "invalid_attributes", body["_response_info"]["status"]
+ end
+ end
+end
View
16 test/unit/decorators/changeset_test.rb
@@ -22,6 +22,22 @@ class ChangesetTest < ActiveSupport::TestCase
assert_equal Time.parse("2013-10-01 10:00:00"), change.current.created_at
end
+ should "attach notes to a revision" do
+ notes = [
+ {
+ text: "test",
+ author: {
+ name: "Winston Smith-Churchill"
+ },
+ created_at: Time.parse("2013-10-02 10:00:00")
+ }
+ ]
+ change = Changeset.new(@revision, nil, notes)
+ assert_equal "test", change.notes[0][:text]
+ assert_equal Time.parse("2013-10-02 10:00:00"), change.notes[0][:created_at]
+ assert_equal "Winston Smith-Churchill", change.notes[0][:author][:name]
+ end
+
context "calculating the changes with a previous revision" do
should "include changes in values for existing keys" do
previous_revision = OpenStruct.new(
View
9 test/unit/decorators/need_with_changesets_test.rb
@@ -38,4 +38,13 @@ class NeedWithChangesetsTest < ActiveSupport::TestCase
]
}
end
+
+ should "fetch notes" do
+ @revisions.map { |revision|
+ { revision: revision.id }
+ }.each do |search_term|
+ Note.expects(:where).with(search_term)
+ end
+ @decorator.changesets
+ end
end
View
56 test/unit/note_test.rb
@@ -0,0 +1,56 @@
+require_relative '../test_helper'
+
+class NoteTest < ActiveSupport::TestCase
+ setup do
+ @need = FactoryGirl.create(:need)
+ @atts = {
+ text: "test",
+ need_id: @need.need_id,
+ author: {
+ name: "Winston Smith-Churchill",
+ email: "winston@alphagov.co.uk",
+ uid: "win5t0n"
+ }
+ }
+ end
+
+ should "be able to create a note" do
+ Need.expects(:where).twice.returns([@need])
+
+ note = Note.new(@atts)
+ note.save
+ note.reload
+
+ assert_equal "test", note.text
+ assert_equal "Winston Smith-Churchill", note.author["name"]
+ assert_equal @need.revisions.first.id.to_s, note.revision
+ end
+
+ should "not save a note without a need_id" do
+ note = Note.new(@atts.except(:need_id))
+
+ refute note.save
+ assert note.errors.has_key?(:need_id)
+ end
+
+ should "not save a note without a valid need_id" do
+ note = Note.new(@atts.merge(need_id: :foo))
+
+ refute note.save
+ assert note.errors.has_key?(:need_id)
+ end
+
+ should "not save a note without text" do
+ note = Note.new(@atts.except(:text))
+
+ refute note.save
+ assert note.errors.has_key?(:text)
+ end
+
+ should "not save a note without an author" do
+ note = Note.new(@atts.except(:author))
+
+ refute note.save
+ assert note.errors.has_key?(:author)
+ end
+end
View
24 test/unit/presenters/changeset_presenter_test.rb
@@ -16,9 +16,20 @@ class ChangesetPresenterTest < ActiveSupport::TestCase
created_at: Time.parse("2013-01-01")
)
+ note = OpenStruct.new(
+ text: "test",
+ author: {
+ name: "Sir John Anderson",
+ email: "jock@alphagov.co.uk",
+ uid: "j0hn"
+ },
+ created_at: Time.parse("2013-01-02")
+ )
+
@changeset = OpenStruct.new(
current: current,
- changes: { role: [ "user", "home owner" ] }
+ changes: { role: [ "user", "home owner" ] },
+ notes: [note]
)
end
@@ -35,4 +46,15 @@ class ChangesetPresenterTest < ActiveSupport::TestCase
assert_equal ["user", "home owner"], response[:changes][:role]
end
+
+ should "return notes as part of the changset" do
+ response = ChangesetPresenter.new(@changeset).present
+
+ note = response[:notes][0]
+ assert_equal "test", note[:text]
+ assert_equal "Sir John Anderson", note[:author][:name]
+ assert_equal "jock@alphagov.co.uk", note[:author][:email]
+ assert_equal "j0hn", note[:author][:uid]
+ assert_equal Time.parse("2013-01-02"), note[:created_at]

can you split these note assertions into a separate should block ? i feel notes cannot be considered as basic attributes of a need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
end
Something went wrong with that request. Please try again.