System comments #2032

csordasmarton · 2019-04-03T13:07:10Z

Closes #746

Show system comments at bugs.

Screenshot:

whisperity · 2019-04-04T08:32:01Z

web/api/v6/report_server.thrift

@@ -85,6 +85,11 @@ enum ExtendedReportDataType {
  FIXIT = 20,
 }

+enum CommentKind {
+  GENERAL, // User based comments.


whisperity · 2019-04-04T08:32:52Z

web/config/system_comment_map.json

@@ -0,0 +1,6 @@
+{


Can we call this file differently? system_comment_kinds? system_comment_expansion? "Map" is too generic for a config file name.

whisperity · 2019-04-04T09:48:22Z

web/server/codechecker_server/api/store_handler.py

@@ -564,4 +564,4 @@ def addFileRecord(session, filepath, content_hash):
            .filter(File.content_hash == content_hash,
                    File.filepath == filepath).one_or_none()

-    return file_record.id if file_record else None
+    return file_record.id if file_record else None


unrelated diff in patch, also doesn't not having a line break at the end violate pycodestyle?

whisperity · 2019-04-04T10:30:44Z

It'd be even fancier if the "blips" could also be turned into icons (like how bug path elements have an icon, or how the blips of moments on GitHub on this patch also have icons), but that's just continuing to polish it fancy...

I really like the whole "bus schedule" looks to it. 😍

gyorb

Will this blow up our database size? How many entries are generated for a typical store?

Turning some checker profiles on and off could result in a lot of detection status changes.

gyorb · 2019-04-17T13:04:07Z

web/api/v6/report_server.thrift

@@ -85,6 +85,11 @@ enum ExtendedReportDataType {
  FIXIT = 20,
 }

+enum CommentKind {
+  GENERAL, // User-given comments.


I would rename this to USER, GENERAL is a bit too general ;)

gyorb · 2019-04-17T13:05:04Z

web/server/codechecker_server/api/report_server.py

@@ -313,6 +314,20 @@ def extended_data_db_to_api(erd):
        fileId=erd.file_id)


+def comment_kind_str(status):
+    if status == ttypes.CommentKind.GENERAL:
+        return 'general'


The same here user

gyorb · 2019-04-17T13:25:31Z

web/config/system_comment_kinds.json

@@ -0,0 +1,6 @@
+{
+  "rev_st_changed": "%author% changed review status from <b class=\"old-review-status\">{0}</b> to <b class=\"new-review-status\">{1}</b> %date%",


Do we need to store the whole text, couldn't we create some more dense representation?

We are not store the whole text in the database we just store the type of the system comment and the arguments: rev_st_changed Unreviewed Confirmed. We are using this file only to display the comment in the gui.

csordasmarton · 2019-12-09T09:50:25Z

web/server/codechecker_server/migrations/report/versions/6cb6a3a41967_system_comments.py

+
+
+def upgrade():
+    op.add_column('comments', sa.Column('kind', sa.Enum(u'user',


It will fail on postgresql. We can fix this by using the following instructions: https://stackoverflow.com/questions/37848815/sqlalchemy-postgresql-enum-does-not-create-type-on-db-migrate.

comment_kind_name = 'comment_kind' enum_values = (u'user', u'system') commnet_kind_column = sa.Column('kind', sa.Enum(*enum_values, name=comment_kind_name), server_default=u'user', nullable=False) def upgrade(): ctx = op.get_context() dialect = ctx.dialect.name if dialect == 'postgresql': from sqlalchemy.dialects import postgresql comment_kind = postgresql.ENUM(*enum_values, name=comment_kind_name) comment_kind.create(op.get_bind()) op.add_column('comments', commnet_kind_column) elif dialect == 'sqlite': op.add_column('comments', commnet_kind_column) def downgrade(): op.drop_column('comments', 'kind') ctx = op.get_context() dialect = ctx.dialect.name if dialect == 'postgresql': from sqlalchemy.dialects import postgresql comment_kind = postgresql.ENUM(*enum_values, name=comment_kind_name) comment_kind.drop(op.get_bind())

But as we discussed it personally we will use integers instead of enums to avoid this kind of hacks.

csordasmarton · 2019-12-09T10:28:20Z

@gyorb I have refactored the migration script to use Integer instead of Enum.

gyorb · 2019-12-09T10:32:51Z

web/server/www/scripts/codecheckerviewer/CommentView.js

          } catch (ex) { util.handleThriftException(ex); }

-          that._message.set('content',
-            newContent.replace(/(?:\r\n|\r|\n)/g, '<br>'));
+          that._message.innerHTML =


Issue found: Unsafe assignment to innerHTML (no-unsafe-innerhtml/no-unsafe-innerhtml)

gyorb · 2019-12-09T10:32:52Z

web/server/www/scripts/codecheckerviewer/CommentView.js

+            domElement.getElementsByClassName(className);
+
+          for (var i = 0; i < domStatuses.length; i++) {
+            var domStatus = domStatuses[i];


Issue found: Variable Assigned to Object Injection Sink (security/detect-object-injection)

gyorb · 2019-12-09T10:32:54Z

web/server/www/scripts/codecheckerviewer/CommentView.js

@@ -108,7 +107,7 @@ function (declare, dom, style, topic, Memory, Observable, ConfirmDialog,
        content   : 'Are you sure you want to delete this?',
        onExecute : function () {
          try {
-            CC_SERVICE.removeComment(that.cId);
+            CC_SERVICE.removeComment(that.commentData.id);


Issue found: 'CC_SERVICE' is not defined. (no-undef)

gyorb · 2019-12-09T10:32:55Z

web/server/www/scripts/codecheckerviewer/CommentView.js

-            message  : comment.message,
-            sender   : sender
-          }));
+          if (comment.kind === CC_OBJECTS.CommentKind.SYSTEM) {


Issue found: 'CC_OBJECTS' is not defined. (no-undef)

gyorb · 2019-12-09T14:28:42Z

web/api/v6/report_server.thrift

@@ -93,6 +93,11 @@ enum ExtendedReportDataType {
  FIXIT = 20,
 }

+enum CommentKind {


I'm thinking maybe it would be better if we introduce some prefix for the thrift types.
There are conversion functions which convert from CommentKind to CommentKind one of them is a thrift enum and the other is a local enum it is hard to follow which type is used where and what is converted to what.

gyorb · 2019-12-09T14:31:43Z

web/server/codechecker_server/api/report_server.py

@@ -469,6 +477,20 @@ def extended_data_db_to_api(erd):
        fileId=erd.file_id)


+def comment_kind_str(status):


Is there any string involved in this conversion? Just another enum value is returned.
I'm thinking would it be better to tie this conversion functions closer to the CommentKind local enum type?
With some other name which makes the conversion more explicit to_thrift_type, from_thrift_type.

A function docstring would be nice for the conversion functions.

gyorb · 2019-12-09T14:34:07Z

web/server/codechecker_server/api/report_server.py

@@ -653,6 +675,17 @@ def sort_run_data_query(query, sort_mode):
    return query


+def escape_whitespaces(s, whitespaces=None):


Please add function docstring.

gyorb · 2019-12-09T14:38:30Z

web/server/codechecker_server/api/report_server.py

+
+                    sys_comment = comment_kind_str(ttypes.CommentKind.SYSTEM)
+                    if comment.kind == sys_comment:
+                        elements = shlex.split(message)


why do we need the shlex.split here?

Before we store a system comment in the database, we will escape the white spaces (escape_whitespaces). We are using shlex here to split this escaped string properly.

gyorb · 2019-12-09T14:41:00Z

web/server/codechecker_server/api/report_server.py

@@ -2441,6 +2520,7 @@ def checker_is_unavailable(checker_name):
                detection_status = 'new'
                detected_at = run_history_time

+                old_status = None


Do we need to set the default value here? The variable is used only inside the if block below I did not found any other references.

gyorb · 2019-12-09T14:43:00Z

web/server/codechecker_server/database/run_db_model.py

@@ -360,12 +360,16 @@ class Comment(Base):
    bug_hash = Column(String, nullable=False, index=True)
    author = Column(String, nullable=False)
    message = Column(Binary, nullable=False)
+    kind = Column(Integer,
+                  nullable=False,
+                  server_default="0")


Maybe a comment about the default value here would be nice. '0' means a comment type by the user tight?

gyorb

Please increase thrift api version

Show system comments at bugs.

gyorb · 2019-12-10T16:26:32Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 6
- Added 6
           

Complexity increasing per file
==============================
- web/server/codechecker_server/migrations/report/versions/6cb6a3a41967_system_comments.py  1
- web/tests/functional/comment/test_comment.py  2
         

Clones added
============
- web/tests/functional/comment/test_comment.py  2

See the complete overview on Codacy

whisperity · 2019-12-17T12:44:10Z

Oho, this is amazing, thank you!

csordasmarton added enhancement 🌟 database 🗄️ Issues related to the database schema. GUI 🎨 labels Apr 3, 2019

csordasmarton added this to the release 6.10.0 milestone Apr 3, 2019

csordasmarton requested review from whisperity and gyorb April 3, 2019 13:07

whisperity reviewed Apr 4, 2019

View reviewed changes

csordasmarton force-pushed the system_comments branch from bc16dd6 to 57f5c80 Compare April 4, 2019 13:52

Ericsson deleted a comment Apr 4, 2019

gyorb removed this from the release 6.10.0 milestone Apr 13, 2019

gyorb suggested changes Apr 17, 2019

View reviewed changes

csordasmarton force-pushed the system_comments branch from 57f5c80 to e604684 Compare April 18, 2019 13:04

Ericsson deleted a comment Apr 18, 2019

csordasmarton requested a review from gyorb April 18, 2019 13:10

csordasmarton force-pushed the system_comments branch from e604684 to 07f69c0 Compare April 18, 2019 13:23

Ericsson deleted a comment Apr 18, 2019

Ericsson deleted a comment Nov 28, 2019

gyorb requested a review from dkrupp December 3, 2019 10:16

csordasmarton force-pushed the system_comments branch 2 times, most recently from fdbc3fa to d172bbe Compare December 7, 2019 10:38

csordasmarton requested a review from bruntib as a code owner December 7, 2019 10:38

csordasmarton requested review from gyorb December 7, 2019 10:38

csordasmarton commented Dec 9, 2019

View reviewed changes

csordasmarton force-pushed the system_comments branch from d172bbe to 91be1a5 Compare December 9, 2019 10:27

gyorb reviewed Dec 9, 2019

View reviewed changes

gyorb suggested changes Dec 9, 2019

View reviewed changes

csordasmarton force-pushed the system_comments branch from 91be1a5 to 7bf2713 Compare December 10, 2019 10:16

csordasmarton requested a review from gyorb December 10, 2019 10:18

csordasmarton force-pushed the system_comments branch from 7bf2713 to 67dcdd0 Compare December 10, 2019 13:44

gyorb approved these changes Dec 10, 2019

View reviewed changes

gyorb suggested changes Dec 10, 2019

View reviewed changes

System comments

d7b2a56

Show system comments at bugs.

csordasmarton force-pushed the system_comments branch from 67dcdd0 to d7b2a56 Compare December 10, 2019 16:24

csordasmarton requested a review from gyorb December 11, 2019 08:54

gyorb approved these changes Dec 11, 2019

View reviewed changes

gyorb merged commit 06632d3 into Ericsson:master Dec 11, 2019

whisperity mentioned this pull request Dec 16, 2019

Offer a product (or run) wide "update feed" of the hottest reports #2496

Open

whisperity mentioned this pull request Dec 17, 2019

Comments not updated with system comment about review status change when review status is changed #2497

Closed

csordasmarton deleted the system_comments branch December 19, 2019 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

System comments #2032

System comments #2032

csordasmarton commented Apr 3, 2019

whisperity Apr 4, 2019

whisperity Apr 4, 2019

whisperity Apr 4, 2019

whisperity commented Apr 4, 2019

gyorb left a comment

gyorb Apr 17, 2019

gyorb Apr 17, 2019

gyorb Apr 17, 2019

csordasmarton Apr 18, 2019

csordasmarton Dec 9, 2019

csordasmarton commented Dec 9, 2019

gyorb Dec 9, 2019

gyorb Dec 9, 2019

gyorb Dec 9, 2019

gyorb Dec 9, 2019

gyorb Dec 9, 2019

gyorb Dec 9, 2019

gyorb Dec 9, 2019

gyorb Dec 9, 2019

csordasmarton Dec 10, 2019

gyorb Dec 9, 2019

gyorb Dec 9, 2019

gyorb left a comment

gyorb commented Dec 10, 2019

whisperity commented Dec 17, 2019

		@@ -0,0 +1,6 @@
		{
		"rev_st_changed": "%author% changed review status from <b class=\"old-review-status\">{0}</b> to <b class=\"new-review-status\">{1}</b> %date%",



		def upgrade():
		op.add_column('comments', sa.Column('kind', sa.Enum(u'user',

		@@ -469,6 +477,20 @@ def extended_data_db_to_api(erd):
		fileId=erd.file_id)


		def comment_kind_str(status):

		@@ -653,6 +675,17 @@ def sort_run_data_query(query, sort_mode):
		return query


		def escape_whitespaces(s, whitespaces=None):

System comments #2032

System comments #2032

Conversation

csordasmarton commented Apr 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity commented Apr 4, 2019

gyorb left a 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

csordasmarton commented Dec 9, 2019

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

gyorb left a comment

Choose a reason for hiding this comment

gyorb commented Dec 10, 2019

whisperity commented Dec 17, 2019