Summary

Dramatically improve the experience for admins with other users' drafts.

Review Request #14020 — Created July 10, 2024 and submitted Aug. 6, 2024, 4:50 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

4659, 4770

Depends On

Reviewers

Groups

reviewboard

People

Description

For a very long time, we've had an annoying situation with privileged
users and drafts. Users who are admins or have the
can_edit_reviewrequest privilege have permission to create or edit
drafts on other people's review requests, but we had some
inconsistencies that made the experience less than desirable:

The vast majority of the time, users with privileges really just want
to operate as if they were a normal user doing reviews. Seeing the
draft data was annoying in this case.
Loading the diff attached to a draft would fail. We'd happily send the
user the diff context, but the diff fragment view was not properly
loading the draft, and would return 404s.
It was not clear at all that the admin user was seeing draft data.
Even as someone who has been using Review Board for all of its
existence, I frequently got confused.

This change makes a huge improvement to the user experience here. When
loading a review request, privileged users will see the published data
rather than the draft, but the unified banner will have a note that
there's an unpublished draft, with a link to reload the page with a new
?view-draft=1 parameter. When viewing the draft, that note tells them
that they're viewing a draft on a review request owned by another user,
and gives them a link to go back to the public data.

In cases where there is no public data to go back to (for example,
viewing a review request which has never been published, or a file
attachment which only exists on the draft), the notice is shown to the
user but there's no link in the banner.

This ends up adding three new pieces of data to the reviewable page
context:

user_draft_exists will be True if the review request is owned by
another user but there exists a draft which is accessible by the
person viewing the page.
viewing_user_draft will be True if the person viewing the page is
currently viewing data which is contained in the draft. This will
happen either if the current data is only available in the draft (such
as an unpublished review request) or if they explicitly wanted to see
it by including ?view-draft=1 in the URL.
force_view_user_draft will be True if the current page is only
available in draft form, and we want to suppress the link to go back
to published data.

While adding those to the context, I've also moved more things into the
make_review_request_context method in order to be more consistent
about how stuff ends up in the ReviewRequestContext dict, since we
were repeating ourselves a fair bit in each of the views that calls it.

There's one major piece of implementation left to do. This does not yet
change anything about the admin user actually making changes to the
review request/draft data. Now that the admin user will see non-draft
data by default, we will need some logic about what to do when they
start making changes, especially in the case where there's already a
draft present. This will be done in a separate change because this one
is already way too big.

There are also a couple of bugs I've discovered while implementing this,
which will be fixed in their own changes.

Testing Done


Ran Python unit tests.
Ran JS unit tests.
Created review requests with various draft states and checked access

  from the owner user, an admin user, and a regular user. These draft

  states include unpublished review requests that include both diffs and

  file attachments, and draft updates to review requests that include

  new revisions of diffs, new file attachments, or new revisions of

  public file attachments.

Commits

Summary	ID
Dramatically improve the experience for admins with other users' drafts. For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments. Fixes bugs 4659 and 4770.	7068b1886f96b2e8cb850c810be9b3dbc49fc3d8

Summary

Dramatically improve the experience for admins with other users' drafts.

For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments. Fixes bugs 4659 and 4770.

7068b1886f96b2e8cb850c810be9b3dbc49fc3d8

Files

Issues

Description	From	Last Updated
I feel like a different banner colour here would be nicer, make it stand out more. Maybe ink-p-accent-info-bg or ink-p-accent-highlight-bg?	maubin	July 22, 2024, 8:44 a.m.
Can you update this bottom border colour to match the new banner colour (--ink-p-accent-info-border-color)	maubin	July 29, 2024, 9:20 a.m.
continuation line unaligned for hanging indent Column: 5 Error code: E131	reviewbot	July 10, 2024, 2:20 p.m.
continuation line unaligned for hanging indent Column: 5 Error code: E131	reviewbot	July 10, 2024, 2:23 p.m.
continuation line unaligned for hanging indent Column: 5 Error code: E131	reviewbot	July 10, 2024, 2:23 p.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	July 10, 2024, 2:23 p.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	July 10, 2024, 2:23 p.m.
line too long (83 > 79 characters) Column: 80 Error code: E501	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
continuation line missing indentation or outdented Column: 13 Error code: E122	reviewbot	July 10, 2024, 2:23 p.m.
line too long (86 > 79 characters) Column: 80 Error code: E501	reviewbot	July 10, 2024, 2:25 p.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	July 10, 2024, 2:25 p.m.
missing whitespace around operator Column: 30 Error code: E225	reviewbot	July 10, 2024, 2:25 p.m.
undefined name 'Diffset' Column: 33 Error code: F821	reviewbot	July 10, 2024, 2:25 p.m.
Should we also do an admin check here? I know that we do checks later so that when a regular …	maubin	July 22, 2024, 8:08 a.m.
To reduce the number of lines and code repetition, you could turn this into a function that takes context, draft, …	maubin	July 22, 2024, 8:27 a.m.
For TypedDicts elsewhere, we've been making sure new fields have the Version Added directly.	chipx86	Aug. 5, 2024, 9:12 a.m.
I don't see anything replacing this variable.	chipx86	Aug. 5, 2024, 12:15 a.m.
These need to be explicitly typed as bool or they can end up as Literal[False].	chipx86	Aug. 5, 2024, 9:12 a.m.
Can we use keyword arguments for this?	chipx86	Aug. 5, 2024, 9:14 a.m.
Can we make these keyword-only?	chipx86	Aug. 5, 2024, 9:14 a.m.
Just a suggestion, but maybe move all of these inside an if draft, so that there's only one False code …	chipx86	Aug. 5, 2024, 9:14 a.m.
We check this in a couple of places. It might be nice to have a function somewhere that just wraps …	chipx86	Aug. 5, 2024, 9:22 a.m.
Version Added?	chipx86	Aug. 5, 2024, 9:22 a.m.
Version Added?	chipx86	Aug. 5, 2024, 9:22 a.m.
To keep tests speedy, let's fetch these at once. We can do: users = User.objects.in_bulk( ['doc', 'grumpy', 'admin', 'dopey'], field_name='username') …	chipx86	Aug. 5, 2024, 9:31 a.m.
Since this is a private method, it should go after the public methods.	chipx86	Aug. 5, 2024, 9:24 a.m.
""" should be on the next line. Same with the other tests below.	chipx86	Aug. 5, 2024, 9:25 a.m.
Rather than the cast every time, maybe the util function should just take a dictionary and then check for the …	chipx86	Aug. 5, 2024, 9:28 a.m.
Needs a Version Added.	chipx86	Aug. 5, 2024, 9:31 a.m.
Needs Version Added.	chipx86	Aug. 5, 2024, 9:30 a.m.
Needs Version Added.	chipx86	Aug. 5, 2024, 9:30 a.m.
Needs a Version Added.	chipx86	Aug. 5, 2024, 9:31 a.m.
We have model locally, so we can use that.	chipx86	Aug. 5, 2024, 9:31 a.m.
line too long (81 > 79 characters) Column: 80 Error code: E501	reviewbot	Aug. 5, 2024, 10:15 a.m.

flake8 failed.

JSHint passed.

flake8

```
Warning: Showing 30 of 52 failures.
```
reviewboard/reviews/context.py (Diff revision 1)
The issue has been resolved. Show all issues
```
continuation line unaligned for hanging indent

Column: 5
Error code: E131
```
reviewboard/reviews/context.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line unaligned for hanging indent

Column: 5
Error code: E131
```
reviewboard/reviews/context.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line unaligned for hanging indent

Column: 5
Error code: E131
```
reviewboard/reviews/context.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (83 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 1)
The issue has been dropped. Show all issues
```
continuation line missing indentation or outdented

Column: 13
Error code: E122
```

Commits:

	Summary	ID
	Dramatically improve the experience for admins with other users' drafts. For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments.	355e48fae8a2c941e5812d2f1d1c786b9e340a81
	Dramatically improve the experience for admins with other users' drafts. For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments.	b316920b0740112d1aef2b697cd63c0105bbcfa1

Diff:

Revision 2 (+3808 -368)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_other_user_drafts.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/attachments.py
	reviewboard/reviews/views/diffviewer.py
	13 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/ui/base.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (86 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/reviews/ui/base.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/reviews/ui/base.py (Diff revision 2)
The issue has been dropped. Show all issues
```
missing whitespace around operator

Column: 30
Error code: E225
```
reviewboard/reviews/views/diffviewer.py (Diff revision 2)
The issue has been dropped. Show all issues
```
undefined name 'Diffset'

Column: 33
Error code: F821
```

Commits:

	Summary	ID
	Dramatically improve the experience for admins with other users' drafts. For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments.	b316920b0740112d1aef2b697cd63c0105bbcfa1
	Dramatically improve the experience for admins with other users' drafts. For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments.	f10cd90108c460a8877bdb79683084e881f76f76

Diff:

Revision 3 (+3812 -368)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_other_user_drafts.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/attachments.py
	reviewboard/reviews/views/diffviewer.py
	13 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Draft available to view. (Revision 1)

The issue has been resolved. Show all issues

I feel like a different banner colour here would be nicer, make it stand out more. Maybe ink-p-accent-info-bg or ink-p-accent-highlight-bg?

reviewboard/reviews/context.py (Diff revision 3)

The issue has been dropped. Show all issues

Should we also do an admin check here? I know that we do checks later so that when a regular user actually tries to view someone's draft, they can't. But if a caller were to use this function and assume these were the only checks to be done, it would lead to the problem where regular users can see everyone's drafts. Either we do an admin check here too, or say in the docs that this does not check for whether the user has admin permissions.

david

July 22, 2024, 8:08 a.m.

The access control happens in review_request.get_draft(). I don't think it's helpful to do it twice.

reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 3)

The issue has been resolved. Show all issues

To reduce the number of lines and code repetition, you could turn this into a function that takes context, draft, mutable_by_user, force_view_user_draft, and etc as arguments. Can default all of the booleans to False and then for each test pass in True when needed. 
I've done this in the past and I know its a bit tedious to switch everything over :p but makes for much cleaner code, and if you ever have to add any new assertions, you only need to do it in one spot instead of having to repeat it for each unit test.

Added Files:

New banner color (Revision 1)

The issue has been dropped. Show all issues

Can you update this bottom border colour to match the new banner colour (--ink-p-accent-info-border-color)

david

July 22, 2024, 9:16 a.m.

Unfortunately this border is owned by the unified banner, not this notice.

Commits:

	Summary	ID
	Dramatically improve the experience for admins with other users' drafts. For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments.	f10cd90108c460a8877bdb79683084e881f76f76
	Dramatically improve the experience for admins with other users' drafts. For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments.	3c487ff7f6af3f514a4ef2b6866eea1119fcc75f

Diff:

Revision 4 (+3812 -368)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_other_user_drafts.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/attachments.py
	reviewboard/reviews/views/diffviewer.py
	13 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

	Summary	ID
	Dramatically improve the experience for admins with other users' drafts. For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments.	3c487ff7f6af3f514a4ef2b6866eea1119fcc75f
	Dramatically improve the experience for admins with other users' drafts. For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments.	04149ed84208e48038ad2b8ad49291a667ca6bd8

Diff:

Revision 5 (+3938 -370)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_other_user_drafts.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/attachments.py
	reviewboard/reviews/views/diffviewer.py
	14 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Bugs:: 4659, 4770

Ship it!

I still wish that we could fix the bottom border of the banner to match the new colour somehow, but other than that looks good.

Maybe in a separate change we could add some flag to the unified banner whenever this unpublished draft banner is present, and use that to change the bottom border colour?

chipx86

Aug. 4, 2024, 10:54 p.m.

We could opportunistically do it with a :has(...) CSS rule. Since it's pure style and not important for layout, it'd be safe.

reviewboard/reviews/context.py (Diff revision 5)

The issue has been resolved. Show all issues

For TypedDicts elsewhere, we've been making sure new fields have the Version Added directly.

reviewboard/reviews/context.py (Diff revision 5)

The issue has been dropped. Show all issues

I don't see anything replacing this variable.

david

Aug. 4, 2024, 11:56 p.m.

This wasn't used anywhere. I think it's a relic from long before we had the review request fields abstraction.

reviewboard/reviews/context.py (Diff revision 5)

The issue has been resolved. Show all issues

These need to be explicitly typed as bool or they can end up as Literal[False].

reviewboard/reviews/context.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Can we use keyword arguments for this?
```
reviewboard/reviews/context.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Can we make these keyword-only?
```

reviewboard/reviews/context.py (Diff revision 5)

The issue has been resolved. Show all issues

Just a suggestion, but maybe move all of these inside an if draft, so that there's only one False code path?

reviewboard/reviews/detail.py (Diff revision 5)

The issue has been resolved. Show all issues

We check this in a couple of places. It might be nice to have a function somewhere that just wraps this, so the logic never differs anywhere.

david

Aug. 5, 2024, 9:32 a.m.

Turns out this code was just left over from before I added the should_view_draft method.

reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Version Added?
```

reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 5)

The issue has been resolved. Show all issues

Version Added?

david

Aug. 4, 2024, 11:56 p.m.

I don't see the point of adding those for test code.

chipx86

Aug. 5, 2024, 12:17 a.m.

Not super important, just we've been doing that so far with new test files and classes. Mostly it's just useful when we have to figure out when things were introduced, more than anything.

reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 5)

The issue has been resolved. Show all issues

To keep tests speedy, let's fetch these at once.

We can do:

users = User.objects.in_bulk(
    ['doc', 'grumpy', 'admin', 'dopey'],
    field_name='username')

And then either just use that or we can assign each via a lookup in the dictionary.

reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Since this is a private method, it should go after the public methods.
```
reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 5)
The issue has been resolved. Show all issues
```
""" should be on the next line.
Same with the other tests below.
```

reviewboard/reviews/tests/test_other_user_drafts.py (Diff revision 5)

The issue has been dropped. Show all issues

Rather than the cast every time, maybe the util function should just take a dictionary and then check for the appropriate keys.

david

Aug. 4, 2024, 11:56 p.m.

cast is a no-op, and is purely a hint for type checkers.

chipx86

Aug. 5, 2024, 12:17 a.m.

Yeah, it's just noisy to have to repeat it everywhere. I don't feel strongly.

reviewboard/static/rb/js/reviews/models/reviewRequestEditorModel.ts (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs a Version Added.
```

reviewboard/static/rb/js/reviews/models/reviewRequestEditorModel.ts (Diff revision 5)

The issue has been resolved. Show all issues

Needs Version Added.

reviewboard/static/rb/js/reviews/models/unifiedBannerModel.ts (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs Version Added.
```
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs a Version Added.
```
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 5)
The issue has been resolved. Show all issues
```
We have model locally, so we can use that.
```

Commits:

	Summary	ID
	Dramatically improve the experience for admins with other users' drafts. For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments.	04149ed84208e48038ad2b8ad49291a667ca6bd8
	Dramatically improve the experience for admins with other users' drafts. For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments. Fixes bugs 4659 and 4770.	64d2e05d92ee127610054c95cee76fd839671f52

Diff:

Revision 6 (+4148 -376)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_other_user_drafts.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/attachments.py
	reviewboard/reviews/views/diffviewer.py
	14 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/detail.py (Diff revision 6)
The issue has been dropped. Show all issues
```
line too long (81 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Dramatically improve the experience for admins with other users' drafts. For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments. Fixes bugs 4659 and 4770.	64d2e05d92ee127610054c95cee76fd839671f52
	Dramatically improve the experience for admins with other users' drafts. For a very long time, we've had an annoying situation with privileged users and drafts. Users who are admins or have the `can_edit_reviewrequest` privilege have permission to create or edit drafts on other people's review requests, but we had some inconsistencies that made the experience less than desirable: - The vast majority of the time, users with privileges really just want to operate as if they were a normal user doing reviews. Seeing the draft data was annoying in this case. - Loading the diff attached to a draft would fail. We'd happily send the user the diff context, but the diff fragment view was not properly loading the draft, and would return 404s. - It was not clear at all that the admin user was seeing draft data. Even as someone who has been using Review Board for all of its existence, I frequently got confused. This change makes a huge improvement to the user experience here. When loading a review request, privileged users will see the published data rather than the draft, but the unified banner will have a note that there's an unpublished draft, with a link to reload the page with a new `?view-draft=1` parameter. When viewing the draft, that note tells them that they're viewing a draft on a review request owned by another user, and gives them a link to go back to the public data. In cases where there is no public data to go back to (for example, viewing a review request which has never been published, or a file attachment which only exists on the draft), the notice is shown to the user but there's no link in the banner. This ends up adding three new pieces of data to the reviewable page context: * `user_draft_exists` will be `True` if the review request is owned by another user but there exists a draft which is accessible by the person viewing the page. * `viewing_user_draft` will be `True` if the person viewing the page is currently viewing data which is contained in the draft. This will happen either if the current data is only available in the draft (such as an unpublished review request) or if they explicitly wanted to see it by including `?view-draft=1` in the URL. * `force_view_user_draft` will be `True` if the current page is only available in draft form, and we want to suppress the link to go back to published data. While adding those to the context, I've also moved more things into the `make_review_request_context` method in order to be more consistent about how stuff ends up in the `ReviewRequestContext` dict, since we were repeating ourselves a fair bit in each of the views that calls it. There's one major piece of implementation left to do. This does not yet change anything about the admin user actually making changes to the review request/draft data. Now that the admin user will see non-draft data by default, we will need some logic about what to do when they start making changes, especially in the case where there's already a draft present. This will be done in a separate change because this one is already way too big. There are also a couple of bugs I've discovered while implementing this, which will be fixed in their own changes. Testing Done: - Ran Python unit tests. - Ran JS unit tests. - Created review requests with various draft states and checked access from the owner user, an admin user, and a regular user. These draft states include unpublished review requests that include both diffs and file attachments, and draft updates to review requests that include new revisions of diffs, new file attachments, or new revisions of public file attachments. Fixes bugs 4659 and 4770.	7068b1886f96b2e8cb850c810be9b3dbc49fc3d8

Diff:

Revision 7 (+4150 -376)

Show changes

	reviewboard/reviews/context.py
	reviewboard/reviews/detail.py
	reviewboard/reviews/models/review_request_draft.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/reviews/tests/test_other_user_drafts.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/attachments.py
	reviewboard/reviews/views/diffviewer.py
	14 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.x (f695895)