From f352a1c4ad3f944ba46db6c5c07ad749bae2e8d2 Mon Sep 17 00:00:00 2001 From: Aaron Manaloto Date: Tue, 18 Jun 2024 17:47:04 +0800 Subject: [PATCH 1/4] Add AttachmentsCopier concern --- CHANGELOG | 1 + .../concerns/attachments_copier.rb | 19 +++++++++++++++++++ app/controllers/evidence_controller.rb | 3 +++ app/controllers/notes_controller.rb | 7 ++++++- app/models/attachment.rb | 2 ++ spec/features/evidence_moving_spec.rb | 10 +++++++++- spec/features/note_moving_spec.rb | 10 +++++++++- 7 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 app/controllers/concerns/attachments_copier.rb diff --git a/CHANGELOG b/CHANGELOG index 4968516d7..55c5c3f83 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -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: diff --git a/app/controllers/concerns/attachments_copier.rb b/app/controllers/concerns/attachments_copier.rb new file mode 100644 index 000000000..ff093a3f8 --- /dev/null +++ b/app/controllers/concerns/attachments_copier.rb @@ -0,0 +1,19 @@ +module AttachmentsCopier + def copy_attachments(record) + record.content.scan(Attachment::SCREENSHOT_REGEX).each do |screenshot_url| + _, _, _, _, project_id, node_id, filename, _ = screenshot_url + + attachment = Attachment.find_by(filename: filename, node_id: record.node_id_was) + + 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 diff --git a/app/controllers/evidence_controller.rb b/app/controllers/evidence_controller.rb index e66247f42..1d78da371 100644 --- a/app/controllers/evidence_controller.rb +++ b/app/controllers/evidence_controller.rb @@ -1,4 +1,5 @@ class EvidenceController < NestedNodeResourceController + include AttachmentsCopier include ConflictResolver include EvidenceHelper include LiquidEnabledResource @@ -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) diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index 895e094f0..71715460d 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -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 @@ -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 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 diff --git a/app/models/attachment.rb b/app/models/attachment.rb index f80c888bd..25e78e4dd 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -66,6 +66,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) diff --git a/spec/features/evidence_moving_spec.rb b/spec/features/evidence_moving_spec.rb index d0a211996..76a92f726 100644 --- a/spec/features/evidence_moving_spec.rb +++ b/spec/features/evidence_moving_spec.rb @@ -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 @@ -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 diff --git a/spec/features/note_moving_spec.rb b/spec/features/note_moving_spec.rb index 8b4890c67..53b5c08c6 100644 --- a/spec/features/note_moving_spec.rb +++ b/spec/features/note_moving_spec.rb @@ -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 @@ -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 From 8645017b96fa5c06abebfd71495ed1dc706f4d27 Mon Sep 17 00:00:00 2001 From: Aaron Manaloto Date: Tue, 18 Jun 2024 17:55:30 +0800 Subject: [PATCH 2/4] Appease rubocop --- app/models/attachment.rb | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/app/models/attachment.rb b/app/models/attachment.rb index 25e78e4dd..455afe832 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -14,7 +14,6 @@ ** =end - # ==Description # This class in an abstraction layer to the attachments/ folder. It allows # access to the folder content in a way that mimics the working of ActiveRecord @@ -83,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 @@ -93,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 @@ -112,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) @@ -131,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 From f44129cd44180fc05c9848ea34666275fdecefd6 Mon Sep 17 00:00:00 2001 From: Aaron Manaloto Date: Mon, 24 Jun 2024 17:36:28 +0800 Subject: [PATCH 3/4] Handle attachments with spaces and duplicated attachments --- app/controllers/concerns/attachments_copier.rb | 15 ++++++++------- spec/features/evidence_moving_spec.rb | 5 ++++- spec/features/note_moving_spec.rb | 5 ++++- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/app/controllers/concerns/attachments_copier.rb b/app/controllers/concerns/attachments_copier.rb index ff093a3f8..7db9ec05e 100644 --- a/app/controllers/concerns/attachments_copier.rb +++ b/app/controllers/concerns/attachments_copier.rb @@ -1,18 +1,19 @@ module AttachmentsCopier def copy_attachments(record) - record.content.scan(Attachment::SCREENSHOT_REGEX).each do |screenshot_url| - _, _, _, _, project_id, node_id, filename, _ = screenshot_url + record.content.scan(Attachment::SCREENSHOT_REGEX).each do |screenshot_path| + full_screenshot_path, _, _, _, project_id, node_id, filename, _ = screenshot_path - attachment = Attachment.find_by(filename: filename, node_id: record.node_id_was) + attachment = Attachment.find_by(filename: CGI::unescape(filename), node_id: record.node_id_was) 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" + new_filename = ERB::Util.url_encode(new_attachment.filename) + new_path = full_screenshot_path.gsub( + /nodes\/[0-9]+\/attachments\/.+/, + "nodes/#{new_attachment.node_id}/attachments/#{new_filename}" ) - record.content = record.content.gsub(screenshot_url[0], new_url) + record.content = record.content.gsub(full_screenshot_path, new_path) end end end diff --git a/spec/features/evidence_moving_spec.rb b/spec/features/evidence_moving_spec.rb index 76a92f726..ea01c8f0b 100644 --- a/spec/features/evidence_moving_spec.rb +++ b/spec/features/evidence_moving_spec.rb @@ -33,10 +33,13 @@ def create_node(label, parent = nil) 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(:attachment) { create(:attachment, filename: 'name with spaces.png', node: @node_5) } let(:content) { "#[Description]#\n!/projects/#{current_project.id}/nodes/#{@node_5.id}/attachments/#{attachment.filename}!\n" } before do + # Ensure this works with duplicated attachment + create(:attachment, filename: 'name with spaces.png', node: @node_1) + within('#modal_move_evidence') do click_link @node_1.label click_submit diff --git a/spec/features/note_moving_spec.rb b/spec/features/note_moving_spec.rb index 53b5c08c6..836402a62 100644 --- a/spec/features/note_moving_spec.rb +++ b/spec/features/note_moving_spec.rb @@ -34,10 +34,13 @@ def create_node(label, parent = nil) 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(:attachment) { create(:attachment, filename: 'name with spaces.png', node: @node_5) } let(:text) { "#[Description]#\n!/projects/#{current_project.id}/nodes/#{@node_5.id}/attachments/#{attachment.filename}!\n" } before do + # Ensure this works with duplicated attachment + create(:attachment, filename: 'name with spaces.png', node: @node_1) + within('#modal_move_note') do click_link @node_1.label click_submit From 33256b590c1bfb79dc103401cb9deb4ccabcc318 Mon Sep 17 00:00:00 2001 From: Aaron Manaloto Date: Tue, 25 Jun 2024 16:26:05 +0800 Subject: [PATCH 4/4] Use url_encoded_filename method --- app/controllers/concerns/attachments_copier.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/concerns/attachments_copier.rb b/app/controllers/concerns/attachments_copier.rb index 7db9ec05e..3ad9f5db7 100644 --- a/app/controllers/concerns/attachments_copier.rb +++ b/app/controllers/concerns/attachments_copier.rb @@ -7,7 +7,7 @@ def copy_attachments(record) if attachment new_attachment = attachment.copy_to(record.node) - new_filename = ERB::Util.url_encode(new_attachment.filename) + new_filename = new_attachment.url_encoded_filename new_path = full_screenshot_path.gsub( /nodes\/[0-9]+\/attachments\/.+/, "nodes/#{new_attachment.node_id}/attachments/#{new_filename}"