-
Notifications
You must be signed in to change notification settings - Fork 16
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
Refactor extract File Metadata to its own model #285
Conversation
264 - Add the AccessFileMenu to File Page
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.
Looks good, I like how the code is shared between the Dataset Page and the File Page!
I just have some questions about file access logic.
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.
the issues were addressed, looks good!
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.
Code changes have been reviewed and workflows work as usual.
Approving.
Refactor extract File Metadata to its own model
What this PR does / why we need it:
This PR introduces a refactor to separate file metadata into its own model. This change is necessary to enable the reuse of file metadata in both the File model and the FilePreview model
Special notes for your reviewer:
I didn't add this PR to the sprint board because I think that this PR is part of the other PR #281. I've just split the PR to facilitate the code review, but once both PRs are reviewed we can merge this one into #281 to QA them together. Let me know if that makes sense
Suggestions on how to test this:
Run boh unit and e2e tests and check that everything works as usual
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
no
Is there a release notes update needed for this change?:
no