Add a new batch operation view

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

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

'typing.TYPE_CHECKING' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

For functions that take lots of arguments, I'd like to get in the habit of making the arguments keyword-only (short …

chipx86chipx86

This argument isn't documented.

chipx86chipx86

This should have a Version Added.

chipx86chipx86

The description and argument name don't seem to match? Can we clarify the name?

chipx86chipx86

The type for user wouldn't allow this, so I think the type should probably be Optional[User]? Also, can you run …

chipx86chipx86

I'd expect updating this to fail type checking. Can we add types for this and anything else that doesn't infer …

chipx86chipx86

If not properly prepared for this function, the following will result in database queries: review.comments review.user reply.comments reply.user reply.base_reply_to That …

chipx86chipx86

We should add a Dict[str, Any] type here.

chipx86chipx86

T before U.

chipx86chipx86

Can you add -> str:?

chipx86chipx86

Can you update this to use the style used elsewhere where each arg gets its own line?

chipx86chipx86

Trying to keep newer versions first, since those are the ones more relevant when reading through docs.

chipx86chipx86

Rather than Union[..., None], let's do Optional[Union[...]]. While equivalent, it conveys the intent a bit better, and is more consistent.

chipx86chipx86

The ) should be on the next line, with a return type. This parameter should have a trailing comma.

chipx86chipx86

The descriptions need to be indented 1 more space.

chipx86chipx86

While these are being changed, can we update them to use the full class path?

chipx86chipx86

These should be swapped.

chipx86chipx86

reviewboard.testing should be after reviewboard.site.

chipx86chipx86

No period needed here.

maubinmaubin

No period needed here.

maubinmaubin

We could do a .only('visibility') here and reduce what the test has to fetch.

chipx86chipx86

This could be a good thing to add to Djblets' TestCase class, would be useful for other API tests.

maubinmaubin

Can we add doc comments for each of these? Especially given that last comment.

chipx86chipx86

Left-over debugging.

chipx86chipx86

This is undocumented.

chipx86chipx86

Given exc_info=True, this can be logger.exception(). Same with others.

chipx86chipx86

This could probably fit fine with user= beginning on the review.publish line.

chipx86chipx86

Blank line between these.

chipx86chipx86

Params can probably begin on the send_email( line.

chipx86chipx86

Might be worth adding a bulk-updating function for visibility. (Doesn't have to be this change.)

chipx86chipx86

This is undocumented.

chipx86chipx86

As above, this can be logger.exception

chipx86chipx86

This is undocumented.

chipx86chipx86

As above, it'd be nice to support bulk visibility updating.

chipx86chipx86

Can you add a Version Added at the end of this?

chipx86chipx86

Should add a Raises for the Http404.

chipx86chipx86

Can you sort these alphabetically?

chipx86chipx86

It'd be nice to do one query with the combined list, and then sort them into reviews vs. replies in-code. …

chipx86chipx86

continuation line under-indented for visual indent Column: 17 Error code: E128

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
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
maubin
  1. 
      
  2. No period needed here.

  3. No period needed here.

  4. This could be a good thing to add to Djblets' TestCase class, would be useful for other API tests.

    1. Not going to do this now but I'll stick it in the back of my head.

    2. Sounds good.

  5. 
      
chipx86
  1. A lot of this is typing or doc stuff.

    Some of it is optimization questions.

    Overall though, it looks great!

  2. For functions that take lots of arguments, I'd like to get in the habit of making the arguments keyword-only (short of any that make sense as positional). This helps avoid complications down the road if we're trying to add or, especially, deprecate arguments.

    Can we add a * as the first argument here?

  3. This argument isn't documented.

  4. reviewboard/notifications/email/message.py (Diff revision 9)
     
     
     
     

    This should have a Version Added.

  5. reviewboard/notifications/email/message.py (Diff revision 9)
     
     
     

    The description and argument name don't seem to match? Can we clarify the name?

  6. reviewboard/notifications/email/message.py (Diff revision 9)
     
     
     

    The type for user wouldn't allow this, so I think the type should probably be Optional[User]?

    Also, can you run the type checker through this file again? There's some things in here I'd expect it to notice.

  7. I'd expect updating this to fail type checking. Can we add types for this and anything else that doesn't infer from a call?

  8. reviewboard/notifications/email/message.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    If not properly prepared for this function, the following will result in database queries:

    • review.comments
    • review.user
    • reply.comments
    • reply.user
    • reply.base_reply_to

    That can get expensive fast.

    There's a lot going into this. Can we verify that we're not re-querying anything in this code here? If we are, we should fix that up before the call.

    Ideally this call would be able to assert that it won't result in any new calls, but that means digging into model caches.. :/

  9. We should add a Dict[str, Any] type here.

  10. reviewboard/reviews/managers.py (Diff revision 9)
     
     

    T before U.

  11. reviewboard/reviews/managers.py (Diff revision 9)
     
     

    Can you add -> str:?

  12. reviewboard/reviews/managers.py (Diff revision 9)
     
     
     
     
     
     
     
     

    Can you update this to use the style used elsewhere where each arg gets its own line?

    1. Isn't that what this is?

  13. reviewboard/reviews/managers.py (Diff revision 9)
     
     
     
     

    Trying to keep newer versions first, since those are the ones more relevant when reading through docs.

  14. reviewboard/reviews/managers.py (Diff revision 9)
     
     

    Rather than Union[..., None], let's do Optional[Union[...]]. While equivalent, it conveys the intent a bit better, and is more consistent.

  15. reviewboard/reviews/managers.py (Diff revision 9)
     
     

    The ) should be on the next line, with a return type. This parameter should have a trailing comma.

  16. reviewboard/reviews/models/review_request.py (Diff revision 9)
     
     
     
     
     
     
     
     

    The descriptions need to be indented 1 more space.

  17. reviewboard/reviews/models/review_request.py (Diff revision 9)
     
     
     
     
     
     
     

    While these are being changed, can we update them to use the full class path?

  18. These should be swapped.

  19. reviewboard.testing should be after reviewboard.site.

  20. reviewboard/reviews/tests/test_batch_view.py (Diff revision 9)
     
     
     
     

    We could do a .only('visibility') here and reduce what the test has to fetch.

    1. .only doesn't work with .get. Given that this is a tiny object being fetched in a unit test that's only run a few times, I don't think we need to go too crazy on the optimization here.

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

    Can we add doc comments for each of these? Especially given that last comment.

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

    Left-over debugging.

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

    This is undocumented.

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

    Given exc_info=True, this can be logger.exception().

    Same with others.

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

    This could probably fit fine with user= beginning on the review.publish line.

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

    Blank line between these.

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

    Params can probably begin on the send_email( line.

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

    Might be worth adding a bulk-updating function for visibility. (Doesn't have to be this change.)

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

    This is undocumented.

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

    As above, this can be logger.exception

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

    This is undocumented.

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

    As above, it'd be nice to support bulk visibility updating.

  33. reviewboard/reviews/views/email.py (Diff revision 9)
     
     
     
     

    Can you add a Version Added at the end of this?

  34. reviewboard/reviews/views/email.py (Diff revision 9)
     
     
     
     
     
     
     

    Should add a Raises for the Http404.

  35. reviewboard/reviews/views/email.py (Diff revision 9)
     
     
     
     
     
     
     
     

    Can you sort these alphabetically?

  36. reviewboard/reviews/views/email.py (Diff revision 9)
     
     
     

    It'd be nice to do one query with the combined list, and then sort them into reviews vs. replies in-code. One less database query.

  37. 
      
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-6.x (21c5669)
Loading...