Skip to content
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

Merged
merged 4 commits into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
[v#.#.#] ([month] [YYYY])
- Attachments: Copy attachments when moving an evidence/note
- Liquid: Make project-level collections available for Liquid syntax
- Upgraded gems: nokogiri, rails, rexml
- Bugs fixes:
Expand Down
19 changes: 19 additions & 0 deletions app/controllers/concerns/attachments_copier.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
module AttachmentsCopier
def copy_attachments(record)
record.content.scan(Attachment::SCREENSHOT_REGEX).each do |screenshot_url|
aapomm marked this conversation as resolved.
Show resolved Hide resolved
_, _, _, _, project_id, node_id, filename, _ = screenshot_url
aapomm marked this conversation as resolved.
Show resolved Hide resolved

attachment = Attachment.find_by(filename: filename, node_id: record.node_id_was)
aapomm marked this conversation as resolved.
Show resolved Hide resolved

if attachment
new_attachment = attachment.copy_to(record.node)
new_url = screenshot_url[0].gsub(
/nodes\/[0-9]+\/attachments/,
"nodes/#{new_attachment.node_id}/attachments"
)

record.content = record.content.gsub(screenshot_url[0], new_url)
end
end
end
end
3 changes: 3 additions & 0 deletions app/controllers/evidence_controller.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
class EvidenceController < NestedNodeResourceController
include AttachmentsCopier
include ConflictResolver
include EvidenceHelper
include LiquidEnabledResource
Expand Down Expand Up @@ -55,6 +56,8 @@ def update
@evidence.assign_attributes(evidence_params)
autogenerate_issue if evidence_params[:issue_id].blank?

copy_attachments(@evidence) if @evidence.node_changed?

if @evidence.save
track_updated(@evidence)
check_for_edit_conflicts(@evidence, updated_at_before_save)
Expand Down
7 changes: 6 additions & 1 deletion app/controllers/notes_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# This controller exposes the REST operations required to manage the Note
# resource.
class NotesController < NestedNodeResourceController
include AttachmentsCopier
include ConflictResolver
include LiquidEnabledResource
include Mentioned
Expand Down Expand Up @@ -44,7 +45,11 @@ def edit
# Update the attributes of a Note
def update
updated_at_before_save = @note.updated_at.to_i
if @note.update(note_params)

@note.assign_attributes(note_params)
copy_attachments(@note) if @note.node_changed?

if @note.save
Comment on lines +49 to +52
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good consistency move.

track_updated(@note)
check_for_edit_conflicts(@note, updated_at_before_save)
# if the note has just been moved to another node, we must reload
Expand Down
18 changes: 9 additions & 9 deletions app/models/attachment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
**
=end


# ==Description
# This class in an abstraction layer to the <tt>attachments/</tt> folder. It allows
# access to the folder content in a way that mimics the working of ActiveRecord
Expand Down Expand Up @@ -66,6 +65,8 @@ class Attachment < File
AttachmentPwd = Rails.env.test? ? Rails.root.join('tmp', 'attachments') : Rails.root.join('attachments')
FileUtils.mkdir_p(AttachmentPwd) unless File.exists?(AttachmentPwd)

SCREENSHOT_REGEX = /\!((https?:\/\/.+)|((\/pro)?\/projects\/(\d+)\/nodes\/(\d+)\/attachments\/(.+?)(\(.*\))?))\!/i

# -- Class Methods ---------------------------------------------------------

def self.all(*args)
Expand All @@ -81,7 +82,7 @@ def self.count
end

def self.find_by(filename:, node_id:)
find(filename, conditions: { node_id: node_id } )
find(filename, conditions: { node_id: node_id })
rescue StandardError
end

Expand All @@ -91,17 +92,17 @@ def self.find(*args)
dir = Dir.new(pwd)

# makes the find request and stores it to resources
return_value = case args.first
case args.first
when :all, :first, :last
attachments = []
if options[:conditions] && options[:conditions][:node_id]
node_id = options[:conditions][:node_id].to_s
raise "Node with ID=#{node_id} does not exist" unless Node.exists?(node_id)
if (File.exist?( File.join(pwd, node_id)))
if (File.exist?(File.join(pwd, node_id)))
node_dir = Dir.new(pwd.join(node_id)).sort
node_dir.each do |attachment|
next unless (attachment =~ /^(.+)$/) == 0 && !File.directory?(pwd.join(node_id, attachment))
attachments << Attachment.new(:filename => $1, :node_id => node_id.to_i)
attachments << Attachment.new(filename: $1, node_id: node_id.to_i)
end
end
else
Expand All @@ -110,7 +111,7 @@ def self.find(*args)
node_dir = Dir.new(pwd.join(node)).sort
node_dir.each do |attachment|
next unless (attachment =~ /^(.+)$/) == 0 && !File.directory?(pwd.join(node, attachment))
attachments << Attachment.new(:filename => $1, :node_id => node.to_i)
attachments << Attachment.new(filename: $1, node_id: node.to_i)
end
end
attachments.sort_by!(&:filename)
Expand All @@ -129,18 +130,17 @@ def self.find(*args)
# in this routine we find the attachment by file name and node id
filename = args.first
attachments = []
raise "You need to supply a node id in the condition parameter" unless options[:conditions] && options[:conditions][:node_id]
raise 'You need to supply a node id in the condition parameter' unless options[:conditions] && options[:conditions][:node_id]
node_id = options[:conditions][:node_id].to_s
raise "Node with ID=#{node_id} does not exist" unless Node.exists?(node_id)
node_dir = Dir.new(pwd.join(node_id)).sort
node_dir.each do |attachment|
next unless ((attachment =~ /^(.+)$/) == 0 && $1 == filename)
attachments << Attachment.new(:filename => $1, :node_id => node_id.to_i)
attachments << Attachment.new(filename: $1, node_id: node_id.to_i)
end
raise "Could not find Attachment with filename #{filename}" if attachments.empty?
attachments.first
end
return return_value
end

def self.model_name
Expand Down
10 changes: 9 additions & 1 deletion spec/features/evidence_moving_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,13 @@ def create_node(label, parent = nil)
click_move_evidence
end

let(:current_evidence) { @evidence = create(:evidence, node: @node_5) }
let(:content) { "#[Description]#\nTest Evidence\n" }
let(:current_evidence) { @evidence = create(:evidence, content: content, node: @node_5) }

describe 'moving an evidence to a different node' do
let(:attachment) { create(:attachment, node: @node_5) }
let(:content) { "#[Description]#\n!/projects/#{current_project.id}/nodes/#{@node_5.id}/attachments/#{attachment.filename}!\n" }

before do
within('#modal_move_evidence') do
click_link @node_1.label
Expand All @@ -46,6 +50,10 @@ def create_node(label, parent = nil)
it 'should redirect to evidence show path' do
expect(current_path).to eq(project_node_evidence_path(current_project, @node_1, current_evidence))
end

it 'should update the attachment reference to the new node' do
expect(current_evidence.reload.content).to include("nodes/#{@node_1.id}")
end
end

describe 'moving a evidence to a similar node', js: true do
Expand Down
10 changes: 9 additions & 1 deletion spec/features/note_moving_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,13 @@ def create_node(label, parent = nil)
click_move_note
end

let(:current_note) { @note = create(:note, node: @node_5) }
let(:text) { "#[Description]#\nTest Note\n" }
let(:current_note) { @note = create(:note, text: text, node: @node_5) }

describe 'moving a note to a different node' do
let(:attachment) { create(:attachment, node: @node_5) }
let(:text) { "#[Description]#\n!/projects/#{current_project.id}/nodes/#{@node_5.id}/attachments/#{attachment.filename}!\n" }

before do
within('#modal_move_note') do
click_link @node_1.label
Expand All @@ -47,6 +51,10 @@ def create_node(label, parent = nil)
it 'should redirect to note show path' do
expect(current_path).to eq(project_node_note_path(current_project, @node_1, current_note))
end

it 'should update the attachment reference to the new node' do
expect(current_note.reload.content).to include("nodes/#{@node_1.id}")
end
end

describe 'moving a note to a similar node' do
Expand Down
Loading