diff --git a/CHANGELOG b/CHANGELOG index c2724509c..c46a3bfdf 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,8 +1,10 @@ [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: - Issues: Update Affected column after a node has been renamed or merged + - Navigation: Restore functionality of native browser back/forward buttons - Bug tracker items: - [item] - New integrations: diff --git a/app/assets/javascripts/shared/behaviors.js b/app/assets/javascripts/shared/behaviors.js index cd6ac4f75..9aa1e52a0 100644 --- a/app/assets/javascripts/shared/behaviors.js +++ b/app/assets/javascripts/shared/behaviors.js @@ -81,11 +81,18 @@ } // Update address bar with current tab param - $('[data-bs-toggle~=tab]').on('shown.bs.tab', function (e) { - let currentTab = $(e.target).attr('href').substring(1); - searchParams.set('tab', currentTab); - history.pushState(null, null, `?${searchParams.toString()}`); - }); + $(parentElement) + .find('[data-bs-toggle~=tab]') + .on('shown.bs.tab', function (e) { + let currentTab = $(e.target).attr('href').substring(1); + searchParams.set('tab', currentTab); + let urlWithTab = `?${searchParams.toString()}`; + history.pushState( + { turbolinks: true, url: urlWithTab }, + '', + urlWithTab + ); + }); } document.addEventListener('turbolinks:load', function () { diff --git a/app/controllers/concerns/attachments_copier.rb b/app/controllers/concerns/attachments_copier.rb new file mode 100644 index 000000000..3ad9f5db7 --- /dev/null +++ b/app/controllers/concerns/attachments_copier.rb @@ -0,0 +1,20 @@ +module AttachmentsCopier + def copy_attachments(record) + 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: CGI::unescape(filename), node_id: record.node_id_was) + + if attachment + new_attachment = attachment.copy_to(record.node) + 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}" + ) + + record.content = record.content.gsub(full_screenshot_path, new_path) + end + end + end +end diff --git a/app/controllers/concerns/liquid_enabled_resource.rb b/app/controllers/concerns/liquid_enabled_resource.rb index 1dac5829e..aa0661e31 100644 --- a/app/controllers/concerns/liquid_enabled_resource.rb +++ b/app/controllers/concerns/liquid_enabled_resource.rb @@ -35,6 +35,6 @@ def project_assigns project = Project.find(params[:project_id]) authorize! :use, project - LiquidAssignsService.new(project: project, text: params[:text]).assigns + LiquidCachedAssigns.new(project: project) 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..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 @@ -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) @@ -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 @@ -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 @@ -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) @@ -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 diff --git a/app/services/liquid_assigns_service.rb b/app/services/liquid_assigns_service.rb deleted file mode 100644 index b781c77b6..000000000 --- a/app/services/liquid_assigns_service.rb +++ /dev/null @@ -1,61 +0,0 @@ -class LiquidAssignsService - AVAILABLE_PROJECT_ASSIGNS = %w{ evidences issues nodes notes tags }.freeze - - attr_accessor :project, :text - - def initialize(project:, text: nil) - @project = project - @text = text - end - - def assigns - result = project_assigns - result.merge!(assigns_pro) if defined?(Dradis::Pro) - result - end - - private - - def assigns_pro - end - - # This method uses Liquid::VariableLookup to find all liquid variables from - # a given text. We use the list to know which project assign we need. - def assigns_from_content - return AVAILABLE_PROJECT_ASSIGNS if text.nil? - - variable_lookup = Liquid::VariableLookup.parse(text) - return (variable_lookup.lookups & AVAILABLE_PROJECT_ASSIGNS) - end - - def cached_drops(records, record_type) - return [] if records.empty? - - cache_key = "liquid-project-#{project.id}-#{record_type.pluralize}:#{records.maximum(:updated_at).to_i}-#{records.count}" - drop_class = "#{record_type.camelize}Drop".constantize - - Rails.cache.fetch(cache_key) do - records.map { |record| drop_class.new(record) } - end - end - - def project_assigns - project_assigns = { 'project' => ProjectDrop.new(project) } - - assigns_from_content.each do |record_type| - records = - case record_type - when 'evidences' - project.evidence - when 'nodes' - project.nodes.user_nodes - else - project.send(record_type.to_sym) - end - - project_assigns.merge!(record_type => cached_drops(records, record_type.singularize)) - end - - project_assigns - end -end diff --git a/app/services/liquid_cached_assigns.rb b/app/services/liquid_cached_assigns.rb new file mode 100644 index 000000000..5f1e82562 --- /dev/null +++ b/app/services/liquid_cached_assigns.rb @@ -0,0 +1,70 @@ +class LiquidCachedAssigns < Hash + AVAILABLE_PROJECT_ASSIGNS = %w{ evidences issues nodes notes project tags }.freeze + + attr_accessor :assigns, :project + + def initialize(project:) + @project = project + + @assigns = { 'project' => ProjectDrop.new(project) } + @assigns.merge!(assigns_pro) + end + + def [](record_type) + assigns[record_type] ||= cached_drops(record_type) + end + + # SEE: https://github.com/Shopify/liquid/blob/77bc56/lib/liquid/context.rb#L211 + # Liquid is checking if the variable is present in the assigns hash by + # calling the `key?` method. Since we're lazily loading the keys, the variable + # may not yet be present in the assigns hash. + def key?(key) + AVAILABLE_PROJECT_ASSIGNS.include?(key.to_s) || assigns.key?(key) + end + + def merge(hash) + lca = LiquidCachedAssigns.new(project: project) + lca.assigns = @assigns.merge(hash) + lca + end + + def merge!(hash) + @assigns.merge!(hash) + self + end + + private + + def assigns_pro + {} + end + + def cached_drops(record_type) + records = project_records(record_type) + + return [] if records.empty? + + cache_key = ActiveSupport::Cache.expand_cache_key([project.id, records], 'liquid') + drop_class = "#{record_type.singularize.camelize}Drop".constantize + + Rails.cache.fetch(cache_key) do + records.map { |record| drop_class.new(record) } + end + end + + def project_records(record_type) + return [] unless AVAILABLE_PROJECT_ASSIGNS.include?(record_type) + + case record_type + when 'evidences' + project.evidence + when 'nodes' + project.nodes.user_nodes + when 'notes' + # FIXME - ISSUE/NOTE INHERITANCE + project.notes.where.not(node_id: project.issue_library.id) + else + project.send(record_type.to_sym) + end + end +end diff --git a/spec/features/evidence_moving_spec.rb b/spec/features/evidence_moving_spec.rb index d0a211996..ea01c8f0b 100644 --- a/spec/features/evidence_moving_spec.rb +++ b/spec/features/evidence_moving_spec.rb @@ -29,10 +29,17 @@ 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, 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 @@ -46,6 +53,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..836402a62 100644 --- a/spec/features/note_moving_spec.rb +++ b/spec/features/note_moving_spec.rb @@ -30,10 +30,17 @@ 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, 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 @@ -47,6 +54,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 diff --git a/spec/services/liquid_assigns_service_spec.rb b/spec/services/liquid_assigns_service_spec.rb deleted file mode 100644 index 5e1fd0796..000000000 --- a/spec/services/liquid_assigns_service_spec.rb +++ /dev/null @@ -1,59 +0,0 @@ -require 'rails_helper' - -RSpec.describe LiquidAssignsService do - let!(:project) { create(:project) } - - before do - node = create(:node, project: project) - issue = create(:issue, node: project.issue_library) - create(:evidence, issue: issue, node: node) - create(:note, node: node) - create(:tag) - end - - describe '#project_assigns' do - context 'with the :text argument' do - LiquidAssignsService::AVAILABLE_PROJECT_ASSIGNS.each do |assign| - it "adds #{assign} to the project_assigns if present in the text" do - text = "#[Description]#\n {% for #{assign.singularize} in #{assign} %}{% endfor %}\n" - liquid_assigns = described_class.new(project: project, text: text).assigns - - expect(liquid_assigns.keys).to include(assign) - end - end - end - - context 'without the :text argument' do - let(:liquid_assigns) { described_class.new(project: project).assigns } - - it 'builds a hash of liquid assigns' do - expect(liquid_assigns['project'].name).to eq(project.name) - expect(liquid_assigns['issues'].map(&:title)).to eq(project.issues.map(&:title)) - expect(liquid_assigns['evidences'].map(&:title)).to eq(project.evidence.map(&:title)) - expect(liquid_assigns['nodes'].map(&:label)).to eq(project.nodes.user_nodes.map(&:label)) - expect(liquid_assigns['notes'].map(&:title)).to eq(project.notes.map(&:title)) - expect(liquid_assigns['tags'].map(&:display_name)).to eq(project.tags.map(&:display_name)) - end - end - end - - context 'with pro records', skip: !defined?(Dradis::Pro) do - let(:liquid_assigns) { described_class.new(project: project).assigns } - - let!(:project) { create(:project, :with_team) } - - before do - report_content = project.content_library - report_content.properties = { 'dradis.project' => project.name } - report_content.save - - create(:content_block, project: project) - end - - it 'builds a hash with Dradis::Pro assigns' do - expect(liquid_assigns['document_properties'].available_properties).to eq({ 'dradis.project' => project.name }) - expect(liquid_assigns['team'].name).to eq(project.team.name) - expect(liquid_assigns['content_blocks'].map(&:content)).to eq(project.content_blocks.map(&:content)) - end - end -end diff --git a/spec/services/liquid_cached_assigns_spec.rb b/spec/services/liquid_cached_assigns_spec.rb new file mode 100644 index 000000000..34889a466 --- /dev/null +++ b/spec/services/liquid_cached_assigns_spec.rb @@ -0,0 +1,63 @@ +require 'rails_helper' + +RSpec.describe LiquidCachedAssigns do + let!(:project) { create(:project) } + let(:liquid_assigns) { described_class.new(project: project) } + + before do + node = create(:node, project: project) + issue = create(:issue, node: project.issue_library) + create(:evidence, issue: issue, node: node) + create(:note, node: node) + create(:tag) + end + + context 'fetching an assign from an available collection' do + it 'lazily loads the assigns' do + expect(liquid_assigns.assigns.keys).to_not include( + %w{issues evidences nodes notes tags} + ) + end + + it 'builds a hash of liquid assigns' do + issues = project.issues.map(&:title) + + expect(liquid_assigns['project'].name).to eq(project.name) + expect(liquid_assigns['issues'].map(&:title)).to eq(issues) + expect(liquid_assigns['evidences'].map(&:title)).to eq(project.evidence.map(&:title)) + expect(liquid_assigns['nodes'].map(&:label)).to eq(project.nodes.user_nodes.map(&:label)) + expect(liquid_assigns['notes'].map(&:title)).to eq(project.notes.map(&:title) - issues) + expect(liquid_assigns['tags'].map(&:display_name)).to eq(project.tags.map(&:display_name)) + end + end + + context 'fetching an assign from a unavailable collection' do + it 'returns an empty array' do + expect(liquid_assigns['fake']).to be_empty + end + end + + context 'with pro records', skip: !defined?(Dradis::Pro) do + let!(:project) { create(:project, :with_team) } + + before do + report_content = project.content_library + report_content.properties = { 'dradis.project' => project.name } + report_content.save + + create(:content_block, project: project) + end + + context 'fetching an assign from an available collection' do + it 'lazily loads the assigns' do + expect(liquid_assigns.assigns.keys).to_not include('content_blocks') + end + + it 'builds a hash with Dradis::Pro assigns' do + expect(liquid_assigns['document_properties'].available_properties).to eq({ 'dradis.project' => project.name }) + expect(liquid_assigns['team'].name).to eq(project.team.name) + expect(liquid_assigns['content_blocks'].map(&:content)).to eq(project.content_blocks.map(&:content)) + end + end + end +end