Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add notes to history #54

Merged
merged 10 commits into from Feb 28, 2014

Conversation

Projects
None yet
4 participants
Contributor

bishboria commented Feb 21, 2014

No description provided.

@bishboria bishboria referenced this pull request in alphagov/gds-api-adapters Feb 21, 2014

Merged

Add create_note method for need api #125

Contributor

fatbusinessman commented Feb 21, 2014

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.

Contributor

bishboria commented Feb 24, 2014

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.

@vinayvinay vinayvinay commented on an outdated diff Feb 24, 2014

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
@vinayvinay

vinayvinay Feb 24, 2014

Contributor

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

@vinayvinay vinayvinay commented on an outdated diff Feb 24, 2014

app/models/changeset.rb
- def initialize(current, previous)
+ def initialize(current, previous, notes)
@vinayvinay

vinayvinay Feb 24, 2014

Contributor

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.

@vinayvinay vinayvinay commented on the diff Feb 24, 2014

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]
@vinayvinay

vinayvinay Feb 24, 2014

Contributor

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

Contributor

vinayvinay commented Feb 24, 2014

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

@fatbusinessman fatbusinessman and 2 others commented on an outdated diff Feb 25, 2014

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])
@fatbusinessman

fatbusinessman Feb 25, 2014

Contributor

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.

@bishboria

bishboria Feb 25, 2014

Contributor

_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.

@vinayvinay

vinayvinay Feb 25, 2014

Contributor

@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.

@fatbusinessman

fatbusinessman Feb 25, 2014

Contributor

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.

Contributor

fatbusinessman commented Feb 25, 2014

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.

Contributor

bishboria commented Feb 25, 2014

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.

@benilovj benilovj commented on an outdated diff Feb 27, 2014

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

benilovj Feb 27, 2014

Contributor

it'd be good to return the validation errors here

@benilovj benilovj commented on the diff Feb 27, 2014

app/models/note.rb
@@ -0,0 +1,17 @@
+class Note
@benilovj

benilovj Feb 27, 2014

Contributor

I think that this class should have some validations

Contributor

bishboria commented Feb 28, 2014

@benilovj I've pushed those changes

benilovj added a commit that referenced this pull request Feb 28, 2014

@benilovj benilovj merged commit 5deb520 into master Feb 28, 2014

1 check passed

default "Build #14 succeeded on Jenkins"
Details

@benilovj benilovj deleted the add-notes-to-history branch Feb 28, 2014

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