-
Notifications
You must be signed in to change notification settings - Fork 189
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
Add AttachmentsCopier concern #1271
Conversation
@note.assign_attributes(note_params) | ||
copy_attachments(@note) if @note.node_changed? | ||
|
||
if @note.save |
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.
good consistency move.
new_path = full_screenshot_path.gsub( | ||
/nodes\/[0-9]+\/attachments\/.+/, | ||
"nodes/#{new_attachment.node_id}/attachments/#{new_filename}" | ||
) |
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.
I'm concerned about this text manipulation. Then I thought about the "copy link" feature of the attachments sidebar, perhaps we can use the same approach?
https://github.com/dradis/dradis-ce/blob/main/app/views/attachments/_attachment_box.html.erb#L59
I could even consider moving this into a helper to avoid duplication, but we don't currently have an attachment_helper.rb it could fit in.
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.
It's tricky to DRY this considering that the contexts are different. The attachment box is in the view that has access to the route helpers while this code isn't. There's also some possible rabbit hole with the default_url_options = /pro
once we move the link helpers out of the view. Happy to explore this more if we need this code on many more places but currently, this seems like the simplest approach.
Spec
When moving an instance of evidence to another node the attachments are not moved. This means that if we delete the original node, the attachments are destroyed and the attachment can no longer be referenced in the evidence.
Proposed solution
Copy the attachment to the new node when an evidence or note are moved
Other Information
If there's anything else that's important and relevant to your pull
request, mention that information here. This could include
benchmarks, or other information.
Thanks for contributing to Dradis!
Copyright assignment
Collaboration is difficult with commercial closed source but we want
to keep as much of the OSS ethos as possible available to users
who want to fix it themselves.
In order to unambiguously own and sell Dradis Framework commercial
products, we must have the copyright associated with the entire
codebase. Any code you create which is merged must be owned by us.
That's not us trying to be a jerks, that's just the way it works.
Please review the CONTRIBUTING.md
file for the details.
You can delete this section, but the following sentence needs to
remain in the PR's description:
Check List