Add a new batch operation view

Review Request #12695 — Created Oct. 20, 2022 and updated

david
Review Board
release-6.x
reviewboard

For upcoming feature work, we want the ability to make a single request
to publish a bunch of individual items (review requests and reviews) all
together.

To do this, we're introducing a new API-like endpoint for performing
batch operations. This doesn't exist in the official API because it
doesn't really mesh well with REST, and the actual endpoint may be in
flux for a bit.

We already have some batch-like operations in the dashboard UI that
currently just make a whole bunch of API requests for closing review
requests or changing visibility state. This new endpoint will also
enable these workflows, so we can handle it much more efficiently.

The new batch endpoint supports these operations:

  • close
  • discard
  • archive
  • mute
  • unarchive (effectively unmute too)
  • publish
  • Looked at e-mail previews for batch publishes with all combinations of
    review requests, review request updates, reviews, and review replies in
    both HTML and text mode.
  • Used all operations except for publish from real UI.
  • Ran unit tests.
Summary
Add a new batch operation view
Description From Last Updated

I don't see any tests for when there's more than one review or review request and one of them fails …

maubinmaubin

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

local variable 'draft' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

local variable 'draft' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

local variable 'draft' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

local variable 'draft' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 21 Error code: E131

reviewbotreviewbot

continuation line over-indented for hanging indent Column: 21 Error code: E126

reviewbotreviewbot

continuation line over-indented for hanging indent Column: 21 Error code: E126

reviewbotreviewbot

line too long (81 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

continuation line unaligned for hanging indent Column: 17 Error code: E131

reviewbotreviewbot

continuation line over-indented for hanging indent Column: 21 Error code: E126

reviewbotreviewbot

continuation line over-indented for hanging indent Column: 21 Error code: E126

reviewbotreviewbot

Missing , optional

chipx86chipx86

Mising , optional

chipx86chipx86

Missing , optional

chipx86chipx86

While here, can you move this above Args (and make it Version Changed)?

chipx86chipx86

This should also go up above and use Version Changed. We've mostly been doing it in order from newest to …

chipx86chipx86

Since this is a private utility endpoint, let's call this _batch/.

chipx86chipx86

Needs a Version Added.

chipx86chipx86

Args are indented too far.

chipx86chipx86

We need to use the accessible queries, or this will end up letting people perform operations on review requests they …

chipx86chipx86

In the common case, we'll have the right number of IDs. So instead of a separate COUNT() query, let's jsut …

chipx86chipx86

We can simplify this if we just define the proper ID field in the conditional and perform the lookup outside …

chipx86chipx86

Let's sort this result, so errors are consistent.

chipx86chipx86

Can we move this entirely into its own function, like the other batch operations?

chipx86chipx86

This also needs to be an accessible check.

chipx86chipx86

Same note above about saving a COUNT() query.

chipx86chipx86

Given the complexity of the arguments (and usage of flags), it'd be nice to force keyword arguments. Same for others.

chipx86chipx86

For consistency with other errors elsewhere, these should all end with a period.

chipx86chipx86

Since we need to pull drafts, our initial query should do a prefetch or a select_related.

chipx86chipx86

We should catch errors here. Extensions can block publishes.

chipx86chipx86

We should catch errors here.

chipx86chipx86

Could mention this PublishError in the docstring.

maubinmaubin

Should we check that the error messages in the responses match what we expect them to be?

maubinmaubin

Same here and for other invalid requests. I think it would be a good idea to check the error message …

maubinmaubin

Seems like there should be a "with" here: "publish op with archive after publish".

maubinmaubin

Should we add typing here and for any other helper methods in test files?

maubinmaubin

local variable 'queries' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

local variable 'queries' is assigned to but never used Column: 9 Error code: F841

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

flake8

david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
maubin
  1. Looks pretty good so far.

  2. I wonder if it would be worthwhile for failed responses to include comprehensive information about every object that causes a fail, instead of just returning a response for the first one that causes a fail. That way people wouldn't have to make a bunch of batch requests and keep discovering errors one by one. But I don't know if it makes sense to return lists of status codes and error messages in a response. What might be better is to gather lists of objects that cause each error, and then check if each list is non-empty, if so return a failed response for that type of error and include all of the items in the list in the error message. So messages would be like User does not have permission to publish review request(s): 1, 2, 3..

  3. 
      
chipx86
  1. Overall design looks good. Just to help solidify it, it'd be nice to document the format of each possible request payload in the class's docstring.

    Some of my comments are things that I know are in flight (and even mentioned in the description), but I'm leaving my thoughts anyway.

    The most critical two things in my mind are:

    1. This needs to be more query-efficient. There are places we're going to be repeatedly querying the database, which can lead to poor performance. Every unit test should use assertQueries().
    2. There are some potential security issues in the way that review requests and reviews are being fetched. We should make sure to use accessible checks for possible review requests or reviews, so we centralize that logic and avoid exposing information about review requests or reviews that should be hidden.

    Regarding the validation stuff mentioned in the review request description...

    I like the idea of being able to validate all the publishes before we publish, and would prefer that if it was easy to do. But I don't think it is. There are a couple of problems:

    1. We could still get errors from writing the review request (such as conflicts) or from the review_request_published signal.
    2. We'd be changing the nature of validation (an extension signal handler on review_request_publishing responding to review request #2 might be sensitive to the published state of review request #1, or that of state expected to be handled in a review_request_published).

    (The second point presents a larger issue of, do we need to think about the order in which review requests are published? E.g., following depends_on. That's probably not something to solve right now, but worth considering.)

    I think ultimately, we'd be better off doing the full publish operation one-by-one. It's less work, it's probably safer, and it keeps in line with standard behavior.

    I do think we should bail at the first review request that fails, though. That's less likely to leave the rest of the review requests in a bad state.

    We can provide a list of all review request IDs that published and all that have not, in the order of publish, allowing the caller to continue where it left off if necessary. (Especially useful if we do later make this public API in some form.)

    1. The most common case from our UI will be publishing a review request update and one or more replies. At the moment we won't have any UI that does more than one review request, so I think if we fail on publishing the review request and bail immediately, we'll end up with a nice user experience.

  2. reviewboard/reviews/models/review.py (Diff revision 3)
     
     

    Missing , optional

  3. Mising , optional

    1. This is not actually optional now that we've gotten rid of type. I'll fix it up.

  4. Missing , optional

  5. reviewboard/reviews/models/review_request.py (Diff revision 3)
     
     
     
     
     
     
     

    While here, can you move this above Args (and make it Version Changed)?

  6. reviewboard/reviews/models/review_request.py (Diff revision 3)
     
     
     
     

    This should also go up above and use Version Changed.

    We've mostly been doing it in order from newest to oldest.

  7. reviewboard/reviews/urls.py (Diff revision 3)
     
     

    Since this is a private utility endpoint, let's call this _batch/.

  8. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     
     
     
     
     

    Needs a Version Added.

  9. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     
     
     
     
     
     

    Args are indented too far.

  10. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     
     
     
     
     
     
     

    We need to use the accessible queries, or this will end up letting people perform operations on review requests they don't have access to.

    Even if we perform access checks later, we don't want to risk the API being used to even infer anything about inaccessible IDs.

  11. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     

    In the common case, we'll have the right number of IDs. So instead of a separate COUNT() query, let's jsut fetch the found_ids and then compare counts. This will save a query per batch operation in most cases.

  12. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     
     
     
     
     

    We can simplify this if we just define the proper ID field in the conditional and perform the lookup outside of it.

  13. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     

    Let's sort this result, so errors are consistent.

  14. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     

    Can we move this entirely into its own function, like the other batch operations?

    1. I felt like the separation I have is good -- queries happen inside post(), and then doing stuff with the results of those queries happen in the individual methods.

  15. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     
     
     

    This also needs to be an accessible check.

  16. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     

    Same note above about saving a COUNT() query.

  17. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     
     
     
     
     

    Given the complexity of the arguments (and usage of flags), it'd be nice to force keyword arguments.

    Same for others.

  18. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     
     

    For consistency with other errors elsewhere, these should all end with a period.

  19. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     

    Since we need to pull drafts, our initial query should do a prefetch or a select_related.

  20. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     
     
     
     

    We should catch errors here. Extensions can block publishes.

  21. reviewboard/reviews/views/batch.py (Diff revision 3)
     
     

    We should catch errors here.

  22. 
      
david
david
maubin
  1. This is a lot of work, nice job!

  2. I don't see any tests for when there's more than one review or review request and one of them fails to publish/close (leading to a mixed stat in the response).

  3. Could mention this PublishError in the docstring.

  4. Should we check that the error messages in the responses match what we expect them to be?

  5. Same here and for other invalid requests. I think it would be a good idea to check the error message of the response so that we know exactly why the request is failing, and that this matches what we expect.

  6. Seems like there should be a "with" here: "publish op with archive after publish".

  7. Should we add typing here and for any other helper methods in test files?

  8. 
      
david
Review request changed

Change Summary:

  • Fix an issue with publishing replies via batch.
  • Check response content to make sure errors match what we expect.
  • Fix up response content when reviews fail to publish.
  • Make other requested changes.

Commits:

Summary
-
Add a new batch operation view
+
Add a new batch operation view

Diff:

Revision 6 (+4446 -108)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
Review request changed
Loading...