Review status of issues. #768

csordasmarton · 2017-08-03T12:31:20Z

Closes #684 issue.

Xazax-hun · 2017-08-07T08:45:36Z

api/report_server.thrift

+                                           // execution step list.
+  8: shared.Severity      severity         // checker severity
+  9: shared.ReviewStatus  reviewStatus     // bug review status.
+  10: string              reviewComment    // review commment if review status is changed.


Do we need this comment in the Report data? Shouldn't the comment only be the part of the bug history that is queried with a separate API?

This is not the same as the comments table, after the latest discussion review comments will be handled separately.
But I agree a separate api might be better to get these values. Will we show these comments on the main report lists?

We show these values at the bug overview:

If we remove these values from here, we need to query these values for every report id which isn't very good for performance.

Xazax-hun · 2017-08-07T08:46:15Z

api/report_server.thrift

@@ -61,8 +61,7 @@ struct ReportFilter{
  3: optional shared.Severity severity,
  4: optional string          checkerId,          // should filter in the fully qualified checker id name such as alpha.core.
                                                  // the analyzed system. Projects can optionally use this concept.
-  5: optional bool            suppressed = false, // if the bug state is suppressed
-  6: optional string          bugHash
+  5: optional string          bugHash


Shouldn't we be able to filter on review status?

Filtering is an upcoming feature in a different patch, #687.

Xazax-hun · 2017-08-07T08:47:25Z

api/report_server.thrift

-  shared.SuppressBugList getSuppressedBugs(1: i64 run_id)
-                                          throws(1: shared.RequestFailed requestError),
+  // change review status of a bug.
+  bool changeReviewStatusByHash(1: string bugHash,


Do we want to expose bug hash in an API call like this?

Agreed, same as with "hiding" the bugHash behind the comment ID for the commenting feature, let's explain this better.

And the document string is not a full sentence either. 😉

reportId should be used instead of the hash.

I removed this function from the API. I use the changeReviewStatus function instead of this.

Xazax-hun · 2017-08-07T08:47:47Z

api/report_server.thrift

-                     throws (1: shared.RequestFailed requestError),
+  // change review status of a bug.
+  bool changeReviewStatus(1: i64 reportId,
+                          2: i64 status,


Why not shared.ReviewStatus?

There will be no other report server now to share the types with. Everything should be here.

Thanks, I changed the type to shared.ReviewStatus.

If I put the ReviewStatus type to this file, there will be a conflict in the client_db_access_handler.py because it imports the orm_model module which indicates a conflict, because the table name is the same as the type.

Xazax-hun · 2017-08-07T08:49:45Z

api/shared.thrift

@@ -68,3 +68,11 @@ exception RequestFailed {
  2: string    message
 }

+//-----------------------------------------------------------------------------


I am not fond of these comments, I'd rather remove them all than adding new ones.

Xazax-hun · 2017-08-07T09:09:57Z

docs/user_guide.md

@@ -701,18 +672,7 @@ void test() {
  --export-source-suppress


Do we want to keep this? Shouldn't we have something like export user data, or export issue data instead?

export-source-suppress works at the analysis phase (collects suppress comments) and does not get the values from the database. I think we should not remove this yet. At least not as part of this pull request.

Xazax-hun · 2017-08-07T09:12:41Z

libcodechecker/cmd/cmd_line_client.py

        report_filter = [
-            codeCheckerDBAccess.ttypes.ReportFilter(suppressed=suppr)]
+            codeCheckerDBAccess.ttypes.ReportFilter(r)]


What is r here?

Xazax-hun · 2017-08-07T09:17:22Z

libcodechecker/orm_model.py

@@ -287,6 +266,16 @@ def __init__(self, bug_hash, author, message, created_at):
        self.created_at = created_at


+class ReviewStatus(Base):


How do we display these? Shouldn't status changes be shown like regular comments?

If we want to support displaying the whole history of bug status changes, can the bug_hash be a primary key? Multiple status changes can happen to the same bug.

bug_hash perhaps won't be sufficient later on, when PathHash is introduced, if it is.,

Only one review status will be available for a bug_hash. Later, we will insert system events in the Comment table, see #746 issue.

whisperity

👍 ❤️

I can't seem to change the review status with an empty comment. Is this intended?

whisperity · 2017-08-07T12:18:05Z

docs/user_guide.md

                                [-p PORT]
                                [--verbose {info,debug,debug_analyzer}]

-Exports or imports suppressions from a CodeChecker server from/to a suppress
-file, and is also used (if '--bugid' is specified) to (un)suppress reports.
+Imports suppressions from a CodeChecker server from a suppress file.


Import [...] from [...] from [...] ? Also the text here does not match the description of the subcommand --help.

whisperity · 2017-08-07T12:20:11Z

libcodechecker/libhandlers/cmd.py

@@ -300,14 +300,6 @@ def __register_suppress(parser):

    group = parser.add_mutually_exclusive_group(required=True)

-    group.add_argument('-e', '--export',


If we are changing it and updating the usage of cmd suppress, let's update the oneline and the multiline description later on where the subcommand is defined, and keep the user_guide.md in line with these changes.

whisperity · 2017-08-07T12:22:14Z

libcodechecker/libhandlers/store.py

 from libcodechecker import generic_package_context
 from libcodechecker import host_check
-from libcodechecker import suppress_file_handler


We removed the import of this lib. I take it it's still used elsewhere in the cmd so we can't fully get rid of it? (Yet?)

We use it in two places:

https://github.com/csordasmarton/codechecker/blob/review_status/libcodechecker/cmd/cmd_line_client.py#L476 (Use it to import data from suppression file).

https://github.com/csordasmarton/codechecker/blob/review_status/libcodechecker/generic_package_suppress_handler.py#L13 (Use it to process suppression in codes).

whisperity · 2017-08-07T12:25:18Z

www/scripts/codecheckerviewer/ListOfBugs.js

@@ -195,7 +204,8 @@ function (declare, Deferred, ObjectStore, Store, QueryResults, topic,
        { name : 'Message', field : 'checkerMsg', width : '100%' },
        { name : 'Checker name', field : 'checkerId', cellClasses : 'link', width : '50%' },
        { name : 'Severity', field : 'severity' },
-        { name : 'Suppress', field : 'suppressComment' }
+        { name : 'Review status', field : 'reviewStatusHtml', cellClasses : 'review-status', width : '25%' },
+        { name : 'Review comment', field : 'reviewComment' }


Do we really wish to show the full comment on the bug list? What if the comment gets too long?

Perhaps we should instead only show a "message" icon or something clickable that presents the comment as a pop-up?
Or implement a way to "crop" the too long text at least, because this happens:

I increased the width of this column. If the content is larger than the width of this column, the content is cropped. I also show the full comment message in a tooltip on hover of this column.

whisperity · 2017-08-07T12:28:15Z

www/scripts/codecheckerviewer/BugViewer.js

+
+      //--- Review status comment message. ---//
+
+      var reviewComment = dom.create('span', {


I like this idea very much! 💛

However, I don't really like the fact that this icon is there even if no custom comment is given -- and by default it shows the same text as the review status drop-down next to it. Perhaps we could hide it, if the isn't a comment.

Great idea, thanks 👍

whisperity · 2017-08-07T12:30:27Z

www/style/codecheckerviewer.css

+}
+
+.review-comment::before {


If there is a chance, perhaps this icon should have a cursor: help; style added to it. I think it requires a new style element, not in the ::before.

whisperity

As discussed:

Let's allow changing the review status with an empty comment.
Also add an appropriate placeholder text for the comment window: (Optionally) explain the status change..., or something akin to this.

whisperity

I forgot another thing: suppressions in the source code should set the bug's state to... either false positive or won't fix (@dkrupp please clarify.)
Please import the suppression comment as the review status' comment from the source code into the database.

whisperity

The browser floods the console with TypeError: item.reviewComment is null in ListOfBugs.js:264:13 if the mouse is moved on an empty review comment cell.

The tooltip widths doesn't seem to be enough:

Sorry, it looks like the screenshot taker is slower than the onMouseOver.

csordasmarton · 2017-08-08T14:29:47Z

Thanks, I added a CSS word-wrap property for the tooltip. Now it breaks the long words. and I fixed the empty comment problem.

whisperity

Okay. Truly thelast issue.

When I change a bug using the BugPath tree view on the left, it is still the review status of the bug that I opened the current tab with.

(The title of the tab does not change either, when I navigate bugs in the same file.)

whisperity

Feature-wise we are good. Let's make the tests a bit more resillient and we'll be good to go.

whisperity · 2017-08-09T11:10:35Z

tests/functional/review_status/test_review_status.py

+            shared.ttypes.ReviewStatus.CONFIRMED,
+            review_comment)
+
+        self.assertTrue(success)


Shouldn't we also check that the state actually did change on the server? This only checks that the API returns True, it does not check the proper database write.

whisperity · 2017-08-09T11:10:56Z

tests/functional/review_status/test_review_status.py

+        bug = run_results[0]
+
+        # Change review status to confirmed bug.
+        review_comment = r'This is really a bug'


I would simply use "This is really a bug" here.

whisperity · 2017-08-09T11:11:27Z

tests/functional/review_status/test_review_status.py

+        logging.debug("Bug review status didn't changed")
+
+        # Change review status to false positive.
+        review_comment = r'This is not a bug'


No need to introduce raw strings, if not neccessary. A simple "string" should work.

csordasmarton added API change 📄 Content of patch changes API! WARN ⚠️: Backward compatibility breaker! MIND THE GAP! Merging this patch will mess up compatibility with the previous releases! documentation 📖 Changes to documentation. GUI 🎨 labels Aug 3, 2017

csordasmarton added this to the 6.0 pre2 milestone Aug 3, 2017

csordasmarton force-pushed the review_status branch from 381fdf3 to 5472af1 Compare August 3, 2017 12:50

whisperity self-requested a review August 3, 2017 16:26

csordasmarton mentioned this pull request Aug 4, 2017

Complex issue filtering by run names, checkername, severity, checker message, file filter, review status [128h] #687

Closed

6 tasks

Xazax-hun suggested changes Aug 7, 2017

View reviewed changes

whisperity requested changes Aug 7, 2017

View reviewed changes

csordasmarton force-pushed the review_status branch 6 times, most recently from f4f60cb to 2f271f1 Compare August 8, 2017 13:59

whisperity requested changes Aug 8, 2017

View reviewed changes

csordasmarton force-pushed the review_status branch from 2f271f1 to 8b5821a Compare August 8, 2017 14:26

whisperity requested changes Aug 8, 2017

View reviewed changes

csordasmarton force-pushed the review_status branch 5 times, most recently from dd114b8 to 09014f3 Compare August 9, 2017 10:10

whisperity requested changes Aug 9, 2017

View reviewed changes

Review status of issues.

08f9716

csordasmarton force-pushed the review_status branch from 09014f3 to 08f9716 Compare August 9, 2017 11:35

whisperity approved these changes Aug 9, 2017

View reviewed changes

whisperity merged commit 7c1503e into Ericsson:version6 Aug 9, 2017

csordasmarton deleted the review_status branch August 9, 2017 14:53

This was referenced Aug 9, 2017

show filter in the url #775

Merged

Fix bug tree showing all bugs in the project #806

Merged

whisperity mentioned this pull request Aug 17, 2017

Detection status #724

Merged

		@@ -287,6 +266,16 @@ def __init__(self, bug_hash, author, message, created_at):
		self.created_at = created_at


		class ReviewStatus(Base):

		@@ -300,14 +300,6 @@ def __register_suppress(parser):

		group = parser.add_mutually_exclusive_group(required=True)

		group.add_argument('-e', '--export',


		//--- Review status comment message. ---//

		var reviewComment = dom.create('span', {

		}

		.review-comment::before {

Review status of issues. #768

Review status of issues. #768

Conversation

csordasmarton commented Aug 3, 2017

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

whisperity left a comment

Choose a reason for hiding this comment

whisperity Aug 7, 2017 • edited Loading

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

whisperity left a comment

Choose a reason for hiding this comment

whisperity left a comment

Choose a reason for hiding this comment

whisperity left a comment • edited Loading

Choose a reason for hiding this comment

csordasmarton commented Aug 8, 2017

whisperity left a comment

Choose a reason for hiding this comment

whisperity 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

whisperity Aug 7, 2017 •

edited

Loading

whisperity left a comment •

edited

Loading