-
Notifications
You must be signed in to change notification settings - Fork 1
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
Second Attachments PR #716
Open
miggol
wants to merge
50
commits into
major/4
Choose a base branch
from
feature/attachments-4
base: major/4
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I've just gone ahead and merged major/4 into here, to fix some bugs and get access to the new study end questions etc. |
…ent_dowload_permissions Feature/attachment download permissions
* feat: Shortcut to check if a slot is required * feat: Distinct AttachmentItem for getting attachment errors * feat: Make the build_css_classes generic * fix: Contingency for when an item doesn't have a form_class * style: Black and djlint
* fix: Remove old documents check while developing * feat: Copy attachments to new proposal * feat: Revise attachments owned by multiple objects * fix: Bug that prevented creation of fresh attachments * style: black and djlint * feat: Forgotten detach_form.html template
* feat: Link to add extra attachment * feat: KIND_CHOICES constant for... Kind choices * feat: AttachFormView can now handle extra attachments * feat: Match extra attachments to additional slots * feat: Remove extra arg, kinds are now selectable when editing * feat: Avoid using kinds to determine attached model * fix: Missing detach form * style: Black and djlint * docs: Add comment to attachment_slots * fix: Wrong object PK * fix: Improve description of action in attach form * feat: Much nicer styling on the attachments page * fix: Untranslated string
* fix: Remove old documents check while developing * feat: Copy attachments to new proposal * feat: Revise attachments owned by multiple objects * fix: Bug that prevented creation of fresh attachments * style: black and djlint * feat: New and old documents together * wip: Work on better documents_list * feat: Renderable now takes a template_name argument * fix: Turn form buttons on attachments page into normal buttons * feat: get_download_url method for Attachment * feat: Use get_download_url method in single attachment template * wip: documents_list for new attachments system * feat: Forgotten detach_form.html template * feat: Break out proposal urls into multiple files * feat: AttachmentsList renderable and mixin * feat: CompareAttachments view * debug: Add mime type to error message * fix: Remove documents checker * feat: Move AttachmentList helpers into attachment utils * style: Black and djlint * feat: Friendly sunny icon when an attachment is new * fix: Attachments in a new proposal are never new * fix: Correction, attachments in a new proposal are *always* new * feat: Break out document-getting from documents_list * feat: Append legacy documents to Attachments page * style: Black and djlint * feat: Warning on detach_form * feat: minor string changes and translations * Feature/detailed review attachments view (#752) * feat: Move utility functions from Item into Slot * feat: New detailed attachments list view for reviewers * feat: Link for getting FETC filename of attachments * fix: Bring RDS up to speed with the changes * i18n: Translations * feat: Remove deprecated provision method * feat: Remove deprecated dmp_edit_link remnants * stucture: Move Attachmentslist &co to a more logical place * feat: expansion of the ReviewSidebarMixin empire * fix: Remove evil twin * fix: Colectomy * fix: Remove deprecated templatetag * docs: Some comments * feat: Whoops, forgot a template * feat: Hide stepper when secretary edits documents * fix: Unused or missing imports * feat: Hide legacy documents if none are present * fix: Missing migrations * feat: Buttons to return to review detail page * style: Black and djlint * fix: Also hide form buttons on proposal attachments if secretary * fix: get_legacy_documents now returns an empty list when no files are found * fix: Conditionality for form buttons in attachments.html * fix: Doclist now always returns a proposal container to hold the PDF * feat: Attachment detailed list now works for supervisor reviews * i18n: Minor translations * merge: attachments-4 * style: black and djlint
* feat: create all InformationLetter kinds * feat: add correct informationletter in checker * feat: check if Study has AV registration * fix: bug, where legal basis can be 0 ... woopsie * feature: add all possible kinds * feature: add all slots, through checkers * feat: implement navigation buttons for ProposalsAttachmentsView * feature: a lotta translations * style: black sabbath * fix: Make get_kind_from_str resilient to unknown kinds * feat: Rename OtherProposalAttachment * feat: Optionality groups for attachment slots * feat: Render help texts next to slots * style: Black and djlint * fix: logic of slots * feature: implement match + match_and_set * feat: has_adults() for Study * feat: base ConsentForm on whether Study.has_adults() * style: black betty * fix: Give match_and_set() a return value * fix: potential TypeError: addition of list and set * merge: merge translations and do a style pass --------- Co-authored-by: Michael Villeneuve <m.l.villeneuve@uu.nl>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Draft because this is still incomplete, but better than nothing.
Quick rundown
The
Attachment
model holds the attachment file, but is in itself not attached to anything. Actual usable attachments are subclasses of this model with anattached_to
field to point to their owner. This allows for Attachments to many different kinds of other models, should we want to attach files to Review or Wmo objects in the future. The Django docs on subclassing models are recommended reading for a good understanding of this.Attachment slots are what they sound like. They are created by checkers, are held by the Stepper object, and render as a place to put an attachment.
Attachment views
The Attach view is used to attach a new file to an object. There are different URLs for Proposal and Study attachments, but these simply point to the same view with a small modifier. If a kind is passed in the URL, this will be enforced in the form. This is the case for filling "required" slots. If no kind is passed, this is probably an extra file and the user is free to choose a kind.
The detach view is like a Delete view, except that it calls
Attachment.detach()
, which only deletes the attachment if it is no longer attached to any other models. Otherwise it simply removes the given ManyToMany relation.The update view isn't too special at this point. If an Attachment is only attached to one object, it should just update the attachment as it does now. However, if the attachment is attached to multiple objects, it should create a copy and set its
parent
relation to the old object. This is not implemented yet, but is necessary to not mess with historical objects. I'm 90% sure about how this will work from previous revisions of this branch but bear with me while I test a little more.Attachment kinds
In previous implementations these were far more elaborate, but I've moved away from putting logic in them. We still could, but right now they're just places to put strings, defaults, and help texts relating to different kinds of attachments. Which is fine.
What I still need to do
The main yet-to-be completed feature is Attachment revisions in AttachmentUpdateView as described above. This needs implementation and testing with regards to proposal copies and revisions.
What still needs doing but not necessarily by me
documents_list
for the review_detail_sidebarstepper.attachment_slots
and check for any slots which are required but have no attachment.In conclusion
It's hard to believe that I've ended up with the core of this feature being less than 500 lines or so. But it took a lot of rewriting overly complex ideas I had to get to this point. I probably should have published something earlier, I dithered a lot on this so sorry about that.
If something looks weird please keep in mind this is a little rushed and there's probably vestigial code lying around from previous revisions. I would not recommend an in-depth review at this point, but this branch is definitely ready to receive further feature PRs into it.