Fix file attachment revision info with draft updates.
Review Request #14021 — Created July 10, 2024 and submitted
When a file attachment has an updated revision in a pending draft, that
revision could leak into the page for users not able to see the draft.
If they then moved the revision slider to show the new revision, they'd
get a 404.This change makes it so we check the last attachment in the history to
see if it's a draft, and only include it in the revision list if that
draft should be visible.
- Ran unit tests.
- Created a draft with an updated revision to a file. Saw that the owner
saw two revisions, a regular user saw one, and admins saw either one
or two depending on if they were choosing to view draft data.
Summary | ID |
---|---|
012b6fe552ad20450fcd500a6d7259c93f6a9e8a |
Description | From | Last Updated |
---|---|---|
continuation line under-indented for visual indent Column: 21 Error code: E128 |
![]() |
|
line too long (80 > 79 characters) Column: 80 Error code: E501 |
![]() |
|
closing bracket does not match indentation of opening bracket's line Column: 25 Error code: E123 |
![]() |
|
Left-over debug code. |
|
|
Can we use keyword arguments to help self-document this? We should also explicitly compare request against None, to ensure a … |
|
|
Let's protect against an empty list of files. I know we have a FileAttachmentHistory, but I'd feel more comfortable if … |
|
|
I had to re-read this several times. It's late, so that's maybe part of it, but I think this might … |
|
- Commits:
-
Summary ID eb3dbcf840e82da0cecd841e042ef01794af5a7e 51e65e7385d78587884327155a016d8741dc4b2a
Checks run (2 succeeded)
-
-
-
Can we use keyword arguments to help self-document this?
We should also explicitly compare
request
againstNone
, to ensure abool
result. -
Let's protect against an empty list of files. I know we have a
FileAttachmentHistory
, but I'd feel more comfortable if we safeguard against something having gone wrong with entries here.
- Commits:
-
Summary ID 51e65e7385d78587884327155a016d8741dc4b2a 84257474feab38c4516d67c1b7609a4baf1dfb1a
Checks run (2 succeeded)
-
-
I had to re-read this several times. It's late, so that's maybe part of it, but I think this might be more clear as:
if (include_draft or review_request.get_file_attachment_state( last_attachment ) not in ( FileAttachmentState.NEW, FileAttachmentState.NEW_REVISION, )):
Still kind of weird, but keeps the call and the list from running together.