Fix `rbt status` returning blank summary for draft review requests

Review Request #11174 — Created Sept. 18, 2020 and submitted

ceciliawei
RBTools
release-1.0.x
4855
8f635ea...
rbtools, students

When displaying the summary for the review request, the code attempts to get the
summary from request.summary, which is null for draft reviews. In this
code change, we added a condition on where to get the summary - if the
draft exists, we get the draft summary; otherwise we get it from the
published review request.

The issue was fixed by the code change.

  1. Reproduced issue in 4855, confirmed the summary for a draft review shows as expected after the code change
+--------+-----------------------------------------------------+---------------+
| Status |                   Review Request                    |    Branch     |
+========+=====================================================+===============+
| Draft  | r/11174 - fix 4855:rbtools 1.0.2 returns blank      | release-1.0.x |
|        | summary for draft review requests                   |               |
+--------+-----------------------------------------------------+---------------+
  1. Ran the unit tests using ./tests/runtests.py, all passed.
Description From Last Updated

The summary and description can be improved here. We generally don't need to list bug numbers or versions there, since ...

daviddavid

Getting pretty close. Can we make a couple little tweaks in the description? Please wrap it to 70 columns. The ...

daviddavid

Looks like the description is wrapping a bit too early. Can you target ~70 (upwards of 75) characters for the ...

chipx86chipx86

In your description, you use "CR", which isn't an abbreviation that we use in the codebase. Let's say "summary for ...

daviddavid

In testing done, it's better to use ./tests/runtests.py rather than nosetests. They're mostly the same thing, but just to be ...

daviddavid

E501 line too long (87 > 79 characters)

reviewbotreviewbot

It's just a tiny style nitpick, but can we add a blank line between these two please?

daviddavid

Let's add a trailing comma at the end of this line.

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

ceciliawei
david
  1. 
      
  2. The summary and description can be improved here. We generally don't need to list bug numbers or versions there, since that data is elsewhere in the review request fields (branch / bugs fields).

    See https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea for our guide on what we like to see for the summary and description.

    1. Thanks! I will keep that in mind.

  3. rbtools/commands/status.py (Diff revision 2)
     
     
     

    It's just a tiny style nitpick, but can we add a blank line between these two please?

    1. Yes I will address this in the revision.

  4. 
      
ceciliawei
ceciliawei
  1. 
      
  2. 
      
david
  1. 
      
  2. Getting pretty close. Can we make a couple little tweaks in the description?

    Please wrap it to 70 columns.

    The first line there duplicates what you have in the summary, and can be deleted.

    We don't need to include quite so much actual code snippet in the description--if I want to know exactly what the code does, I'll look at the diff. Let's just say that if the draft exists, we get the draft summary, otherwise we get it from the published review request.

    1. Addressed in revision.

  3. rbtools/commands/status.py (Diff revision 3)
     
     

    Let's add a trailing comma at the end of this line.

    1. Addressed in revision.

  4. 
      
ceciliawei
ceciliawei
  1. 
      
  2. 
      
chipx86
  1. 
      
  2. Looks like the description is wrapping a bit too early. Can you target ~70 (upwards of 75) characters for the wrap point? Helps keep it from appearing really short. A good trick is to compose it in an editor with a wrap point (Review Board doesn't do this built-in because different companies/teams have different requirements).

    1. Just updated the description.

    2. Looks great!

      (After working with review requests for so long, we've developed this amazing, almost-useless sixth sense to determine the rough line length of a review request description purely by sight.)

  3. 
      
ceciliawei
david
  1. Just a couple very minor comments about your review request metadata:

    1. Addressed in revision. Thanks for catching those :)

  2. In your description, you use "CR", which isn't an abbreviation that we use in the codebase. Let's say "summary for the review request".

  3. In testing done, it's better to use ./tests/runtests.py rather than nosetests. They're mostly the same thing, but just to be consistent let's prefer the former. You also don't necessarily need to include the output from the suite, you can just say you ran the unit tests.

  4. 
      
ceciliawei
david
  1. Ship It!
  2. 
      
ceciliawei
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-1.0.x (9213a78)
Loading...