create file approve & reject urls #193
Conversation
madwort
commented
Mar 22, 2024
•
madwort
commented
- I think this fixes Support approving/rejecting individual files #137
madwort
force-pushed
the
madwort/implement-file-approval-frontend
branch
2 times, most recently
from
ecb3fb7
to
c1ef7c3
Compare
| if path == "": | ||
| file_approve_url = None | ||
| file_reject_url = None | ||
| else: |
There was a problem hiding this comment.
nb. not a big fan of this big indent block, might refactor before asking for final review
There was a problem hiding this comment.
Possible alternative: pass the file relpath as a POST parameter like we do for the workspace_add_file_to_request view, so the url is just /requests/approve/<str:request_id>/. The template logic already deals with things that are or aren't files (displaying content etc)
madwort
force-pushed
the
madwort/implement-file-approval-frontend
branch
3 times, most recently
from
c1356b1
to
0aec1ac
Compare
| if path == "": | ||
| file_approve_url = None | ||
| file_reject_url = None | ||
| else: |
There was a problem hiding this comment.
Possible alternative: pass the file relpath as a POST parameter like we do for the workspace_add_file_to_request view, so the url is just /requests/approve/<str:request_id>/. The template logic already deals with things that are or aren't files (displaying content etc)
| if existing_reviews[0].status == FileReviewStatus.APPROVED: | ||
| file_approve_url = None | ||
| else: | ||
| file_reject_url = None |
There was a problem hiding this comment.
This check could move to the ReleaseRequest, and do something similar to is_supporting_file to get the RequestFile from the UrlPath, and look at its reviews for this user. It shouldn't need to know about the group then.
(We also don't want to show approve or reject buttons for supporting files, so it'll need to check the filetype too)
There was a problem hiding this comment.
have added handling for supporting files & refactored a bit to use ReleaseRequest rather than think about groups here
madwort
force-pushed
the
madwort/implement-file-approval-frontend
branch
6 times, most recently
from
0304705
to
466faf6
Compare
madwort
force-pushed
the
madwort/implement-file-approval-frontend
branch
2 times, most recently
from
027f572
to
dad5591
Compare
| @@ -401,6 +401,16 @@ def output_files_set(self): | |||
| for request_file in filegroup.output_files | |||
| } | |||
|
|
|||
| def get_file_review_for_reviewer(self, urlpath: UrlPath, reviewer: str): | |||
There was a problem hiding this comment.
Should this be a BLL method that calls down to the DAL (like get_release_request), rather than a method on ReleaseRequest? Then we can pass actually retrieving the file review down to the local_db provider, and it becomes a django FileReview.objects.filter(...).first()
There was a problem hiding this comment.
hmm, yeah, the original ticket specified a get_file_approvals() like that, which I initially did, and then removed after PR review from Simon #174 (review) in favour of keeping the interface between the BLL & DAL just the one big call with the entire state of everything.
Obviously a method that calls down to the DAL would have dodged the issue in tests with stale release_request objects, but refreshing the release_request seems ok in that situation?
There was a problem hiding this comment.
I think this could be the use case Simon mentioned for just getting a file review without the rest of the release request? Although, I guess we did fetch it all already, and getting the release request already extracted all the reviews from the database, so this avoids another db call
madwort
force-pushed
the
madwort/implement-file-approval-frontend
branch
2 times, most recently
from
e392a28
to
1cc8783
Compare
* remove ugly equivalent tests in test_request
madwort
force-pushed
the
madwort/implement-file-approval-frontend
branch
from
1cc8783
to
1d63e5f
Compare
madwort
force-pushed
the
madwort/implement-file-approval-frontend
branch
from
1d63e5f
to
ceecbac
Compare
