Refactor comments to support markup types (#707) #717

xcoulon · 2017-02-02T15:45:22Z

Added a markup column in the comments table using
a migration script.
When creating or updating a comment, the default markup
is used if none was provided.
When showing comments, the default markup is also returned
if none was found in the database (ie, for older comments).

Fixes #707

xcoulon · 2017-02-02T15:46:30Z

Marked as WIP since this PR is based on a commit for issue #714 whose PR (#716) is still pending

Added a `markup` column in the `comments` table using a migration script. When creating or updating a comment, the default markup is used if none was provided. When showing comments, the default markup is also returned if none was found in the database (ie, for older comments). Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

codecov-io · 2017-02-07T11:53:15Z

Codecov Report

Merging #717 into master will increase coverage by 0.03%.

@@            Coverage Diff             @@
##           master     #717      +/-   ##
==========================================
+ Coverage   71.12%   71.16%   +0.03%     
==========================================
  Files          80       80              
  Lines        4603     4616      +13     
==========================================
+ Hits         3274     3285      +11     
- Misses       1013     1016       +3     
+ Partials      316      315       -1

Impacted Files	Coverage Δ
comment/comment.go	`72.28% <ø> (ø)`	✅
account/user.go	`45.61% <ø> (ø)`	✅
migration/migration.go	`50% <100%> (+0.22%)`	✅
work-item-comments.go	`73.01% <100%> (-2.43%)`	❌
rendering/markup_content.go	`86.11% <100%> (+1.73%)`	✅
comments.go	`77.33% <100%> (+1.99%)`	✅

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 741a7a0...e7b8de4. Read the comment docs.

kwk · 2017-02-07T12:14:33Z

design/comments.go

@@ -44,6 +47,9 @@ var createCommentAttributes = a.Type("CreateCommentAttributes", func() {
 		a.MinLength(1) // Empty comment not allowed
 		a.Example("This is really interesting")
 	})
+	a.Attribute("markup", d.String, "The comment markup associated with the body", func() {


Does it make sense to have this attribute outsourced because you define it tiwce?!

it is outsourced twice just as body is, because it is used in the types associated with the create and update operations

hectorj2f · 2017-02-07T12:17:20Z

comments_blackbox_test.go

@@ -45,6 +46,10 @@ func (s *CommentsSuite) TearDownTest() {
 	s.clean()
 }

+var markdownMarkup = rendering.SystemMarkupMarkdown


I don't know if it is a code convention but you could also define it like this:

var ( markdownMarkup = rendering.SystemMarkupMarkdown plaintextMarkup = rendering.SystemMarkupPlainText defaultMarkup = rendering.SystemMarkupDefault ) You type less characters :)

kwk · 2017-02-07T12:16:00Z

rendering/markup_content_test.go

+	// when
+	result := NilSafeGetMarkup(nil)
+	// then
+	require.NotNil(t, result)


I'm not sure you need this nil check because the assert.Equal below just compares values and it is safe to compare nil values as long as you don't dereference a nil value. Right?

yes, correct, I can probably remove that

kwk · 2017-02-07T12:16:04Z

rendering/markup_content_test.go

+	// when
+	result := NilSafeGetMarkup(&markup)
+	// then
+	require.NotNil(t, result)


I'm not sure you need this nil check because the assert.Equal below just compares values and it is safe to compare nil values as long as you don't dereference a nil value. Right?

kwk · 2017-02-07T12:16:08Z

rendering/markup_content_test.go

+	// when
+	result := NilSafeGetMarkup(&markup)
+	// then
+	require.NotNil(t, result)


I'm not sure you need this nil check because the assert.Equal below just compares values and it is safe to compare nil values as long as you don't dereference a nil value. Right?

kwk · 2017-02-07T12:17:48Z

rendering/markup_content_test.go

+import "github.com/stretchr/testify/assert"
+import "github.com/stretchr/testify/require"
+
+func TestGetDefaultMarkupFromNil(t *testing.T) {


I think the functions TestGetDefaultMarkupFromNil, TestGetMarkupFromValue, and TestGetMarkupFromEmptyValue can be merged into one test function, don't you think?

@kwk why would I do that ? I want short unit tests that follow as much as possible the "given/when/then" pattern, which is why I have 3 tests here.

Okay, agreed. they just seemed so small, tiny, and cozy. Those poor little tests looked like they should be sleeping under a shared blanket. :)

@kwk I've had enough of co-sleeping in the past ;-)

kwk · 2017-02-07T12:18:12Z

work-item-comments.go

 		reqComment := ctx.Payload.Data
-
+		fmt.Println(fmt.Sprintf("Processing markup value: '%v'", reqComment.Attributes.Markup))


Please, remove print.

kwk · 2017-02-07T12:20:55Z

work-item-comments_test.go

@@ -181,6 +193,7 @@ func assertComment(t *testing.T, c *app.Comment) {
 	assert.Equal(t, "comments", c.Type)
 	assert.NotNil(t, c.ID)
 	assert.NotNil(t, c.Attributes.Body)
+	assert.NotNil(t, c.Attributes.Markup)
 	assert.NotNil(t, c.Attributes.CreatedAt)


This should be require.NotNil because you dereference the value one line below.

kwk · 2017-02-07T12:21:14Z

work-item-comments_test.go

@@ -181,6 +193,7 @@ func assertComment(t *testing.T, c *app.Comment) {
 	assert.Equal(t, "comments", c.Type)
 	assert.NotNil(t, c.ID)
 	assert.NotNil(t, c.Attributes.Body)
+	assert.NotNil(t, c.Attributes.Markup)
 	assert.NotNil(t, c.Attributes.CreatedAt)
 	assert.WithinDuration(t, time.Now(), *c.Attributes.CreatedAt, 2*time.Second)
 	assert.NotNil(t, c.Relationships)


This should be require.NotNil because you dereference the value one line below.

Taking review comments into account Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

aslakknutsen · 2017-02-07T14:30:04Z

migration/sql-files/023-comment-markup.sql

@@ -0,0 +1,2 @@
+-- add a 'markup' column in the 'comments' table
+alter table comments add column markup text; 


Add PlainText to the existing ones?

I can provide an SQL script for that, indeed.

Sorry, that I didn't notice that.

aslakknutsen · 2017-02-07T14:30:44Z

work-item-comments.go

 		newComment := comment.Comment{
 			ParentID:  ctx.ID,
 			Body:      reqComment.Attributes.Body,
+			Markup:    markup,


htmlescape?

this will be addressed in #724, actually

…ric8-services#717) * Refactor comments to support markup types (fabric8-services#707) Added a `markup` column in the `comments` table using a migration script. When creating or updating a comment, the default markup is used if none was provided. When showing comments, the default markup is also returned if none was found in the database (ie, for older comments). Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

xcoulon force-pushed the Issue707_Comment_markup branch from 27006f4 to 19d7370 Compare February 2, 2017 17:55

xcoulon self-assigned this Feb 7, 2017

xcoulon force-pushed the Issue707_Comment_markup branch from 19d7370 to 748a186 Compare February 7, 2017 11:53

xcoulon changed the title ~~WIP - Refactor comments to support markup types (#707)~~ Refactor comments to support markup types (#707) Feb 7, 2017

xcoulon requested review from aslakknutsen and kwk February 7, 2017 11:53

kwk reviewed Feb 7, 2017

View reviewed changes

hectorj2f reviewed Feb 7, 2017

View reviewed changes

kwk suggested changes Feb 7, 2017

View reviewed changes

Refactor comments to support markup types (fabric8-services#707)

638b67e

Taking review comments into account Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

xcoulon force-pushed the Issue707_Comment_markup branch from ebcf0e9 to 638b67e Compare February 7, 2017 13:31

kwk approved these changes Feb 7, 2017

View reviewed changes

Merge branch 'master' into Issue707_Comment_markup

e7b8de4

kwk merged commit e28a1b7 into fabric8-services:master Feb 7, 2017

aslakknutsen reviewed Feb 7, 2017

View reviewed changes

xcoulon deleted the Issue707_Comment_markup branch February 9, 2017 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor comments to support markup types (#707) #717

Refactor comments to support markup types (#707) #717

xcoulon commented Feb 2, 2017

xcoulon commented Feb 2, 2017

codecov-io commented Feb 7, 2017 •

edited

kwk Feb 7, 2017

xcoulon Feb 7, 2017

hectorj2f Feb 7, 2017

xcoulon Feb 7, 2017

kwk Feb 7, 2017

xcoulon Feb 7, 2017

kwk Feb 7, 2017

xcoulon Feb 7, 2017

kwk Feb 7, 2017

xcoulon Feb 7, 2017

kwk Feb 7, 2017

xcoulon Feb 7, 2017

kwk Feb 7, 2017

xcoulon Feb 7, 2017

kwk Feb 7, 2017

xcoulon Feb 7, 2017

kwk Feb 7, 2017

xcoulon Feb 7, 2017

kwk Feb 7, 2017

xcoulon Feb 7, 2017

aslakknutsen Feb 7, 2017

xcoulon Feb 7, 2017

kwk Feb 9, 2017

aslakknutsen Feb 7, 2017

xcoulon Feb 7, 2017

		reqComment := ctx.Payload.Data

		fmt.Println(fmt.Sprintf("Processing markup value: '%v'", reqComment.Attributes.Markup))

		@@ -0,0 +1,2 @@
		-- add a 'markup' column in the 'comments' table
		alter table comments add column markup text;

Refactor comments to support markup types (#707) #717

Refactor comments to support markup types (#707) #717

Conversation

xcoulon commented Feb 2, 2017

xcoulon commented Feb 2, 2017

codecov-io commented Feb 7, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 7, 2017 •

edited