Submitter:	Christian Hammond	Reviewers
Branch:	trunk	Groups:	reviewboard
Bugs:	323	People:
Change Number:	None	Repository:	Review Board SVN

Description:

We had several issues with draft diffs.

1) They were automatically placed in the diffset history of a review request, making them publicly accessible. This meant they were not destroyed when destroying a draft. I'm unsure as to how long this has been broken.
2) The queries for grabbing a draft diff didn't take into account the user accessing the review request.
3) The diff viewer didn't really let you know you were looking at a draft diff, and didn't work when #1 was fixed.

We now keep diffs out of a review request's diffset history when creating them, and we assign them an appropriate revision number. We only check for the draft diffs if the requesting user is logged in and owns the review request. We also modify the diff viewer a bit to tell the user when they're looking at a draft diff.

This fixes bug 323, and should also fix bugs regarding revision numbers being skipped and default reviewers not being set (since that check queried if any diffsets existed for the review request, which they did due to #1 above).

Testing Done:

All my manual tests showed that the correct things were being seen for a logged in user accessing a draft diff, and that other users and non-logged in users couldn't see it.

Verified that interdiffs work with the draft diffs and that discarding a draft deletes the diffset.

All unit tests pass (with a tweak to fix broken behavior).

Diff revision 4 (Latest)

Files Changed:

/trunk/reviewboard/reviews/views.py: 15 changes [ 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 ]
/trunk/reviewboard/templates/diffviewer/view_diff.html: 9 changes [ 1 2 3 4 5 6 7 8 9 ]
/trunk/reviewboard/webapi/json.py: 2 changes [ 1 2 ]
/trunk/reviewboard/webapi/tests.py: 4 changes [ 1 2 3 4 ]

Expand changes

         _("%s's review requests") % username)
     return datagrid.render_to_response(template_name)
-def _query_for_diff(review_request, revision, query_extra=None):
+def _query_for_diff(review_request, user, revision, query_extra=None):
     """
     Queries for a diff based on several parameters.
     If the draft does not exist, this throws an Http404 exception.
     """
     # Normalize the revision, since it might come in as a string.
     if revision != None:
         revision = int(revision)
     if user.is_authenticated() and review_request.submitter == user:
-    # This will try to grab the diff associated with a draft if the review
+        # This will try to grab the diff associated with a draft if the review
-    # request has an associated draft and is either the revision being
+        # request has an associated draft and is either the revision being
-    # requested or no revision is being requested.
+        # requested or no revision is being requested.
-    draft = get_object_or_none(review_request.reviewrequestdraft_set)
+        draft = get_object_or_none(review_request.reviewrequestdraft_set)
-    if draft and draft.diffset and \
+        if draft and draft.diffset and \
     A wrapper around diffviewer.views.view_diff that handles querying for
     diffs owned by a review request,taking into account interdiffs and
     providing the user's current review of the diff if it exists.
     """
     review_request = get_object_or_404(ReviewRequest, pk=review_request_id)
-    diffset = _query_for_diff(review_request, revision)
+    diffset = _query_for_diff(review_request, request.user, revision)
     interdiffset = None
     interdiffset_id = None
     review = None
     draft = None
     if interdiff_revision and interdiff_revision != revision:
         # An interdiff revision was specified. Try to find a matching
         # diffset.
-        interdiffset = _query_for_diff(review_request, interdiff_revision)
+        interdiffset = _query_for_diff(review_request, request.user,
                                        interdiff_revision)
         interdiffset_id = interdiffset.id
     if request.user.is_authenticated():
         # Try to find an existing pending review of this diff from the
         # current user.
         review = get_object_or_none(Review,
                                     user=request.user,
                                     review_request=review_request,
                                     public=False,
                                     base_reply_to__isnull=True)
         draft = get_object_or_none(review_request.reviewrequestdraft_set,
                                    review_request__submitter=request.user)
-    draft = get_object_or_none(review_request.reviewrequestdraft_set)
     repository = review_request.repository
     has_draft_diff = draft and draft.diffset
     is_draft_diff = has_draft_diff and draft.diffset == diffset
     is_draft_interdiff = has_draft_diff and interdiffset and \
                          draft.diffset == interdiffset
     return view_diff(request, diffset.id, interdiffset_id, {
         'review': review,
         'review_request': review_request,
         'review_request_details': draft or review_request,
         'draft': draft,
         'is_draft_diff': is_draft_diff,
         'is_draft_interdiff': is_draft_interdiff,
         'upload_diff_form': UploadDiffForm(repository),
         'upload_screenshot_form': UploadScreenshotForm(),
         'scmtool': repository.get_scmtool(),
     }, template_name)
 @check_login_required
 def raw_diff(request, review_request_id, revision=None):
     """
     Displays a raw diff of all the filediffs in a diffset for the
     given review request.
     """
     review_request = get_object_or_404(ReviewRequest, pk=review_request_id)
-    diffset = _query_for_diff(review_request, revision)
+    diffset = _query_for_diff(review_request, request.user, revision)
     data = ''.join([filediff.diff for filediff in diffset.files.all()])
     resp = HttpResponse(data, mimetype='text/x-patch')
     resp['Content-Disposition'] = 'inline; filename=%s' % diffset.name
         query_extra = Q(reviewrequestdraft=draft)
     except ReviewRequestDraft.DoesNotExist:
         query_extra = None
     if interdiff_revision is not None:
-        interdiffset = _query_for_diff(review_request, interdiff_revision,
+        interdiffset = _query_for_diff(review_request, request.user,
-                                       query_extra)
+                                       interdiff_revision, query_extra)
         interdiffset_id = interdiffset.id
         # Only the interdiff should have an extra query for the draft.
         # It's going to be the most recent diff (generally). We should be
         # smarter and check.
         query_extra = None
     else:
         interdiffset_id = None
-    diffset = _query_for_diff(review_request, revision, query_extra)
+    diffset = _query_for_diff(review_request, request.user, revision,
                               query_extra)
     return view_diff_fragment(request, diffset.id, filediff_id,
                               interdiffset_id, chunkindex, collapseall,
                               template_name)

 <a name="index_header"></a>
 {% with review_request.diffset_history.diffset_set.latest.revision as latest_revision %}
 {% with diffset.revision as revision %}
 <div class="main">
 {%  if interdiffset %}
 {#   We're showing an interdiff, so indicate the revision range. #}
  <h1>{% blocktrans with interdiffset.revision as interdiff_revision %}Changes between revision {{revision}} and {{interdiff_revision}}{% endblocktrans %}</h1>
 {%  else %}
 {%   ifequal diffset.revision latest_revision %}
  <h1>{% blocktrans %}Diff revision {{revision}} (Latest){% endblocktrans %}</h1>
 {%   else %}
 {#    This is not the most recent diff. See if we're showing a draft. #}
 {%    if is_draft_diff %}
  <h1>{% trans "Draft diff" %}</h1>
  <p>
 {%     blocktrans %}
   This diff is part of your current draft. Other users will not see this
   diff until you publish your draft.
 {%     endblocktrans %}
 {%    else %}
 {#     This is not a draft. Tell the user this is not the most recent. #}
  <h1>{% blocktrans %}Diff revision {{revision}}{% endblocktrans %}</h1>
  <p>
-{%    blocktrans with diffset.revision as revision and review_request.get_absolute_url as review_request_url%}
+{%     blocktrans with diffset.revision as revision and review_request.get_absolute_url as review_request_url%}
   This is not the most recent revision of the diff. The
   <a href="{{review_request_url}}diff/">latest diff</a> is revision
   {{latest_revision}}.
   <a href="{{review_request_url}}diff/{{revision}}-{{latest_revision}}/">See what's changed.</a>
-{%    endblocktrans %}
+{%     endblocktrans %}
 {%    endif %}
  </p>
 {%   endifequal %}
 {%  endif %}
 {#  Notify the user if they have unpublished comments in other diffs. #}
 {%  if review|has_comments_in_diffsets_excluding:diffset_pair %}
 {%   box "important" %}
  <h1>{% trans "You have unpublished comments on other revisions" %}</h1>
  <p>
   {% trans "Your review consists of comments on the following revisions:" %}
  </ul>
  {%  endbox %}
 {%  endif %}
 {%  ifnotequal review_request.diffset_history.diffset_set.count 1 %}
 {#   Show a revision selector for jumping between revisions. #}
  <table class="revision-selector">
   <tr>
    <th><label for="jump_to_revision">Jump to revision:<label></th>
    <td>
     <div id="jump_to_revision" class="revisions">
 {%  for item in review_request.diffset_history|revision_link_list:diffset_pair %}
 {%   if item.is_current %}
      <span class="current">{{item.revision}}</span>
 {%   else %}
      <a href="{{review_request.get_absolute_url}}diff/{{item.revision}}/#index_header">{{item.revision}}</a>
 {%   endif %}
 {%  endfor %}
 {%  if draft and draft.diffset %}
 {#   There's a draft diff, so display it in the list. #}
 {%   if is_draft_diff %}
      <span class="current">{{draft.diffset.revision}}</span>
 {%   else %}
      <a href="{{review_request.get_absolute_url}}diff/{{draft.diffset.revision}}/#index_header">{{draft.diffset.revision}}</a>
 {%   endif %}
 {%  endif %}
     </div>
    </td>
   </tr>
   <tr>
    <th><label for="show_interdiff">Changes between r{{revision}} and:</label></th>
      <span class="current">{{item.revision}}</span>
 {%   else %}
      <a href="{{review_request.get_absolute_url}}diff/{{item.path}}/#index_header">{{item.revision}}</a>
 {%   endif %}
 {%  endfor %}
 {%  if draft and draft.diffset %}
 {#   There's a draft diff, so display it in the list. #}
 {%   if is_draft_diff or is_draft_interdiff %}
      <span class="current">{{draft.diffset.revision}}</span>
 {%   else %}
      <a href="{{review_request.get_absolute_url}}diff/{{diffset_pair.0}}-{{diffset_pair.1}}/#index_header">{{draft.diffset.revision}}</a>
 {%   endif %}
 {%  endif %}
     </div>
    </td>
   </tr>
  </table>
 {% endifnotequal %}

     if not form.is_valid():
         return WebAPIResponseFormError(request, form)
     try:
-        diffset = form.create(request.FILES['path'],
+        diffset = form.create(request.FILES['path'])
                               review_request.diffset_history)
         # Set the initial revision to be one newer than the most recent
         # public revision, so we can reference it in the diff viewer.
+        #
         # It would be nice to later consolidate this with the logic in
         # DiffSet.save.
         public_diffsets = review_request.diffset_history.diffset_set
         if public_diffsets.count() > 0:
             diffset.revision = public_diffsets.latest().revision + 1
             diffset.save()
         else:
             diffset.revision = 1
     except scmtools.FileNotFoundError, e:
         return WebAPIResponseError(request, REPO_FILE_NOT_FOUND, {
             'file': e.path,
             'revision': e.revision
         })

         x, y, w, h = 2, 2, 10, 10
         screenshot = self.testNewScreenshot()
         review_request = screenshot.review_request.get()
         diffset = self.testNewDiff(review_request)
         rsp = self.apiPost("reviewrequests/%s/draft/save" % review_request.id)
         self.assertEqual(rsp['stat'], 'ok')
         self.postNewDiffComment(review_request, diff_comment_text)
         self.postNewScreenshotComment(review_request, screenshot,
                                       screenshot_comment_text, x, y, w, h)
         # Create a review request for this test.
         review_request = self.testNewReviewRequest()
         # Upload the first diff and publish the draft.
-        diffset = self.testNewDiff(review_request)
+        diffset_id = self.testNewDiff(review_request).id
         rsp = self.apiPost("reviewrequests/%s/draft/save" % review_request.id)
         self.assertEqual(rsp['stat'], 'ok')
         # Upload the second diff and publish the draft.
-        interdiffset = self.testNewDiff(review_request)
+        interdiffset_id = self.testNewDiff(review_request).id
         rsp = self.apiPost("reviewrequests/%s/draft/save" % review_request.id)
         self.assertEqual(rsp['stat'], 'ok')
         # Reload the diffsets, now that they've been modified.
         diffset = DiffSet.objects.get(pk=diffset_id)
         interdiffset = DiffSet.objects.get(pk=interdiffset_id)
         # Get the interdiffs
         filediff = diffset.files.all()[0]
         interfilediff = interdiffset.files.all()[0]
         rsp = self.apiPost(

/trunk/reviewboard/reviews/views.py: 15 changes [ 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 ]
/trunk/reviewboard/templates/diffviewer/view_diff.html: 9 changes [ 1 2 3 4 5 6 7 8 9 ]
/trunk/reviewboard/webapi/json.py: 2 changes [ 1 2 ]
/trunk/reviewboard/webapi/tests.py: 4 changes [ 1 2 3 4 ]

Review Board

beta

This change has been marked as submitted.

Fix several issues with draft diffs

Diff revision 4 (Latest)

Other reviews

Your comment