New implementation for attachments creation

app/assets/javascripts/sitewide/external_protocols.js

@@ -18,8 +18,8 @@ var ExternalProtocols = (function() {
       var msg;
       msg = key.charAt(0).toUpperCase() + key.slice(1) + ': ' + errors.protocol[key].join(', ');
       if ((input.length > 0) && (errors.protocol[key].length > 0)) {
-        input.next('span.help-block').html(msg);
+        input.parent().next('span.help-block').html(msg);
-        input.parent().addClass('has-error');
+        input.parent().parent().addClass('has-error');
       } else if (errors.protocol[key].length > 0) {
         modal.find('.general-error > span').append(msg + '<br/>');
       }
@@ -72,7 +72,7 @@ var ExternalProtocols = (function() {
   }
 
   function initLoadProtocolModalPreview() {
-    var externalProtocols = $('.exteral-proocol-result');
+    var externalProtocols = $('.external-protocol-result');
     externalProtocols.on('click', 'a[data-action="external-import"]', function(e) {
       var link = $(this);
       animateSpinner(null, true);
@@ -120,7 +120,6 @@ var ExternalProtocols = (function() {
   function initFormSubmits() {
     var modal = $('#protocol-preview-modal');
     modal.on('click', 'button[data-action=import_protocol]', function() {
-      // console.log('click');
       var form = modal.find('form');
       var hiddenField = form.find('#protocol_protocol_type');
       hiddenField.attr('value', $(this).data('import_type'));

app/assets/stylesheets/protocols/preview_modal.scss

@@ -15,7 +15,7 @@
     }
   }
 
-  .general-eror {
+  .general-error {
     text-align: center;
   }
 }

app/controllers/external_protocols_controller.rb

@@ -47,23 +47,26 @@ class ExternalProtocolsController < ApplicationController
       protocol_source: new_params[:protocol_source],
       protocol_client_id: new_params[:protocol_client_id],
       user_id: current_user.id,
-      team_id: @team.id
+      team_id: @team.id,
+      build_with_assets: false
     )
-    p = service_call.built_protocol
-    p.valid? # Get validations errors here
 
-    @protocol = p
     if service_call.succeed?
+      @protocol = service_call.built_protocol
+      @protocol&.valid? # Get validations errors here
+
       render json: {
         html: render_to_string(
           partial: 'protocol_importers/import_form.html.erb',
-          locals: { protocol: p, steps_json: service_call.serialized_steps }
+          locals: { protocol: @protocol,
+                    steps_json: service_call.serialized_steps,
+                    steps_assets: service_call.steps_assets }
         ),
-        title: "Import protocol - #{p.name}",
+        title: t('protocol_importers.new.modal_title', protocol_name: @protocol.name),
         footer: render_to_string(
           partial: 'protocol_importers/preview_modal_footer.html.erb'
         ),
-        validation_errors: { protocol: p.errors.messages }
+        validation_errors: { protocol: @protocol.errors.messages }
       }
     else
       render json: { errors: service_call.errors }, status: 400
@@ -74,7 +77,7 @@ class ExternalProtocolsController < ApplicationController
   def create
     service_call = ProtocolImporters::ImportProtocolService.call(
       protocol_params: create_protocol_params,
-      steps_params: JSON.parse(create_steps_params[:steps]),
+      steps_params_json: create_steps_params[:steps],
       user_id: current_user.id,
       team_id: @team.id
     )

app/services/protocol_importers/build_protocol_from_client_service.rb

@@ -5,13 +5,15 @@ module ProtocolImporters
     extend Service
     require 'protocol_importers/protocols_io/v3/errors'
 
-    attr_reader :errors, :built_protocol
+    attr_reader :errors, :built_protocol, :steps_assets
 
-    def initialize(protocol_client_id:, protocol_source:, user_id:, team_id:)
+    def initialize(protocol_client_id:, protocol_source:, user_id:, team_id:, build_with_assets: true)
       @id = protocol_client_id
       @protocol_source = protocol_source
-      @user = User.find user_id
+      @user = User.find_by_id user_id
-      @team = Team.find team_id
+      @team = Team.find_by_id team_id
+      @build_with_assets = build_with_assets
+      @steps_assets = {}
       @errors = {}
     end
 
@@ -26,11 +28,12 @@ module ProtocolImporters
 
       pio = ProtocolImporters::ProtocolIntermediateObject.new(normalized_json: normalized_hash,
                                                               user: @user,
-                                                              team: @team)
+                                                              team: @team,
+                                                              build_with_assets: @build_with_assets)
 
       @built_protocol = pio.build
-      # Protocol with AR errors should not handled here as someting whats wrong.
+      @steps_assets = pio.steps_assets unless @build_with_assets
-      # @errors[:protocol] = pio.protocol.errors unless @built_protocol.valid?
+
       self
     rescue api_errors => e
       @errors[e.error_type] = e.message
@@ -48,14 +51,18 @@ module ProtocolImporters
     end
 
     def serialized_steps
+      # Serialize steps with nested attributes for Tables and NOT nasted attributes for Assets
+      # We want to avoid creating (downloading) Assets instances on building first time and again on importing/creating,
+      # when both actions are not in a row.
+      # Also serialization does not work properly with paperclip attrs
       return nil unless built_protocol
 
       built_protocol.steps.map do |step|
         step_hash = step.attributes.symbolize_keys.slice(:name, :description, :position)
 
-        # if step.assets.any?
+        if !@build_with_assets && @steps_assets[step.position].any?
-        #   step_hash[:assets_attributes] = step.assets.map { |a| a.attributes.symbolize_keys.except(:id) }
+          step_hash[:attachments] = @steps_assets[step.position]
-        # end
+        end
 
         if step.tables.any?
           step_hash[:tables_attributes] = step.tables.map { |t| t.attributes.symbolize_keys.slice(:contents) }

app/services/protocol_importers/import_protocol_service.rb

@@ -6,11 +6,11 @@ module ProtocolImporters
 
     attr_reader :errors, :protocol
 
-    def initialize(protocol_params:, steps_params:, team_id:, user_id:)
+    def initialize(protocol_params:, steps_params_json:, team_id:, user_id:)
-      @user = User.find user_id
+      @user = User.find_by_id user_id
-      @team = Team.find team_id
+      @team = Team.find_by_id team_id
       @protocol_params = protocol_params
-      @steps_params = steps_params
+      @steps_params = JSON.parse(steps_params_json) # catch error here
       @errors = {}
     end
 
@@ -18,10 +18,17 @@ module ProtocolImporters
       return self unless valid?
 
       @protocol = Protocol.new(@protocol_params.merge!(added_by: @user, team: @team))
-      @protocol.steps << @steps_params.collect do |step_params|
-        Step.new(step_params.merge(user: @user, completed: false))
 
-        # TODO: Manually create file here. "Accept nasted attributes won't work here"
+      @protocol.steps << @steps_params.map do |step_params|
+        # Create step with nested attributes for tables
+        s = Step.new(step_params
+                       .symbolize_keys
+                       .slice(:name, :position, :description, :tables_attributes)
+                       .merge(user: @user, completed: false))
+
+        # 'Manually' create assets here. "Accept nasted attributes won't work assets"
+        s.assets << AttachmentsBuilder.generate(step_params.deep_symbolize_keys, user: @user, team: @team)
+        s
       end
 
       @errors[:protocol] = @protocol.errors.messages unless @protocol.save

app/utilities/protocol_importers/attachments_builder.rb

@@ -2,16 +2,27 @@
 
 module ProtocolImporters
   module AttachmentsBuilder
-    def self.generate(step_json)
+    def self.generate(step_json, user: nil, team: nil)
       return [] unless step_json[:attachments]&.any?
 
       step_json[:attachments].map do |f|
         Asset.new(file:  URI.parse(f[:url]),
-                  created_by: @user,
+                  created_by: user,
-                  last_modified_by: @user,
+                  last_modified_by: user,
-                  team: @team,
+                  team: team,
                   file_file_name: f[:name])
       end
     end
+
+    def self.generate_json(step_json)
+      return [] unless step_json[:attachments]&.any?
+
+      step_json[:attachments].map do |f|
+        {
+          name: f[:name],
+          url: f[:url]
+        }
+      end
+    end
   end
 end

app/utilities/protocol_importers/protocol_intermediate_object.rb

@@ -2,12 +2,14 @@
 
 module ProtocolImporters
   class ProtocolIntermediateObject
-    attr_accessor :normalized_protocol_data, :user, :team, :protocol
+    attr_reader :normalized_protocol_data, :user, :team, :protocol, :steps_assets, :build_with_assets
 
-    def initialize(normalized_json: {}, user:, team:)
+    def initialize(normalized_json: {}, user:, team:, build_with_assets: true)
       @normalized_protocol_data = normalized_json.with_indifferent_access[:protocol] if normalized_json
       @user = user
       @team = team
+      @steps_assets = {}
+      @build_with_assets = build_with_assets
     end
 
     def import
@@ -28,7 +30,11 @@ module ProtocolImporters
     def build_steps
       @normalized_protocol_data[:steps].map do |s|
         step = Step.new(step_attributes(s))
-        step.assets << AttachmentsBuilder.generate(s)
+        if @build_with_assets
+          step.assets << AttachmentsBuilder.generate(s, user: user, team: team)
+        else
+          @steps_assets[step.position] = AttachmentsBuilder.generate_json(s)
+        end
         step.tables << TablesBuilder.extract_tables_from_html_string(s[:description][:body], true)
         step.description = StepDescriptionBuilder.generate(s)
         step

app/views/protocol_importers/_import_form.html.erb

@@ -43,7 +43,7 @@
   </div>
   <div id="steps">
     <% protocol.steps.sort_by{ |s| s.position }.each do |step| %>
-      <%= render partial: "steps/step.html.erb", locals: { step: step, preview: true } %>
+      <%= render partial: "steps/step.html.erb", locals: { step: step, steps_assets: steps_assets, preview: true, import: true } %>
     <% end %>
   </div>
 </div>

app/views/protocols/index/_external_protocols_tab.html.erb

@@ -51,6 +51,9 @@
       <div class='row empty-text'>
         <%= t('protocols.index.external_protocols.list_panel.empty_text') %>
       </div>
+        <div class="external-protocol-result">
+          <a href="#" data-source="protocolsio/v3" data-id="11176" data-action="external-import" data-url="<%= team_build_external_protocol_path(current_team.id,) %>">Protocols IO, 11176</a>
+        </div>
 
       <div class='protocol-card'
         data-protocol-source='protocolsio/v3'
@@ -72,11 +75,6 @@
         data-show-protocol-id='errorr'>
         Error protocol (click me, should default to default screen)
       </div>
-
-      <div class="exteral-proocol-result">
-        <a href="#" data-source="protocolsio/v3" data-id="11176" data-action="external-import" data-url="<%= team_build_external_protocol_path(current_team.id,) %>">Protocols IO, 11176</a>
-      </div>
-
     </div>
 
     <div class='col-md-7 protocol-preview-panel'>

app/views/steps/_step.html.erb

@@ -1,4 +1,5 @@
 <% preview = (defined?(preview) ? preview : false) %>
+<% import = (defined?(import) ? import : false) %>
 <div class ="step <%= step.completed? ? "completed" : "not-completed" %>">
   <div class="panel panel-default">
     <div class="panel-heading">
@@ -113,7 +114,12 @@
             </div>
           <% end %>
 
+          <% if import %>
+            <%= render partial: 'steps/attachments/preview_list.html.erb',
+                       locals: {  attachments: steps_assets[step.position]} %>
+          <% else %>
             <%= render partial: 'steps/attachments/list.html.erb', locals: {  step: step, preview: preview } %>
+          <% end %>
 
           <% unless step.checklists.blank? then %>
             <div class="col-xs-12">
@@ -147,10 +153,12 @@
             </div>
           <% end %>
         </div>
+        <% unless import %>
           <hr>
           <%= render partial: 'steps/comments.html.erb', locals: {  comments: step.last_comments,
                                                                     comments_count: step.step_comments.count,
                                                                     step: step } %>
+        <% end %>
       </div>
     </div>
   </div>

app/views/steps/attachments/_new_attachment.html.erb

@@ -0,0 +1,9 @@
+<div class="attachment-placeholder pull-left new">
+  <div class="attachment-thumbnail no-shadow">
+    <i class="fas fa-image"></i>
+  </div>
+  <div class="attachment-label"><%= truncate(file_name || file_url, length: Constants::FILENAME_TRUNCATION_LENGTH) %>
+  </div>
+  <div class="spencer-bonnet-modif">
+  </div>
+</div>

app/views/steps/attachments/_preview_list.html.erb

@@ -0,0 +1,23 @@
+<div class="col-xs-12">
+  <hr>
+</div>
+<div class="col-xs-12 attachments-actions">
+
+  <div class="title">
+    <h4>
+      <%= t('protocols.steps.files', count: attachments.size) %>
+    </h4>
+  </div>
+  <div>
+    <div class="attachemnts-header pull-right">
+    </div>
+  </div>
+</div>
+
+<div class="col-xs-12 attachments">
+  <% attachments.each do |a| %>
+    <%= render partial: 'steps/attachments/new_attachment.html.erb',
+               locals: {  file_url: a[:url], file_name: a[:name] } %>
+  <% end %>
+</div>
+<hr>

config/locales/protocols/en.yml

@@ -1,5 +1,7 @@
 en:
   protocol_importers:
+    new:
+      modal_title: 'Import protocol - %{protocol_name}'
     templates:
       amount:
         title: 'Amount'

spec/controllers/external_protocols_controller_spec.rb

@@ -110,6 +110,8 @@ describe ExternalProtocolsController, type: :controller do
         service = double('success_service')
         allow(service).to(receive(:succeed?)).and_return(true)
         allow(service).to(receive(:built_protocol)).and_return(protocol)
+        allow(service).to(receive(:serialized_steps)).and_return({}.to_s)
+        allow(service).to(receive(:steps_assets)).and_return([])
 
         allow_any_instance_of(ProtocolImporters::BuildProtocolFromClientService)
           .to(receive(:call)).and_return(service)
@@ -148,8 +150,10 @@ describe ExternalProtocolsController, type: :controller do
       let(:params) do
         {
           team_id: team.id,
-          protocol_params: {},
+          protocol: {
-          steps_params: {}
+            name: 'name',
+            steps: {}.to_s
+          }
         }
       end
 
@@ -159,7 +163,7 @@ describe ExternalProtocolsController, type: :controller do
         # Setup double
         service = double('success_service')
         allow(service).to(receive(:succeed?)).and_return(true)
-        allow(service).to(receive(:protocol)).and_return({})
+        allow(service).to(receive(:protocol)).and_return(create(:protocol))
 
         allow_any_instance_of(ProtocolImporters::ImportProtocolService).to(receive(:call)).and_return(service)
 

spec/services/protocol_importers/build_protocol_from_client_service_spec.rb

@@ -9,6 +9,14 @@ describe ProtocolImporters::BuildProtocolFromClientService do
     ProtocolImporters::BuildProtocolFromClientService
       .call(protocol_client_id: 'id', protocol_source: 'protocolsio/v3', user_id: user.id, team_id: team.id)
   end
+  let(:service_call_without_assets) do
+    ProtocolImporters::BuildProtocolFromClientService
+      .call(protocol_client_id: 'id',
+            protocol_source: 'protocolsio/v3',
+            user_id: user.id,
+            team_id: team.id,
+            build_with_assets: false)
+  end
   let(:normalized_response) do
     JSON.parse(file_fixture('protocol_importers/normalized_single_protocol.json').read)
         .to_h.with_indifferent_access
@@ -16,7 +24,7 @@ describe ProtocolImporters::BuildProtocolFromClientService do
 
   context 'when have invalid arguments' do
     it 'returns an error when can\'t find user' do
-      allow(User).to receive(:find).and_return(nil)
+      allow(User).to receive(:find_by_id).and_return(nil)
 
       expect(service_call.errors).to have_key(:invalid_arguments)
     end
@@ -69,5 +77,13 @@ describe ProtocolImporters::BuildProtocolFromClientService do
       expect(service_call.built_protocol).to be_instance_of(Protocol)
     end
     # more tests will be implemented when add error handling to service
+
+    describe 'serialized_steps' do
+      context 'when build without assets' do
+        it 'returns JSON with attachments' do
+          expect(JSON.parse(service_call_without_assets.serialized_steps).first).to have_key('attachments')
+        end
+      end
+    end
   end
 end

spec/services/protocol_importers/import_protocol_service_spec.rb

@@ -8,19 +8,19 @@ describe ProtocolImporters::ImportProtocolService do
   let(:protocol_params) { attributes_for :protocol, :in_public_repository }
   let(:steps_params) do
     [
-      attributes_for(:step).merge!(assets_attributes: [attributes_for(:asset)]),
+      attributes_for(:step).merge!(attachments: [{ name: 'random.jpg', url: 'http://www.example.com/random.jpg' }]),
       attributes_for(:step).merge!(tables_attributes: [attributes_for(:table), attributes_for(:table)])
-    ]
+    ].to_json
   end
 
   let(:service_call) do
     ProtocolImporters::ImportProtocolService
-      .call(protocol_params: protocol_params, steps_params: steps_params, user_id: user.id, team_id: team.id)
+      .call(protocol_params: protocol_params, steps_params_json: steps_params, user_id: user.id, team_id: team.id)
   end
 
   context 'when have invalid arguments' do
     it 'returns an error when can\'t find user' do
-      allow(User).to receive(:find).and_return(nil)
+      allow(User).to receive(:find_by_id).and_return(nil)
 
       expect(service_call.errors).to have_key(:invalid_arguments)
     end
@@ -28,19 +28,20 @@ describe ProtocolImporters::ImportProtocolService do
     it 'returns invalid protocol when can\'t save it' do
       # step with file without name
       steps_invalid_params = [
-        attributes_for(:step).merge!(assets_attributes: [attributes_for(:asset).except(:file_file_name)])
+        attributes_for(:step).except(:name).merge!(tables_attributes: [attributes_for(:table)])
-      ]
+      ].to_json
 
       s = ProtocolImporters::ImportProtocolService.call(protocol_params: protocol_params,
-                                                        steps_params: steps_invalid_params,
+                                                        steps_params_json: steps_invalid_params,
                                                         user_id: user.id, team_id: team.id)
-
       expect(s.protocol).to be_invalid
     end
   end
 
   context 'when have valid arguments' do
     before do
+      stub_request(:get, 'http://www.example.com/random.jpg').to_return(status: 200, body: '', headers: {})
+
       @protocol = Protocol.new
     end
 

spec/utilities/protocol_importers/attachments_builder_spec.rb

@@ -8,6 +8,7 @@ RSpec.describe ProtocolImporters::AttachmentsBuilder do
   end
   let(:generate_files_from_step) { described_class.generate(step) }
   let(:first_file_in_result) { generate_files_from_step.first }
+  let(:generate_json_files_from_step) { described_class.generate_json(step) }
 
   before do
     stub_request(:get, 'https://pbs.twimg.com/media/Cwu3zrZWQAA7axs.jpg').to_return(status: 200, body: '', headers: {})
@@ -15,7 +16,7 @@ RSpec.describe ProtocolImporters::AttachmentsBuilder do
       .to_return(status: 200, body: '', headers: {})
   end
 
-  describe 'self.build_assets_for_step' do
+  describe 'self.generate' do
     it 'returns array of Asset instances' do
       expect(first_file_in_result).to be_instance_of(Asset)
     end
@@ -24,4 +25,14 @@ RSpec.describe ProtocolImporters::AttachmentsBuilder do
       expect(first_file_in_result).to be_valid
     end
   end
+
+  describe 'self.generate_json' do
+    it 'returns JSON with 2 items (files)' do
+      expect(generate_json_files_from_step.size).to be == 2
+    end
+
+    it 'returns JSON item with name and url' do
+      expect(generate_json_files_from_step.first.keys).to contain_exactly(:name, :url)
+    end
+  end
 end

spec/utilities/protocol_importers/protocol_intermediate_object_spec.rb

@@ -5,6 +5,9 @@ require 'rails_helper'
 describe ProtocolImporters::ProtocolIntermediateObject do
   subject(:pio) { described_class.new(normalized_json: normalized_result, user: user, team: team) }
   let(:invalid_pio) { described_class.new(normalized_json: normalized_result, user: nil, team: team) }
+  let(:pio_without_assets) do
+    described_class.new(normalized_json: normalized_result, user: user, team: team, build_with_assets: false)
+  end
   let(:user) { create :user }
   let(:team) { create :team }
   let(:normalized_result) do
@@ -34,5 +37,26 @@ describe ProtocolImporters::ProtocolIntermediateObject do
       it { expect(invalid_pio.import).to be_invalid }
       it { expect { invalid_pio.import }.not_to(change { Protocol.all.count }) }
     end
+
+    context 'when build wihout assets' do
+      it { expect { pio_without_assets.import }.to change { Protocol.all.count }.by(1) }
+      it { expect { invalid_pio.import }.not_to(change { Asset.all.count }) }
+    end
+  end
+
+  describe '.steps_assets' do
+    context 'when have default pio' do
+      it 'retuns empty hash for steps_assets' do
+        pio.build
+        expect(pio.steps_assets).to be == {}
+      end
+    end
+
+    context 'when have pio built without assets' do
+      it 'returns hash with two assets on steps by position' do
+        pio_without_assets.build
+        expect(pio_without_assets.steps_assets.size).to be == 2
+      end
+    end
   end
 end