Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

Record work item comments revisions (#914) #930

Merged
merged 3 commits into from Mar 3, 2017

Conversation

xcoulon
Copy link
Contributor

@xcoulon xcoulon commented Mar 3, 2017

Added a comment_revisions table to retain all changes on comments,
along with a timestamp, the Id of the author identity and the type
of operation that was performed.
When the comment is deleted, the 'body' and 'markup' fields are 'Nil'.
The list of revisions is retrieved in its historical order.

Also renamed the workitem.WorkItemRevision to workitem.Revision
and removed the WorkItem part in the revision constants.

Fixes #914

Signed-off-by: Xavier Coulon xcoulon@redhat.com

Added a `comment_revisions` table to retain all changes on comments,
along with a timestamp, the Id of the author identity and the type
of operation that was performed.
When the comment is deleted, the 'body' and 'markup' fields are 'Nil'.
The list of revisions is retrieved in its historical order.

Also renamed the `workitem.WorkItemRevision` to `workitem.Revision`
and removed the `WorkItem` part in the revision constants.

Fixes fabric8-services#914

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
// RevisionTypeDelete a work item deletion
RevisionTypeDelete // 2
_ // ignore 3rd value
// RevisionTypeUpdate a work item update
Copy link
Contributor

Choose a reason for hiding this comment

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

comment is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

damn'it !

@@ -6,22 +6,22 @@ import (
uuid "github.com/satori/go.uuid"
)

// RevisionType defines the type of revision for a work item
// WorkIteRevisionTypemRevisionType defines the type of revision for a work item
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
@codecov-io
Copy link

codecov-io commented Mar 3, 2017

Codecov Report

Merging #930 into master will increase coverage by 0.04%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #930      +/-   ##
==========================================
+ Coverage   68.59%   68.63%   +0.04%     
==========================================
  Files          91       93       +2     
  Lines        6916     6957      +41     
==========================================
+ Hits         4744     4775      +31     
- Misses       1730     1735       +5     
- Partials      442      447       +5
Impacted Files Coverage Δ
migration/migration.go 45.65% <100%> (+0.15%)
workitem/workitem_repository.go 71.86% <100%> (ø)
comment/comment_revision.go 100% <100%> (ø)
controller/work-item-comments.go 71.08% <100%> (ø)
workitem/workitem_revision_repository.go 93.54% <100%> (ø)
controller/comments.go 77.66% <100%> (ø)
workitem/workitem_revision.go 100% <100%> (ø)
comment/comment_repository.go 56.25% <56.25%> (ø)
comment/comment_revision_repository.go 87.09% <87.09%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7fbff65...e6b450f. Read the comment docs.

Copy link
Member

@sbose78 sbose78 left a comment

Choose a reason for hiding this comment

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

LGTM , some minor comments.

assert.Equal(s.T(), rendering.SystemMarkupMarkdown, *revision1.CommentMarkup)
assert.Equal(s.T(), s.testIdentity1.ID, revision1.ModifierIdentity)
// revision 2
revision2 := commentRevisions[1]
Copy link
Member

Choose a reason for hiding this comment

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

I'm torn between

Whether we should move repetitive code inside a different method ,
and
the prospect of losing line numbers in case of error.

:)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me see what I can about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, code would look like this:

expectedBody := "Body"
expectedMarkup := rendering.SystemMarkupMarkdown
expectedRevision := comment.Revision{
	CommentID:        c.ID,
	ModifierIdentity: s.testIdentity1.ID,
	Type:             comment.RevisionTypeCreate,
	CommentBody:      &expectedBody,
	CommentMarkup:    &expectedMarkup,
}
s.assertRevision(revision1, expectedRevision)

along with

func (s *revisionRepositoryBlackBoxTest) assertRevision(expected, actual comment.Revision) {
	assert.Equal(s.T(), expected.ID, actual.CommentID)
	assert.Equal(s.T(), expected.Type, actual.Type)
	assert.Equal(s.T(), expected.ModifierIdentity, actual.ModifierIdentity)
	assert.Equal(s.T(), expected.CommentBody, *actual.CommentBody)
	assert.Equal(s.T(), expected.CommentMarkup, *actual.CommentMarkup)

}

so there's no real benefit in terms of readability and number of lines, and indeed, in case of assertion error, we would not know which revision is wrong..

}

func (s *revisionRepositoryBlackBoxTest) TearDownTest() {
//s.clean()
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ha yes, good point. I removed it during my last tests to verify the state of the data in the DB. Let me remove the comment.

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
@xcoulon xcoulon merged commit 645ca0e into fabric8-services:master Mar 3, 2017
AdamJ pushed a commit to AdamJ/almighty-core that referenced this pull request Mar 7, 2017
…ervices#930)

* Record work item comments revisions (fabric8-services#914)

Added a `comment_revisions` table to retain all changes on comments,
along with a timestamp, the Id of the author identity and the type
of operation that was performed.
When the comment is deleted, the 'body' and 'markup' fields are 'Nil'.
The list of revisions is retrieved in its historical order.

Also renamed the `workitem.WorkItemRevision` to `workitem.Revision`
and removed the `WorkItem` part in the revision constants.

Fixes fabric8-services#914

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants