-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement file approval backend #174
Conversation
f6ef1d5
to
b97caae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good, all the right pieces in place.
Left a few comments.
However, I am not a fan of get_file_approvals design in the ticket, and deserialisation between the DAL and the BLL.
From my comment there:
I would much prefer it if business_logic.RequestFile grew an
.approvals attr, which is a list of the new
business_logic.FileApproval objects for that file. And that when
get_release_request will build up the full tree of objects via its
existing from_dict mechanism, and we can just iterate the one
object, rather than integrating two data sources.
When we call bll.get_release_request(...)
, we want to get back a single object representation of everything, like so:
ReleaseRequest
.filegroups[]
.files[]
.reviews[]
Rather than having to call .get_release_request and also then .get_file_approvals and pass both into wherever they are used, its all in the one object tree.
I cannot think of a use case for getting the approvals alone, when we don't also need everything else, but maybe I've missed that?
I think if we do this, it should solve a few issues with this PR atm:
- removes the need for the circular FileReview.release_request reference
- makes the from_dict implementation simpler.
b97caae
to
7c972e0
Compare
67b276c
to
df04eb6
Compare
* because we are only unit testing the BLL and not the DAL currently, these handlers cause test coverage to dip from 100% to 99.88%.
is_supporting_file identifies whether a file is a supporting file from a UrlPath. This is different to a ReleaseFile's relpath (it includes the filegroup, which is part of the url for a release file, but is not part of the relpath, which comes from its location in the workspace).
* as per PR review
e767c54
to
33034dc
Compare
I started on this path because I wanted exactly that for the unit tests & then decided to proceed with the implementation in the original ticket. Having looked at it again, though, I think you're absolutely right so have re-worked this in the way that you suggest. |
a034fd0
to
86eaf67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice, thanks for reworking those bits. I think its worked out as I'd hoped 🙏
Left a couple of suggestions, but happy to land this!
a5f1552
to
841f2a9
Compare
as per #137