From 8b334985dabc0081279979b93dcc509c6b913da4 Mon Sep 17 00:00:00 2001
From: Harriet Craven <hc6@sanger.ac.uk>
Date: Wed, 27 Jan 2021 12:07:46 +0000
Subject: [PATCH 01/18] update bundler

---
 Gemfile.lock | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Gemfile.lock b/Gemfile.lock
index 774f319..a71db64 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -232,4 +232,4 @@ DEPENDENCIES
   sqlite3
 
 BUNDLED WITH
-   2.1.4
+   2.2.6

From 40d28435f96a9159bf3d8e0d66b6ff81ba0caf3d Mon Sep 17 00:00:00 2001
From: Harriet Craven <hc6@sanger.ac.uk>
Date: Wed, 27 Jan 2021 15:59:50 +0000
Subject: [PATCH 02/18] started on code review comments

---
 app/models/print_job_wrapper.rb               | 77 +++++++++++++++++++
 app/models/squix/print_job_wrapper.rb         | 68 ----------------
 .../{squix => }/print_job_wrapper_spec.rb     | 16 ++--
 3 files changed, 85 insertions(+), 76 deletions(-)
 create mode 100644 app/models/print_job_wrapper.rb
 delete mode 100644 app/models/squix/print_job_wrapper.rb
 rename spec/models/{squix => }/print_job_wrapper_spec.rb (71%)

diff --git a/app/models/print_job_wrapper.rb b/app/models/print_job_wrapper.rb
new file mode 100644
index 0000000..17d180e
--- /dev/null
+++ b/app/models/print_job_wrapper.rb
@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+# PrintJobWrapper
+class PrintJobWrapper
+  include ActiveModel::Model
+
+  attr_reader :copies
+  attr_accessor :printer_name, :label_template_name, :label_template_id, :labels, :errors
+
+  # TODO: validate
+  # validate :check_attributes, :check_print_job
+
+  def print
+    # return false unless valid?
+
+    case printer_type
+    when 'Toshiba'
+      print_to_toshiba
+    when 'Squix'
+      print_to_squix
+    else
+      # TODO: handle errors
+      # errors << "Printer type #{printer_type} for printer #{printer_name} not recognised."
+      false
+    end
+  end
+
+  def printer_type
+    Printer.find_by(name: printer_name).type
+  end
+
+  def print_to_toshiba
+    unless label_template_id
+      @label_template_id = LabelTemplate.find_by(name: label_template_name).id
+    end
+
+    # labels here from LabWhere already includes the correct number of copies
+    body = { printer_name: printer_name, label_template_id: label_template_id, labels: labels }
+
+    print_job = LabelPrinter::PrintJob.build(body)
+    print_job.execute
+    # do something with response
+  end
+
+  def print_to_squix
+    squix_print_job = Squix::PrintJob.new(
+      printer_name: printer_name,
+      label_template_name:
+      label_template_name,
+      labels: labels,
+      copies: copies
+    )
+    squix_print_job.execute
+    # do something with response
+  end
+
+  def create_response
+    { message: '', status: '' }
+  end
+
+  # def check_attributes
+  #   errors.add(:printer_name, 'does not exist') if printer_name.nil?
+  #   errors.add(:label_template_name, 'does not exist') if label_template_name.nil?
+  # end
+
+  # def check_print_job
+  # case printer_type
+  # when 'Toshiba'
+  #   toshiba.valid?
+  # when 'Squix'
+  #   toshiba.valid?
+  # end
+
+  def copies=(copies)
+    @copies = copies.to_i
+  end
+end
diff --git a/app/models/squix/print_job_wrapper.rb b/app/models/squix/print_job_wrapper.rb
deleted file mode 100644
index e1237b6..0000000
--- a/app/models/squix/print_job_wrapper.rb
+++ /dev/null
@@ -1,68 +0,0 @@
-# frozen_string_literal: true
-
-module Squix
-  # Squix::PrintJobWrapper
-  class PrintJobWrapper
-    include ActiveModel::Model
-
-    attr_accessor :printer_name, :label_template_name, :label_template_id, :labels, :copies, :errors
-
-    # validate
-
-    def initialize(printer_name, label_template_name, label_template_id, labels, copies)
-      @printer_name = printer_name
-      @label_template_name = label_template_name
-      @label_template_id = label_template_id
-      @labels = labels
-      @copies = copies.to_i
-      @errors = []
-    end
-
-    def print
-      # return false unless valid?
-
-      case printer_type
-      when 'Toshiba'
-        print_to_toshiba
-      when 'Squix'
-        print_to_squix
-      else
-        errors << "Printer type #{printer_type} for printer #{printer_name} not recognised."
-        false
-      end
-    end
-
-    def printer_type
-      Printer.find_by(name: printer_name).type
-    end
-
-    def print_to_toshiba
-      unless label_template_id
-        @label_template_id = LabelTemplate.find_by(name: label_template_name).id
-      end
-
-      # labels here from LabWhere already includes the correct number of copies
-      body = { printer_name: printer_name, label_template_id: label_template_id, labels: labels }
-
-      print_job = LabelPrinter::PrintJob.build(body)
-      print_job.execute
-      # do something with response
-    end
-
-    def print_to_squix
-      squix_print_job = Squix::PrintJob.new(
-        printer_name: printer_name,
-        label_template_name:
-        label_template_name,
-        labels: labels,
-        copies: copies
-      )
-      squix_print_job.execute
-      # do something with response
-    end
-
-    def create_response
-      { message: '', status: '' }
-    end
-  end
-end
diff --git a/spec/models/squix/print_job_wrapper_spec.rb b/spec/models/print_job_wrapper_spec.rb
similarity index 71%
rename from spec/models/squix/print_job_wrapper_spec.rb
rename to spec/models/print_job_wrapper_spec.rb
index 9aa9bfb..1ac3a79 100644
--- a/spec/models/squix/print_job_wrapper_spec.rb
+++ b/spec/models/print_job_wrapper_spec.rb
@@ -2,7 +2,7 @@
 
 require 'rails_helper'
 
-RSpec.describe Squix::PrintJobWrapper do
+RSpec.describe PrintJobWrapper do
 
   let!(:printer)          { create(:printer) }
   let(:label_template)      { create(:label_template) }
@@ -11,7 +11,7 @@
 
   describe 'init' do
     it 'has the correct insatnce variables' do
-      pjw = Squix::PrintJobWrapper.new(printer.name, label_template.name, label_template.id, labels, copies)
+      pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
 
       assert_equal printer.name, pjw.printer_name
       assert_equal label_template.id, pjw.label_template_id
@@ -23,7 +23,7 @@
 
   describe 'print' do
     it 'calls print_to_toshiba when printer_type is Toshiba' do
-      pjw = Squix::PrintJobWrapper.new(printer.name, label_template.name, label_template.id, labels, copies)
+      pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
       allow(pjw).to receive(:printer_type).and_return('Toshiba')
 
       expect(pjw).to receive(:print_to_toshiba)
@@ -31,7 +31,7 @@
     end
 
     it 'calls print_to_sprint when service is Squix' do
-      pjw = Squix::PrintJobWrapper.new(printer.name,label_template.name, label_template.id, labels, copies)
+      pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
       allow(pjw).to receive(:printer_type).and_return('Squix')
 
       expect(pjw).to receive(:print_to_squix)
@@ -39,7 +39,7 @@
     end
 
     it 'adds an error when service is down' do
-      pjw = Squix::PrintJobWrapper.new(printer.name,label_template.name, label_template.id, labels, copies)
+      pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
       allow(pjw).to receive(:printer_type).and_return('Unknown')
 
       expect(pjw).not_to receive(:print_to_toshiba)
@@ -54,7 +54,7 @@
       it 'builds a LabelPrinter::PrintJob with the correct arributes and executes' do
         label_template = create(:label_template)
         print_job = build(:print_job)
-        pjw = Squix::PrintJobWrapper.new(printer.name, label_template.name, nil, labels, copies)
+        pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: label_template.name, label_template_id: nil, labels: labels, copies: copies })
 
         allow(LabelPrinter::PrintJob).to receive(:build).and_return(print_job)
 
@@ -68,7 +68,7 @@
     describe 'when label_template id is present' do
       it 'builds a LabelPrinter::PrintJob with the correct arributes' do
         print_job = build(:print_job)
-        pjw = Squix::PrintJobWrapper.new(printer.name, nil, label_template.id, labels, copies)
+        pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: nil, label_template_id: label_template.id, labels: labels, copies: copies })
 
         allow(LabelPrinter::PrintJob).to receive(:build).and_return(print_job)
 
@@ -84,7 +84,7 @@
   describe 'print_to_squix' do
     it 'builds a Squix::PrintJob with the correct arributes' do
       squix_print_job = build(:squix_print_job)
-      pjw = Squix::PrintJobWrapper.new(printer.name, label_template.name, label_template.id, labels, copies)
+      pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
 
       allow(Squix::PrintJob).to receive(:new).and_return(squix_print_job)
 

From fddb7bd889f6e08533f3527e7f7df752bafaa974 Mon Sep 17 00:00:00 2001
From: Harriet Craven <hc6@sanger.ac.uk>
Date: Wed, 27 Jan 2021 16:19:15 +0000
Subject: [PATCH 03/18] return printer rather than printer type in wrapper

---
 app/models/print_job_wrapper.rb       | 12 ++++--------
 spec/models/print_job_wrapper_spec.rb | 28 +++++++++------------------
 2 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/app/models/print_job_wrapper.rb b/app/models/print_job_wrapper.rb
index 17d180e..ce7d736 100644
--- a/app/models/print_job_wrapper.rb
+++ b/app/models/print_job_wrapper.rb
@@ -13,20 +13,16 @@ class PrintJobWrapper
   def print
     # return false unless valid?
 
-    case printer_type
+    case printer.printer_type
     when 'Toshiba'
       print_to_toshiba
     when 'Squix'
       print_to_squix
-    else
-      # TODO: handle errors
-      # errors << "Printer type #{printer_type} for printer #{printer_name} not recognised."
-      false
     end
   end
 
-  def printer_type
-    Printer.find_by(name: printer_name).type
+  def printer
+    Printer.find_by(name: printer_name)
   end
 
   def print_to_toshiba
@@ -64,7 +60,7 @@ def create_response
   # end
 
   # def check_print_job
-  # case printer_type
+  # case printer.printer_type
   # when 'Toshiba'
   #   toshiba.valid?
   # when 'Squix'
diff --git a/spec/models/print_job_wrapper_spec.rb b/spec/models/print_job_wrapper_spec.rb
index 1ac3a79..fdc0a0f 100644
--- a/spec/models/print_job_wrapper_spec.rb
+++ b/spec/models/print_job_wrapper_spec.rb
@@ -4,10 +4,10 @@
 
 RSpec.describe PrintJobWrapper do
 
-  let!(:printer)          { create(:printer) }
+  let!(:printer)            { create(:printer) }
   let(:label_template)      { create(:label_template) }
   let(:labels)              { [{ 'label' => { 'test_attr' => 'test', 'barcode' => '12345' } }] }
-  let(:copies)   { 1 }
+  let(:copies)              { 1 }
 
   describe 'init' do
     it 'has the correct insatnce variables' do
@@ -21,32 +21,22 @@
     end
   end
 
+  # Un 'x' below when merged in GPL-831-2
   describe 'print' do
-    it 'calls print_to_toshiba when printer_type is Toshiba' do
-      pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
-      allow(pjw).to receive(:printer_type).and_return('Toshiba')
-
+    xit 'calls print_to_toshiba when printer_type is Toshiba' do
+      toshiba_printer = create(:printer, printer_type: :toshiba)
+      pjw = PrintJobWrapper.new({ printer_name: toshiba_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
       expect(pjw).to receive(:print_to_toshiba)
       pjw.print
     end
 
-    it 'calls print_to_sprint when service is Squix' do
-      pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
-      allow(pjw).to receive(:printer_type).and_return('Squix')
+    xit 'calls print_to_sprint when printer_type is Squix' do
+      squix_printer = create(:printer, printer_type: :squix)
+      pjw = PrintJobWrapper.new({ printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
 
       expect(pjw).to receive(:print_to_squix)
       pjw.print
     end
-
-    it 'adds an error when service is down' do
-      pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
-      allow(pjw).to receive(:printer_type).and_return('Unknown')
-
-      expect(pjw).not_to receive(:print_to_toshiba)
-      expect(pjw).not_to receive(:print_to_squix)
-      # expect(pjw.errors).to match /Printer type not recognised./
-      pjw.print
-    end
   end
 
   describe 'print_to_toshiba' do

From b3bfaeb3b7cada8ef846eae08c66d7e74551fa60 Mon Sep 17 00:00:00 2001
From: Harriet Craven <hc6@sanger.ac.uk>
Date: Thu, 28 Jan 2021 11:20:53 +0000
Subject: [PATCH 04/18] fixes print_job params

---
 app/controllers/v2/print_jobs_controller.rb | 25 +++++++++++++--------
 spec/v2/print_jobs_spec.rb                  | 21 +++++++++++++++++
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/app/controllers/v2/print_jobs_controller.rb b/app/controllers/v2/print_jobs_controller.rb
index fbc76b5..73c5f95 100644
--- a/app/controllers/v2/print_jobs_controller.rb
+++ b/app/controllers/v2/print_jobs_controller.rb
@@ -9,21 +9,28 @@ def create
       if print_job_wrapper.print
         render json: { message: 'labels successfully printed' }
       else
-        render_error print_job_wrapper
+        render 'error'
+        # render_error print_job_wrapper
       end
     end
 
     private
 
-    # TODO: fix
     def print_job_params
-      params.require(:print_job).permit(
-        :printer_name,
-        :label_template_name,
-        :label_template_id,
-        :copies,
-        # labels: []
-      )
+      p1 = params.permit(
+        print_job: %i[
+          printer_name
+          label_template_name
+          label_template_id
+          copies
+        ]
+      )[:print_job].to_h
+
+      p1.merge(labels: labels_params)
+    end
+
+    def labels_params
+      params.require(:print_job)[:labels].map { |label| label.permit!.to_h }
     end
   end
 end
diff --git a/spec/v2/print_jobs_spec.rb b/spec/v2/print_jobs_spec.rb
index 1010000..a47b8d7 100644
--- a/spec/v2/print_jobs_spec.rb
+++ b/spec/v2/print_jobs_spec.rb
@@ -11,6 +11,12 @@
 
   it 'should print a valid label' do
     body = { printer_name: printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies }
+    pjw = PrintJobWrapper.new(body)
+
+    allow(PrintJobWrapper).to receive(:new).and_return(pjw)
+    allow(pjw).to receive(:print).and_return(true)
+    expect(PrintJobWrapper).to receive(:new)
+
     post v2_print_jobs_path, params: { print_job: body }
     expect(response).to be_successful
     expect(response).to have_http_status(:success)
@@ -18,6 +24,21 @@
     expect(json['message']).to eq('labels successfully printed')
   end
 
+  xit 'should return an error if print fails' do
+    body = { label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies }
+    pjw = PrintJobWrapper.new(body)
+
+    allow(PrintJobWrapper).to receive(:new).and_return(pjw)
+    allow(pjw).to receive(:print).and_return(false)
+    expect(PrintJobWrapper).to receive(:new)
+
+    post v2_print_jobs_path, params: { print_job: body }
+    expect(response).to_not be_successful
+    expect(response).to have_http_status(:unprocessable_entity)
+    json = ActiveSupport::JSON.decode(response.body)
+    expect(find_attribute_error_details(json, 'label_template')).to include('does not exist')
+  end
+
   # TODO: fix
   xit 'should return an error if request provides incorrect parameters' do
     post v2_print_jobs_path, params: { print_job: { printer_name: printer.name } }

From cbf577160812c50063054c88671e7e97d9282eb3 Mon Sep 17 00:00:00 2001
From: Harriet Craven <hc6@sanger.ac.uk>
Date: Thu, 28 Jan 2021 12:59:39 +0000
Subject: [PATCH 05/18] feedback from code review

---
 app/models/print_job_wrapper.rb       |  71 +++++-------
 spec/models/print_job_wrapper_spec.rb | 152 +++++++++++++++++---------
 2 files changed, 127 insertions(+), 96 deletions(-)

diff --git a/app/models/print_job_wrapper.rb b/app/models/print_job_wrapper.rb
index 57f6eb2..a0eeb71 100644
--- a/app/models/print_job_wrapper.rb
+++ b/app/models/print_job_wrapper.rb
@@ -7,67 +7,48 @@ class PrintJobWrapper
   attr_reader :copies
   attr_accessor :printer_name, :label_template_name, :label_template_id, :labels, :errors
 
-  # TODO: validate
-  # validate :check_attributes, :check_print_job
-
   def print
-    # return false unless valid?
+    return false unless print_job.valid?
+
+    print_job.execute
+    # do something with response
+  end
 
+  def print_job
     case printer.printer_type
     when 'toshiba'
-      print_to_toshiba
+      @print_job = toshiba_print_job
     when 'squix'
-      print_to_squix
+      @print_job = squix_print_job
     end
-    true
   end
 
-  def printer
-    Printer.find_by(name: printer_name)
+  def toshiba_print_job
+    LabelPrinter::PrintJob.build(print_job_body)
   end
 
-  def print_to_toshiba
-    unless label_template_id
-      @label_template_id = LabelTemplate.find_by(name: label_template_name).id
-    end
-
-    # labels here from LabWhere already includes the correct number of copies
-    body = { printer_name: printer_name, label_template_id: label_template_id, labels: labels }
-
-    print_job = LabelPrinter::PrintJob.build(body)
-    print_job.execute
-    # do something with response
+  def squix_print_job
+    Squix::PrintJob.new(print_job_body)
   end
 
-  def print_to_squix
-    squix_print_job = Squix::PrintJob.new(
-      printer_name: printer_name,
-      label_template_name:
-      label_template_name,
-      labels: labels,
-      copies: copies
-    )
-    squix_print_job.execute
-    # do something with response
+  def print_job_body
+    case printer.printer_type
+    when 'toshiba'
+      unless label_template_id
+        @label_template_id = LabelTemplate.find_by(name: label_template_name).id
+      end
+      @print_job_body = { printer_name: printer_name, label_template_id: label_template_id,
+                          labels: labels }
+    when 'squix'
+      @print_job_body = { printer_name: printer_name, label_template_name: label_template_name,
+                          labels: labels, copies: copies }
+    end
   end
 
-  def create_response
-    { message: '', status: '' }
+  def printer
+    Printer.find_by(name: printer_name)
   end
 
-  # def check_attributes
-  #   errors.add(:printer_name, 'does not exist') if printer_name.nil?
-  #   errors.add(:label_template_name, 'does not exist') if label_template_name.nil?
-  # end
-
-  # def check_print_job
-  # case printer.printer_type
-  # when 'Toshiba'
-  #   toshiba.valid?
-  # when 'Squix'
-  #   toshiba.valid?
-  # end
-
   def copies=(copies)
     @copies = copies.to_i
   end
diff --git a/spec/models/print_job_wrapper_spec.rb b/spec/models/print_job_wrapper_spec.rb
index e13526a..8f606c6 100644
--- a/spec/models/print_job_wrapper_spec.rb
+++ b/spec/models/print_job_wrapper_spec.rb
@@ -4,93 +4,143 @@
 
 RSpec.describe PrintJobWrapper do
 
-  let!(:printer)            { create(:printer) }
+  let!(:squix_printer)       { create(:printer, printer_type: :squix) }
+  let!(:toshiba_printer)     { create(:printer, printer_type: :toshiba) }
   let(:label_template)      { create(:label_template) }
   let(:labels)              { [{ 'label' => { 'test_attr' => 'test', 'barcode' => '12345' } }] }
-  let(:copies)              { 1 }
+  let(:copies)              { '1' }
 
   describe 'init' do
-    it 'has the correct insatnce variables' do
-      pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
+    it 'has the correct instance variables' do
+      pjw = PrintJobWrapper.new({ printer_name: toshiba_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
 
-      assert_equal printer.name, pjw.printer_name
+      assert_equal toshiba_printer.name, pjw.printer_name
       assert_equal label_template.id, pjw.label_template_id
       assert_equal label_template.name, pjw.label_template_name
       assert_equal labels, pjw.labels
-      assert_equal copies, pjw.copies
+      assert_equal copies.to_i, pjw.copies
     end
   end
 
-  describe 'print' do
-    it 'calls print_to_toshiba when printer_type is Toshiba' do
-      toshiba_printer = create(:printer, printer_type: :toshiba)
-      pjw = PrintJobWrapper.new({ printer_name: toshiba_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
-      expect(pjw).to receive(:print_to_toshiba)
-      pjw.print
+  describe '#print_job' do
+    it 'sets the print job for a Toshiba printer' do
+      body = { printer_name: toshiba_printer.name, label_template_id: label_template.id, labels: labels }
+      pjw = PrintJobWrapper.new(body)
+      toshiba_print_job = LabelPrinter::PrintJob.build(body)
+
+      allow(pjw).to receive(:toshiba_print_job).and_return(toshiba_print_job)
+      expect(pjw.print_job).to eq pjw.toshiba_print_job
     end
 
-    it 'calls print_to_sprint when printer_type is Squix' do
-      squix_printer = create(:printer, printer_type: :squix)
-      pjw = PrintJobWrapper.new({ printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
+    it 'sets the print job for a Squix printer' do
+      body = { printer_name: squix_printer.name, label_template_name: label_template.name, labels: labels, copies: copies }
+      pjw = PrintJobWrapper.new(body)
+      squix_print_job = Squix::PrintJob.new(body)
 
-      expect(pjw).to receive(:print_to_squix)
-      pjw.print
+      allow(pjw).to receive(:squix_print_job).and_return(squix_print_job)
+      expect(pjw.print_job).to eq pjw.squix_print_job
     end
   end
 
-  describe 'print_to_toshiba' do
-    describe 'when label_template id is not present' do
-      it 'builds a LabelPrinter::PrintJob with the correct arributes and executes' do
-        label_template = create(:label_template)
-        print_job = build(:print_job)
-        pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: label_template.name, label_template_id: nil, labels: labels, copies: copies })
+  describe '#print' do
+    context 'when printer_type is Toshiba' do
+      it 'validates the print job and calls execute when valid' do
+        body = { printer_name: toshiba_printer.name, label_template_id: label_template.id, labels: labels }
+
+        pjw = PrintJobWrapper.new(body)
+        toshiba_print_job = LabelPrinter::PrintJob.build(body)
+
+        allow(pjw).to receive(:toshiba_print_job).and_return(toshiba_print_job)
+        allow(toshiba_print_job).to receive(:execute)
 
-        allow(LabelPrinter::PrintJob).to receive(:build).and_return(print_job)
+        pjw.print
+        expect(toshiba_print_job.valid?).to eq true
+      end
+
+      it 'validates the print job and returns false when invalid' do
+        body_missing_attribute = { printer_name: toshiba_printer.name, labels: labels }
+
+        pjw = PrintJobWrapper.new(body_missing_attribute)
+        toshiba_print_job = LabelPrinter::PrintJob.build(body_missing_attribute)
 
-        expected_body = { label_template_id: label_template.id, labels: labels, printer_name: printer.name }
-        expect(LabelPrinter::PrintJob).to receive(:build).with(expected_body)
-        expect(print_job).to receive(:execute)
-        pjw.print_to_toshiba
+        allow(pjw).to receive(:toshiba_print_job).and_return(toshiba_print_job)
+        allow(toshiba_print_job).to receive(:execute)
+
+        pjw.print
+        expect(toshiba_print_job.valid?).to eq false
+        expect(toshiba_print_job.errors.messages).to eq({ label_template: ["does not exist"] })
       end
     end
 
-    describe 'when label_template id is present' do
-      it 'builds a LabelPrinter::PrintJob with the correct arributes' do
-        print_job = build(:print_job)
-        pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: nil, label_template_id: label_template.id, labels: labels, copies: copies })
+    context 'when printer_type is Squix' do
+      it 'validates the print job and calls execute when valid' do
+        body = { printer_name: squix_printer.name, label_template_name: label_template.name, labels: labels, copies: copies }
+        pjw = PrintJobWrapper.new(body)
+        squix_print_job = Squix::PrintJob.new(body)
 
-        allow(LabelPrinter::PrintJob).to receive(:build).and_return(print_job)
+        allow(pjw).to receive(:squix_print_job).and_return(squix_print_job)
+        allow(squix_print_job).to receive(:execute)
 
-        expected_body = { label_template_id: label_template.id, labels: labels, printer_name: printer.name }
-        expect(LabelPrinter::PrintJob).to receive(:build).with(expected_body)
-        expect(print_job).to receive(:execute)
+        pjw.print
+        expect(squix_print_job.valid?).to eq true
+      end
 
-        pjw.print_to_toshiba
+      it 'validates the print job and returns false when invalid' do
+        body_missing_attribute = { printer_name: squix_printer.name, labels: labels, copies: copies }
+        pjw = PrintJobWrapper.new(body_missing_attribute)
+        squix_print_job = Squix::PrintJob.new(body_missing_attribute)
+
+        allow(pjw).to receive(:squix_print_job).and_return(squix_print_job)
+        allow(squix_print_job).to receive(:execute)
+
+        pjw.print
+        expect(squix_print_job.valid?).to eq false
+        expect(squix_print_job.errors.messages).to eq({ label_template_name: ["can't be blank"] })
       end
     end
   end
 
-  describe 'print_to_squix' do
-    it 'builds a Squix::PrintJob with the correct arributes' do
-      squix_print_job = build(:squix_print_job)
-      pjw = PrintJobWrapper.new({ printer_name: printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
-
-      allow(Squix::PrintJob).to receive(:new).and_return(squix_print_job)
+  describe '#toshiba_print_job' do
+    it 'builds a LabelPrinter::PrintJob when label_template_id is provided' do
+      body = { printer_name: toshiba_printer.name, label_template_id: label_template.id, labels: labels }
+      pjw = PrintJobWrapper.new(body)
+      expect(pjw.toshiba_print_job).to be_instance_of(LabelPrinter::PrintJob::LPD)
+    end
 
-      expected_body = { printer_name: printer.name, label_template_name: label_template.name, labels: labels, copies: copies }
-      expect(Squix::PrintJob).to receive(:new).with(expected_body)
-      expect(squix_print_job).to receive(:execute)
+    it 'builds a LabelPrinter::PrintJob when label_template_name is provided' do
+      body = { printer_name: toshiba_printer.name, label_template_name: label_template.name, labels: labels }
+      pjw = PrintJobWrapper.new(body)
+      expect(pjw.toshiba_print_job).to be_instance_of(LabelPrinter::PrintJob::LPD)
+    end
+  end
 
-      pjw.print_to_squix
+  describe '#squix_print_job' do
+    it 'builds a Squix::PrintJob' do
+      body = { printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies }
+      pjw = PrintJobWrapper.new(body)
+      expect(pjw.squix_print_job).to be_instance_of(Squix::PrintJob)
     end
   end
 
-  describe 'create_response' do
+  describe '#print_job_body' do
+    it 'sets the print_job_body for a Toshiba printer' do
+      body = { printer_name: toshiba_printer.name, label_template_name: label_template.name, labels: labels }
+      pjw = PrintJobWrapper.new(body)
+      expect(pjw.print_job_body.keys).to eq [:printer_name, :label_template_id, :labels]
+    end
+
+    it 'sets the print_job_body for a Squix printer' do
+      body = { printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies }
+      pjw = PrintJobWrapper.new(body)
+      expect(pjw.print_job_body.keys).to eq [:printer_name, :label_template_name, :labels, :copies]
+    end
   end
 
-  describe 'get_printer_type' do
-    # TODO GPL-831-2
-    it 'returns the correct printer type' do
+  describe '#printer' do
+    it 'returns the correct printer' do
+      body = { printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies }
+      pjw = PrintJobWrapper.new(body)
+      expect(pjw.printer).to eq squix_printer
     end
   end
 end

From 5bc3384b3f00e1338f7c00cbdfe67d4fa34b243b Mon Sep 17 00:00:00 2001
From: Harriet Craven <hc6@sanger.ac.uk>
Date: Thu, 28 Jan 2021 16:37:32 +0000
Subject: [PATCH 06/18] cleaned up post code review

---
 app/controllers/v2/print_jobs_controller.rb |  5 +-
 app/models/print_job_wrapper.rb             | 24 +++--
 spec/v2/print_jobs_spec.rb                  | 99 ++++++++++++++-------
 3 files changed, 87 insertions(+), 41 deletions(-)

diff --git a/app/controllers/v2/print_jobs_controller.rb b/app/controllers/v2/print_jobs_controller.rb
index 73c5f95..a569535 100644
--- a/app/controllers/v2/print_jobs_controller.rb
+++ b/app/controllers/v2/print_jobs_controller.rb
@@ -5,12 +5,11 @@ module V2
   class PrintJobsController < ApplicationController
     def create
       print_job_wrapper = PrintJobWrapper.new(print_job_params)
-      # TODO: fix
+
       if print_job_wrapper.print
         render json: { message: 'labels successfully printed' }
       else
-        render 'error'
-        # render_error print_job_wrapper
+        render_error print_job_wrapper
       end
     end
 
diff --git a/app/models/print_job_wrapper.rb b/app/models/print_job_wrapper.rb
index a0eeb71..814b5d9 100644
--- a/app/models/print_job_wrapper.rb
+++ b/app/models/print_job_wrapper.rb
@@ -5,21 +5,29 @@ class PrintJobWrapper
   include ActiveModel::Model
 
   attr_reader :copies
-  attr_accessor :printer_name, :label_template_name, :label_template_id, :labels, :errors
+  attr_accessor :printer_name, :label_template_name, :label_template_id, :labels
+
+  validate :check_printer
 
   def print
-    return false unless print_job.valid?
+    return false unless valid?
+
+    unless print_job.valid?
+      print_job.errors.each do |k, v|
+        errors.add(k, v)
+      end
+      return false
+    end
 
     print_job.execute
-    # do something with response
   end
 
   def print_job
     case printer.printer_type
     when 'toshiba'
-      @print_job = toshiba_print_job
+      @print_job ||= toshiba_print_job
     when 'squix'
-      @print_job = squix_print_job
+      @print_job ||= squix_print_job
     end
   end
 
@@ -52,4 +60,10 @@ def printer
   def copies=(copies)
     @copies = copies.to_i
   end
+
+  private
+
+  def check_printer
+    errors.add(:printer, 'does not exist') if printer.blank?
+  end
 end
diff --git a/spec/v2/print_jobs_spec.rb b/spec/v2/print_jobs_spec.rb
index a47b8d7..58f8ef2 100644
--- a/spec/v2/print_jobs_spec.rb
+++ b/spec/v2/print_jobs_spec.rb
@@ -4,50 +4,83 @@
 
 RSpec.describe V2::PrintJobsController, type: :request, helpers: true do
 
-  let!(:printer)          { create(:printer) }
+  let!(:squix_printer)    { create(:printer, printer_type: :squix) }
+  let!(:toshiba_printer)  { create(:printer, printer_type: :toshiba) }
   let!(:label_template)   { create(:label_template) }
   let(:labels)            { [{ 'test_attr' => 'test', 'barcode' => '12345' }] }
-  let(:copies)            { 1 }
+  let(:copies)            { '1' }
 
-  it 'should print a valid label' do
-    body = { printer_name: printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies }
-    pjw = PrintJobWrapper.new(body)
+  describe 'On success' do
+    context 'When printer type is Squix' do
+      it 'should send_print_request to SPrintClient ' do
+        body = { printer_name: squix_printer.name, label_template_name: label_template.name, labels: labels, copies: copies }
+        allow(SPrintClient).to receive(:send_print_request).and_return(true)
 
-    allow(PrintJobWrapper).to receive(:new).and_return(pjw)
-    allow(pjw).to receive(:print).and_return(true)
-    expect(PrintJobWrapper).to receive(:new)
+        post v2_print_jobs_path, params: { print_job: body }
 
-    post v2_print_jobs_path, params: { print_job: body }
-    expect(response).to be_successful
-    expect(response).to have_http_status(:success)
-    json = ActiveSupport::JSON.decode(response.body)
-    expect(json['message']).to eq('labels successfully printed')
+        expect(response).to be_successful
+        expect(response).to have_http_status(:success)
+        json = ActiveSupport::JSON.decode(response.body)
+        expect(json['message']).to eq('labels successfully printed')
+      end
+    end
+
+    context 'When printer type is Toshiba' do
+      it 'should send_print_request to SPrintClient ' do
+        body = { printer_name: toshiba_printer.name, label_template_id: label_template.id,
+        labels: labels }
+        allow_any_instance_of(LabelPrinter::PrintJob::LPD).to receive(:execute).and_return(true)
+
+        post v2_print_jobs_path, params: { print_job: body }
+        expect(response).to be_successful
+        expect(response).to have_http_status(:success)
+        json = ActiveSupport::JSON.decode(response.body)
+        expect(json['message']).to eq('labels successfully printed')
+      end
+    end
   end
 
-  xit 'should return an error if print fails' do
-    body = { label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies }
-    pjw = PrintJobWrapper.new(body)
+  describe 'On failure' do
+    context 'when wrapper is not valid' do
+      it 'should return an error if the printer name is missing' do
+        body = { printer_name: '', label_template_id: label_template.id, labels: labels }
 
-    allow(PrintJobWrapper).to receive(:new).and_return(pjw)
-    allow(pjw).to receive(:print).and_return(false)
-    expect(PrintJobWrapper).to receive(:new)
+        post v2_print_jobs_path, params: { print_job: body }
+        expect(response).to have_http_status(:unprocessable_entity)
 
-    post v2_print_jobs_path, params: { print_job: body }
-    expect(response).to_not be_successful
-    expect(response).to have_http_status(:unprocessable_entity)
-    json = ActiveSupport::JSON.decode(response.body)
-    expect(find_attribute_error_details(json, 'label_template')).to include('does not exist')
-  end
+        json = ActiveSupport::JSON.decode(response.body)
+
+        expect(json['errors']).not_to be_empty
+        expect(find_attribute_error_details(json, 'printer')).to include('does not exist')
+      end
+    end
+
+    context 'when toshiba_print_job is not valid' do
+      it 'should return an error' do
+        body = { printer_name: toshiba_printer.name, label_template_id: '', labels: labels }
+
+        post v2_print_jobs_path, params: { print_job: body }
+        expect(response).to have_http_status(:unprocessable_entity)
+
+        json = ActiveSupport::JSON.decode(response.body)
+
+        expect(json['errors']).not_to be_empty
+        expect(find_attribute_error_details(json, 'label_template')).to include('does not exist')
+      end
+    end
+
+    context 'when squix_print_job is not valid' do
+      it 'should return an error' do
+        body = { printer_name: squix_printer.name, label_template_name: label_template.name, labels: labels }
 
-  # TODO: fix
-  xit 'should return an error if request provides incorrect parameters' do
-    post v2_print_jobs_path, params: { print_job: { printer_name: printer.name } }
-    expect(response).to_not be_successful
-    expect(response).to have_http_status(:unprocessable_entity)
+        post v2_print_jobs_path, params: { print_job: body }
+        expect(response).to have_http_status(:unprocessable_entity)
 
-    json = ActiveSupport::JSON.decode(response.body)
+        json = ActiveSupport::JSON.decode(response.body)
 
-    expect(json['errors']).not_to be_empty
-    expect(find_attribute_error_details(json, 'label_template')).to include('does not exist')
+        expect(json['errors']).not_to be_empty
+        expect(find_attribute_error_details(json, 'copies')).to include("can't be blank")
+      end
+    end
   end
 end
\ No newline at end of file

From 654be2aa1df791b20e011609b3f22c3102a74976 Mon Sep 17 00:00:00 2001
From: Stephen Inglis <si5@sanger.ac.uk>
Date: Mon, 1 Feb 2021 08:36:52 +0000
Subject: [PATCH 07/18] Got to stage where all tests pass or are pending.

---
 app/models/print_job_wrapper.rb       |  33 ++-
 spec/factories/printers.rb            |   7 +
 spec/models/print_job_wrapper_spec.rb | 302 ++++++++++++++++----------
 spec/models/printer_spec.rb           |  35 ++-
 spec/models/squix/print_job.rb        |  38 ----
 spec/models/squix/print_job_spec.rb   |  61 ++++++
 spec/v2/print_jobs_spec.rb            |  14 +-
 7 files changed, 302 insertions(+), 188 deletions(-)
 delete mode 100644 spec/models/squix/print_job.rb
 create mode 100644 spec/models/squix/print_job_spec.rb

diff --git a/app/models/print_job_wrapper.rb b/app/models/print_job_wrapper.rb
index 814b5d9..55949ed 100644
--- a/app/models/print_job_wrapper.rb
+++ b/app/models/print_job_wrapper.rb
@@ -1,13 +1,18 @@
 # frozen_string_literal: true
 
 # PrintJobWrapper
+
+# What did I do:
+# 
 class PrintJobWrapper
   include ActiveModel::Model
 
   attr_reader :copies
-  attr_accessor :printer_name, :label_template_name, :label_template_id, :labels
+  attr_accessor :printer_name, :label_template_name, :labels
+
+  validates :label_template, :printer, :labels, presence: true
 
-  validate :check_printer
+  validate :check_print_job
 
   def print
     return false unless valid?
@@ -42,13 +47,10 @@ def squix_print_job
   def print_job_body
     case printer.printer_type
     when 'toshiba'
-      unless label_template_id
-        @label_template_id = LabelTemplate.find_by(name: label_template_name).id
-      end
-      @print_job_body = { printer_name: printer_name, label_template_id: label_template_id,
+      @print_job_body = { printer_name: printer_name, label_template_id: label_template.id,
                           labels: labels }
     when 'squix'
-      @print_job_body = { printer_name: printer_name, label_template_name: label_template_name,
+      @print_job_body = { printer_name: printer_name, label_template_name: label_template.name,
                           labels: labels, copies: copies }
     end
   end
@@ -57,13 +59,24 @@ def printer
     Printer.find_by(name: printer_name)
   end
 
+  def label_template
+    LabelTemplate.find_by(name: label_template_name)
+  end
+
   def copies=(copies)
-    @copies = copies.to_i
+    @copies = copies.try(:to_i)
+  end
+
+  def copies
+    @copies ||= 1
   end
 
   private
 
-  def check_printer
-    errors.add(:printer, 'does not exist') if printer.blank?
+  def check_print_job
   end
+
+  # def check_printer
+  #   errors.add(:printer, 'does not exist') if printer.blank?
+  # end
 end
diff --git a/spec/factories/printers.rb b/spec/factories/printers.rb
index 23d7840..d26793c 100644
--- a/spec/factories/printers.rb
+++ b/spec/factories/printers.rb
@@ -4,5 +4,12 @@
   factory :printer do
     sequence(:name) { |n| "Printer #{n}" }
     printer_type    { :toshiba }
+
+    # maybe not necessary but more descriptive
+    factory :toshiba_printer
+
+    factory :squix_printer do
+      printer_type    { :squix }
+    end
   end
 end
diff --git a/spec/models/print_job_wrapper_spec.rb b/spec/models/print_job_wrapper_spec.rb
index 8f606c6..11bd766 100644
--- a/spec/models/print_job_wrapper_spec.rb
+++ b/spec/models/print_job_wrapper_spec.rb
@@ -2,145 +2,221 @@
 
 require 'rails_helper'
 
+# What I did:
+# used the printer type factories
+# renamed print_job_wrapper to print_job_wrapper feels better semantically
+# added tests to check validity of print_job_wrapper
+# modified tests which seem to be mocking out the print jobs as we need to check the validity of the print_jobs themselves.
+# changed labels to what should be expected
+# changed assert_equal to rspec expect syntax
+# took copies out of everything except first test
+# did a bit of stuff with copies as it should not be required
+# changed copies to a different number as from what I understand it defaults to 1
 RSpec.describe PrintJobWrapper do
 
-  let!(:squix_printer)       { create(:printer, printer_type: :squix) }
-  let!(:toshiba_printer)     { create(:printer, printer_type: :toshiba) }
+  let!(:squix_printer)      { create(:squix_printer) }
+  let!(:toshiba_printer)    { create(:toshiba_printer) }
   let(:label_template)      { create(:label_template) }
-  let(:labels)              { [{ 'label' => { 'test_attr' => 'test', 'barcode' => '12345' } }] }
-  let(:copies)              { '1' }
+  let(:labels)              { [{ 'test_attr' => 'test', 'barcode' => '12345' }] }
+  let(:copies)              { 2 }
+  let(:attributes)          { { printer_name: toshiba_printer.name, label_template_name: label_template.name, labels: labels, copies: copies } }
 
-  describe 'init' do
+  describe 'new' do
     it 'has the correct instance variables' do
-      pjw = PrintJobWrapper.new({ printer_name: toshiba_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies })
+      print_job_wrapper = PrintJobWrapper.new(attributes)
 
-      assert_equal toshiba_printer.name, pjw.printer_name
-      assert_equal label_template.id, pjw.label_template_id
-      assert_equal label_template.name, pjw.label_template_name
-      assert_equal labels, pjw.labels
-      assert_equal copies.to_i, pjw.copies
+      expect(print_job_wrapper.printer).to eq(toshiba_printer) 
+      expect(print_job_wrapper.label_template).to eq(label_template)
+      expect(print_job_wrapper.labels).to eq(labels)
+      expect(print_job_wrapper.copies).to eq(copies) 
     end
-  end
-
-  describe '#print_job' do
-    it 'sets the print job for a Toshiba printer' do
-      body = { printer_name: toshiba_printer.name, label_template_id: label_template.id, labels: labels }
-      pjw = PrintJobWrapper.new(body)
-      toshiba_print_job = LabelPrinter::PrintJob.build(body)
 
-      allow(pjw).to receive(:toshiba_print_job).and_return(toshiba_print_job)
-      expect(pjw.print_job).to eq pjw.toshiba_print_job
+    it 'will be valid with the correct instance variables' do
+      expect(PrintJobWrapper.new(attributes)).to be_valid
     end
 
-    it 'sets the print job for a Squix printer' do
-      body = { printer_name: squix_printer.name, label_template_name: label_template.name, labels: labels, copies: copies }
-      pjw = PrintJobWrapper.new(body)
-      squix_print_job = Squix::PrintJob.new(body)
-
-      allow(pjw).to receive(:squix_print_job).and_return(squix_print_job)
-      expect(pjw.print_job).to eq pjw.squix_print_job
+    it 'is not valid without a printer name' do
+      expect(PrintJobWrapper.new(attributes.except(:printer_name))).to_not be_valid
     end
-  end
 
-  describe '#print' do
-    context 'when printer_type is Toshiba' do
-      it 'validates the print job and calls execute when valid' do
-        body = { printer_name: toshiba_printer.name, label_template_id: label_template.id, labels: labels }
+    it 'is not valid without a label template name' do
+      expect(PrintJobWrapper.new(attributes.except(:label_template_name))).to_not be_valid
+    end
 
-        pjw = PrintJobWrapper.new(body)
-        toshiba_print_job = LabelPrinter::PrintJob.build(body)
+    it 'is not valid without some labels' do
+      expect(PrintJobWrapper.new(attributes.except(:labels))).to_not be_valid
+    end
 
-        allow(pjw).to receive(:toshiba_print_job).and_return(toshiba_print_job)
-        allow(toshiba_print_job).to receive(:execute)
+    it 'will be valid if copies is not passed' do
+      print_job_wrapper = PrintJobWrapper.new(attributes.except(:copies))
+      expect(print_job_wrapper).to be_valid
+      expect(print_job_wrapper.copies).to be_present
+    end
+  end
 
-        pjw.print
-        expect(toshiba_print_job.valid?).to eq true
+  describe '#print_job' do
+    
+    context 'Toshiba' do
+
+      let(:toshiba_labels) { label_template.dummy_labels.to_h }
+      
+      xit 'creates a new print job of the correct type' do
+        print_job_wrapper = PrintJobWrapper.new(attributes.merge(labels: toshiba_labels))
+        expect(print_job_wrapper.print_job).to be_instance_of(LabelPrinter::PrintJob::LPD)
       end
 
-      it 'validates the print job and returns false when invalid' do
-        body_missing_attribute = { printer_name: toshiba_printer.name, labels: labels }
-
-        pjw = PrintJobWrapper.new(body_missing_attribute)
-        toshiba_print_job = LabelPrinter::PrintJob.build(body_missing_attribute)
-
-        allow(pjw).to receive(:toshiba_print_job).and_return(toshiba_print_job)
-        allow(toshiba_print_job).to receive(:execute)
+      xit 'is not valid if the print job is not valid' do
+        print_job_wrapper = PrintJobWrapper.new(attributes.except(:labels))
+        expect(print_job_wrapper.print_job).to_not be_valid
+      end
 
-        pjw.print
-        expect(toshiba_print_job.valid?).to eq false
-        expect(toshiba_print_job.errors.messages).to eq({ label_template: ["does not exist"] })
+      xit 'will execute the print job if it is valid' do
+        print_job_wrapper = PrintJobWrapper.new(attributes.merge(printer_name: squix_printer.name))
+        allow(print_job_wrapper.print_job).to receive(:execute).and_return(true)
+        expect(print_job_wrapper.execute).to be_truthy
+        expect(print_job_wrapper.print_job).to receive(:execute)
       end
     end
 
-    context 'when printer_type is Squix' do
-      it 'validates the print job and calls execute when valid' do
-        body = { printer_name: squix_printer.name, label_template_name: label_template.name, labels: labels, copies: copies }
-        pjw = PrintJobWrapper.new(body)
-        squix_print_job = Squix::PrintJob.new(body)
-
-        allow(pjw).to receive(:squix_print_job).and_return(squix_print_job)
-        allow(squix_print_job).to receive(:execute)
-
-        pjw.print
-        expect(squix_print_job.valid?).to eq true
+    context 'Squix' do
+      xit 'creates a new print job of the correct type' do
+        print_job_wrapper = PrintJobWrapper.new(attributes.merge(printer_name: squix_printer.name))
+        expect(print_job_wrapper.print_job).to be_instance_of(Squix::PrintJob)
       end
 
-      it 'validates the print job and returns false when invalid' do
-        body_missing_attribute = { printer_name: squix_printer.name, labels: labels, copies: copies }
-        pjw = PrintJobWrapper.new(body_missing_attribute)
-        squix_print_job = Squix::PrintJob.new(body_missing_attribute)
-
-        allow(pjw).to receive(:squix_print_job).and_return(squix_print_job)
-        allow(squix_print_job).to receive(:execute)
-
-        pjw.print
-        expect(squix_print_job.valid?).to eq false
-        expect(squix_print_job.errors.messages).to eq({ label_template_name: ["can't be blank"] })
+      xit 'is not valid if the print job is not valid' do
+        print_job_wrapper = PrintJobWrapper.new(attributes.except(:labels))
+        expect(print_job_wrapper.print_job).to_not be_valid
       end
-    end
-  end
-
-  describe '#toshiba_print_job' do
-    it 'builds a LabelPrinter::PrintJob when label_template_id is provided' do
-      body = { printer_name: toshiba_printer.name, label_template_id: label_template.id, labels: labels }
-      pjw = PrintJobWrapper.new(body)
-      expect(pjw.toshiba_print_job).to be_instance_of(LabelPrinter::PrintJob::LPD)
-    end
-
-    it 'builds a LabelPrinter::PrintJob when label_template_name is provided' do
-      body = { printer_name: toshiba_printer.name, label_template_name: label_template.name, labels: labels }
-      pjw = PrintJobWrapper.new(body)
-      expect(pjw.toshiba_print_job).to be_instance_of(LabelPrinter::PrintJob::LPD)
-    end
-  end
 
-  describe '#squix_print_job' do
-    it 'builds a Squix::PrintJob' do
-      body = { printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies }
-      pjw = PrintJobWrapper.new(body)
-      expect(pjw.squix_print_job).to be_instance_of(Squix::PrintJob)
-    end
-  end
-
-  describe '#print_job_body' do
-    it 'sets the print_job_body for a Toshiba printer' do
-      body = { printer_name: toshiba_printer.name, label_template_name: label_template.name, labels: labels }
-      pjw = PrintJobWrapper.new(body)
-      expect(pjw.print_job_body.keys).to eq [:printer_name, :label_template_id, :labels]
-    end
-
-    it 'sets the print_job_body for a Squix printer' do
-      body = { printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies }
-      pjw = PrintJobWrapper.new(body)
-      expect(pjw.print_job_body.keys).to eq [:printer_name, :label_template_name, :labels, :copies]
+      xit 'will execute the print job if it is valid' do
+        print_job_wrapper = PrintJobWrapper.new(attributes.merge(printer_name: squix_printer.name))
+        allow(print_job_wrapper.print_job).to receive(:execute).and_return(true)
+        expect(print_job_wrapper.execute).to be_truthy
+        expect(print_job_wrapper.print_job).to receive(:execute)
+      end
     end
   end
 
-  describe '#printer' do
-    it 'returns the correct printer' do
-      body = { printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels, copies: copies }
-      pjw = PrintJobWrapper.new(body)
-      expect(pjw.printer).to eq squix_printer
-    end
-  end
+  # describe '#print_job' do
+  #   it 'sets the print job for a Toshiba printer' do
+  #     body = { printer_name: toshiba_printer.name, label_template_id: label_template.id, labels: labels }
+  #     print_job_wrapper = PrintJobWrapper.new(body)
+  #     toshiba_print_job = LabelPrinter::PrintJob.build(body)
+
+  #     allow(print_job_wrapper).to receive(:toshiba_print_job).and_return(toshiba_print_job)
+  #     expect(print_job_wrapper.print_job).to eq print_job_wrapper.toshiba_print_job
+  #   end
+
+  #   it 'sets the print job for a Squix printer' do
+  #     body = { printer_name: squix_printer.name, label_template_name: label_template.name, labels: labels }
+  #     print_job_wrapper = PrintJobWrapper.new(body)
+  #     squix_print_job = Squix::PrintJob.new(body)
+
+  #     allow(print_job_wrapper).to receive(:squix_print_job).and_return(squix_print_job)
+  #     expect(print_job_wrapper.print_job).to eq print_job_wrapper.squix_print_job
+  #   end
+  # end
+
+  # describe '#print' do
+  #   context 'when printer_type is Toshiba' do
+  #     it 'validates the print job and calls execute when valid' do
+  #       body = { printer_name: toshiba_printer.name, label_template_id: label_template.id, labels: labels }
+
+  #       print_job_wrapper = PrintJobWrapper.new(body)
+  #       toshiba_print_job = LabelPrinter::PrintJob.build(body)
+
+  #       allow(print_job_wrapper).to receive(:toshiba_print_job).and_return(toshiba_print_job)
+  #       allow(toshiba_print_job).to receive(:execute)
+
+  #       print_job_wrapper.print
+  #       expect(toshiba_print_job.valid?).to eq true
+  #     end
+
+  #     it 'validates the print job and returns false when invalid' do
+  #       body_missing_attribute = { printer_name: toshiba_printer.name, labels: labels }
+
+  #       print_job_wrapper = PrintJobWrapper.new(body_missing_attribute)
+  #       toshiba_print_job = LabelPrinter::PrintJob.build(body_missing_attribute)
+
+  #       allow(print_job_wrapper).to receive(:toshiba_print_job).and_return(toshiba_print_job)
+  #       allow(toshiba_print_job).to receive(:execute)
+
+  #       print_job_wrapper.print
+  #       expect(toshiba_print_job.valid?).to eq false
+  #       expect(toshiba_print_job.errors.messages).to eq({ label_template: ["does not exist"] })
+  #     end
+  #   end
+
+  #   context 'when printer_type is Squix' do
+  #     it 'validates the print job and calls execute when valid' do
+  #       body = { printer_name: squix_printer.name, label_template_name: label_template.name, labels: labels }
+  #       print_job_wrapper = PrintJobWrapper.new(body)
+  #       squix_print_job = Squix::PrintJob.new(body)
+
+  #       allow(print_job_wrapper).to receive(:squix_print_job).and_return(squix_print_job)
+  #       allow(squix_print_job).to receive(:execute)
+
+  #       print_job_wrapper.print
+  #       expect(squix_print_job.valid?).to eq true
+  #     end
+
+  #     it 'validates the print job and returns false when invalid' do
+  #       body_missing_attribute = { printer_name: squix_printer.name, labels: labels }
+  #       print_job_wrapper = PrintJobWrapper.new(body_missing_attribute)
+  #       squix_print_job = Squix::PrintJob.new(body_missing_attribute)
+
+  #       allow(print_job_wrapper).to receive(:squix_print_job).and_return(squix_print_job)
+  #       allow(squix_print_job).to receive(:execute)
+
+  #       print_job_wrapper.print
+  #       expect(squix_print_job.valid?).to eq false
+  #       expect(squix_print_job.errors.messages).to eq({ label_template_name: ["can't be blank"] })
+  #     end
+  #   end
+  # end
+
+  # describe '#toshiba_print_job' do
+  #   it 'builds a LabelPrinter::PrintJob when label_template_id is provided' do
+  #     body = { printer_name: toshiba_printer.name, label_template_id: label_template.id, labels: labels }
+  #     print_job_wrapper = PrintJobWrapper.new(body)
+  #     expect(print_job_wrapper.toshiba_print_job).to be_instance_of(LabelPrinter::PrintJob::LPD)
+  #   end
+
+  #   it 'builds a LabelPrinter::PrintJob when label_template_name is provided' do
+  #     body = { printer_name: toshiba_printer.name, label_template_name: label_template.name, labels: labels }
+  #     print_job_wrapper = PrintJobWrapper.new(body)
+  #     expect(print_job_wrapper.toshiba_print_job).to be_instance_of(LabelPrinter::PrintJob::LPD)
+  #   end
+  # end
+
+  # describe '#squix_print_job' do
+  #   it 'builds a Squix::PrintJob' do
+  #     body = { printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels }
+  #     print_job_wrapper = PrintJobWrapper.new(body)
+  #     expect(print_job_wrapper.squix_print_job).to be_instance_of(Squix::PrintJob)
+  #   end
+  # end
+
+  # describe '#print_job_body' do
+  #   it 'sets the print_job_body for a Toshiba printer' do
+  #     body = { printer_name: toshiba_printer.name, label_template_name: label_template.name, labels: labels }
+  #     print_job_wrapper = PrintJobWrapper.new(body)
+  #     expect(print_job_wrapper.print_job_body.keys).to eq [:printer_name, :label_template_id, :labels]
+  #   end
+
+  #   it 'sets the print_job_body for a Squix printer' do
+  #     body = { printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels }
+  #     print_job_wrapper = PrintJobWrapper.new(body)
+  #     expect(print_job_wrapper.print_job_body.keys).to eq [:printer_name, :label_template_name, :labels, :copies]
+  #   end
+  # end
+
+  # describe '#printer' do
+  #   it 'returns the correct printer' do
+  #     body = { printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels }
+  #     print_job_wrapper = PrintJobWrapper.new(body)
+  #     expect(print_job_wrapper.printer).to eq squix_printer
+  #   end
+  # end
 end
diff --git a/spec/models/printer_spec.rb b/spec/models/printer_spec.rb
index fb7507f..0574e42 100644
--- a/spec/models/printer_spec.rb
+++ b/spec/models/printer_spec.rb
@@ -2,6 +2,12 @@
 
 require 'rails_helper'
 
+# what I did:
+# removed extra tests that were testing the same thing twice
+# renamed some tests to reflect what they were testing
+# checked printer type values against their enums rather than using strings
+# checked validity of printer types within creating printer
+# added more descriptive factories for different printer types
 RSpec.describe Printer, type: :model do
   it 'should not be valid without a name' do
     expect(build(:printer, name: nil)).to_not be_valid
@@ -20,33 +26,22 @@
   end
 
   describe 'printer_type' do
-    it 'should not be valid without a name' do
+    it 'should not be valid without a type' do
       expect(build(:printer, printer_type: nil)).to_not be_valid
     end
 
-    it 'should create a printer with the squix printer_type' do
-      printer = create(:printer, printer_type: :squix)
-      expect(printer.printer_type).to eq 'squix'
-      expect(printer.squix?).to be_truthy
+    it 'can create a printer with the squix printer_type' do
+      printer = create(:squix_printer)
+      expect(printer).to be_valid
+      expect(printer).to be_squix
     end
 
-    it 'should create a printer with the toshiba printer_type' do
-      printer = create(:printer, printer_type: :toshiba)
-      expect(printer.printer_type).to eq 'toshiba'
-      expect(printer.toshiba?).to be_truthy
+    it 'can create a printer with the toshiba printer_type' do
+      printer = create(:toshiba_printer)
+      expect(printer).to be_valid
+      expect(printer).to be_toshiba
     end
 
-    it 'should not be valid without a printer_type' do
-      expect(build(:printer, name: nil)).to_not be_valid
-    end
-
-    it 'should be valid with a printer_type' do
-      expect(build(:printer, printer_type: :squix)).to be_valid
-    end
-
-    it 'should be valid with a printer_type' do
-      expect(build(:printer, printer_type: :toshiba)).to be_valid
-    end
   end
 
 end
diff --git a/spec/models/squix/print_job.rb b/spec/models/squix/print_job.rb
deleted file mode 100644
index 6795589..0000000
--- a/spec/models/squix/print_job.rb
+++ /dev/null
@@ -1,38 +0,0 @@
-# frozen_string_literal: true
-
-require 'rails_helper'
-
-RSpec.describe Squix::PrintJob do
-
-  let!(:printer)          { create(:printer) }
-  let(:label_template)      { create(:label_template) }
-  let(:labels)              { [{ 'label' => { 'test_attr' => 'test', 'barcode' => '12345' } }] }
-  let(:copies)   { 1 }
-
-  describe 'init' do
-    it 'has the correct insatnce variables' do
-      pj = Squix::PrintJob.new(printer_name: printer.name, label_template_name: label_template.name, labels: labels, copies: copies)
-
-      assert_equal printer.name, pj.printer_name
-      assert_equal label_template.name, pj.label_template_name
-      assert_equal labels, pj.labels
-      assert_equal copies, pj.copies
-    end
-  end
-
-  describe '#execute' do
-    it 'sends a print request' do
-      pj = Squix::PrintJob.new(printer_name: printer.name, label_template_name: label_template.name, labels: labels, copies: copies)
-      expect(SPrintClient).to receive(:send_print_request).with(pj.printer_name, pj.label_template_name, pj.merge_fields_list)
-      pj.execute
-    end
-  end
-
-  describe '#merge_fields_list' do
-    it 'returns the same when copies is 1' do
-      pj = Squix::PrintJob.new(printer_name: printer.name, label_template_name: label_template.name, labels: labels, copies: 2)
-      expect(pj.merge_fields_list).to eq [{ 'label' => { 'test_attr' => 'test', 'barcode' => '12345' } }, { 'label' => { 'test_attr' => 'test', 'barcode' => '12345' } }]
-    end
-  end
-
-end
diff --git a/spec/models/squix/print_job_spec.rb b/spec/models/squix/print_job_spec.rb
new file mode 100644
index 0000000..f19efd8
--- /dev/null
+++ b/spec/models/squix/print_job_spec.rb
@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+# what I did:
+# Renamed the file as it did not have spec at the end. This means tests will not be run when the bundle exec rspec
+# Renamed pj to print_job
+# Modified setup so that you can reuse attributes
+# Added tests to check that print job was valid
+# modified label setup as it didnt seem to be setup to what sprint client was expecting
+RSpec.describe Squix::PrintJob do
+
+  let!(:printer)            { create(:printer) }
+  let(:label_template)      { create(:label_template) }
+  let(:label)               { { 'test_attr' => 'test', 'barcode' => '12345' } }
+  let(:labels)              { [label] }
+  let(:copies)              { 1 }
+  let(:attributes)          { { printer_name: printer.name, label_template_name: label_template.name, labels: [label], copies: copies } }
+
+  describe 'new' do
+    it 'will have the correct instance variables' do
+      print_job = Squix::PrintJob.new(attributes)
+
+      expect(print_job.printer_name).to eq(printer.name) 
+      expect(print_job.label_template_name).to eq(label_template.name) 
+      expect(print_job.labels).to eq(labels) 
+      expect(print_job.copies).to eq(copies) 
+    end
+
+    it 'will not be valid without the printer name' do
+      print_job = Squix::PrintJob.new(attributes.except(:printer_name))
+      expect(print_job).to_not be_valid
+    end
+
+    it 'will not be valid without the label_template_name' do
+      print_job = Squix::PrintJob.new(attributes.except(:label_template_name))
+      expect(print_job).to_not be_valid
+    end
+
+    it 'will not be valid without the labels' do
+      print_job = Squix::PrintJob.new(attributes.except(:labels))
+      expect(print_job).to_not be_valid
+    end
+  end
+
+  describe '#execute' do
+    it 'sends a print request' do
+      print_job = Squix::PrintJob.new(attributes)
+      expect(SPrintClient).to receive(:send_print_request).with(print_job.printer_name, print_job.label_template_name, print_job.merge_fields_list)
+      print_job.execute
+    end
+  end
+
+  describe '#merge_fields_list' do
+    it 'will return the correct number of merge fields when copies is more than 1' do
+      print_job = Squix::PrintJob.new(attributes.merge(copies: 2))
+      expect(print_job.merge_fields_list).to eq([label] * 2)
+    end
+  end
+
+end
diff --git a/spec/v2/print_jobs_spec.rb b/spec/v2/print_jobs_spec.rb
index 58f8ef2..6757ad6 100644
--- a/spec/v2/print_jobs_spec.rb
+++ b/spec/v2/print_jobs_spec.rb
@@ -27,7 +27,7 @@
 
     context 'When printer type is Toshiba' do
       it 'should send_print_request to SPrintClient ' do
-        body = { printer_name: toshiba_printer.name, label_template_id: label_template.id,
+        body = { printer_name: toshiba_printer.name, label_template_name: label_template.name,
         labels: labels }
         allow_any_instance_of(LabelPrinter::PrintJob::LPD).to receive(:execute).and_return(true)
 
@@ -43,7 +43,7 @@
   describe 'On failure' do
     context 'when wrapper is not valid' do
       it 'should return an error if the printer name is missing' do
-        body = { printer_name: '', label_template_id: label_template.id, labels: labels }
+        body = { printer_name: '', label_template_name: label_template.name, labels: labels }
 
         post v2_print_jobs_path, params: { print_job: body }
         expect(response).to have_http_status(:unprocessable_entity)
@@ -51,13 +51,13 @@
         json = ActiveSupport::JSON.decode(response.body)
 
         expect(json['errors']).not_to be_empty
-        expect(find_attribute_error_details(json, 'printer')).to include('does not exist')
+        expect(find_attribute_error_details(json, 'printer')).to include("can't be blank")
       end
     end
 
     context 'when toshiba_print_job is not valid' do
       it 'should return an error' do
-        body = { printer_name: toshiba_printer.name, label_template_id: '', labels: labels }
+        body = { printer_name: toshiba_printer.name, label_template_name: '', labels: labels }
 
         post v2_print_jobs_path, params: { print_job: body }
         expect(response).to have_http_status(:unprocessable_entity)
@@ -65,13 +65,13 @@
         json = ActiveSupport::JSON.decode(response.body)
 
         expect(json['errors']).not_to be_empty
-        expect(find_attribute_error_details(json, 'label_template')).to include('does not exist')
+        expect(find_attribute_error_details(json, 'label_template')).to include("can't be blank")
       end
     end
 
     context 'when squix_print_job is not valid' do
       it 'should return an error' do
-        body = { printer_name: squix_printer.name, label_template_name: label_template.name, labels: labels }
+        body = { printer_name: squix_printer.name, label_template_name: '', labels: labels }
 
         post v2_print_jobs_path, params: { print_job: body }
         expect(response).to have_http_status(:unprocessable_entity)
@@ -79,7 +79,7 @@
         json = ActiveSupport::JSON.decode(response.body)
 
         expect(json['errors']).not_to be_empty
-        expect(find_attribute_error_details(json, 'copies')).to include("can't be blank")
+        expect(find_attribute_error_details(json, 'label_template')).to include("can't be blank")
       end
     end
   end

From b663b150d0a18338d4a047fc46309fe2384ea7ee Mon Sep 17 00:00:00 2001
From: Stephen Inglis <si5@sanger.ac.uk>
Date: Mon, 1 Feb 2021 08:37:55 +0000
Subject: [PATCH 08/18] Rubocopped.

---
 app/models/print_job_wrapper.rb | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/app/models/print_job_wrapper.rb b/app/models/print_job_wrapper.rb
index 55949ed..aec5e85 100644
--- a/app/models/print_job_wrapper.rb
+++ b/app/models/print_job_wrapper.rb
@@ -3,11 +3,10 @@
 # PrintJobWrapper
 
 # What did I do:
-# 
+#
 class PrintJobWrapper
   include ActiveModel::Model
 
-  attr_reader :copies
   attr_accessor :printer_name, :label_template_name, :labels
 
   validates :label_template, :printer, :labels, presence: true
@@ -73,8 +72,7 @@ def copies
 
   private
 
-  def check_print_job
-  end
+  def check_print_job; end
 
   # def check_printer
   #   errors.add(:printer, 'does not exist') if printer.blank?

From 1a90f0c68d28c2e60ccf98ee3c08da72d1c8fb71 Mon Sep 17 00:00:00 2001
From: Stephen Inglis <si5@sanger.ac.uk>
Date: Mon, 1 Feb 2021 09:26:23 +0000
Subject: [PATCH 09/18] Got print job wrapper tests working with correct print
 jobs.

---
 app/models/print_job_wrapper.rb       |  51 ++++-----
 spec/models/print_job_wrapper_spec.rb | 147 +++-----------------------
 2 files changed, 33 insertions(+), 165 deletions(-)

diff --git a/app/models/print_job_wrapper.rb b/app/models/print_job_wrapper.rb
index aec5e85..a60ad46 100644
--- a/app/models/print_job_wrapper.rb
+++ b/app/models/print_job_wrapper.rb
@@ -16,42 +16,25 @@ class PrintJobWrapper
   def print
     return false unless valid?
 
-    unless print_job.valid?
-      print_job.errors.each do |k, v|
-        errors.add(k, v)
-      end
-      return false
-    end
-
     print_job.execute
   end
 
   def print_job
-    case printer.printer_type
-    when 'toshiba'
-      @print_job ||= toshiba_print_job
-    when 'squix'
-      @print_job ||= squix_print_job
-    end
-  end
-
-  def toshiba_print_job
-    LabelPrinter::PrintJob.build(print_job_body)
-  end
-
-  def squix_print_job
-    Squix::PrintJob.new(print_job_body)
+    @print_job ||= case printer.try(:printer_type)
+                   when 'toshiba'
+                     LabelPrinter::PrintJob.build(print_job_body.except(
+                                                    :label_template_name, :copies
+                                                  ))
+                   when 'squix'
+                     Squix::PrintJob.new(print_job_body.except(:label_template_id))
+                   end
   end
 
   def print_job_body
-    case printer.printer_type
-    when 'toshiba'
-      @print_job_body = { printer_name: printer_name, label_template_id: label_template.id,
-                          labels: labels }
-    when 'squix'
-      @print_job_body = { printer_name: printer_name, label_template_name: label_template.name,
+    @print_job_body ||= { printer_name: printer_name, label_template_id:
+                          label_template.try(:id),
+                          label_template_name: label_template.try(:name),
                           labels: labels, copies: copies }
-    end
   end
 
   def printer
@@ -72,9 +55,13 @@ def copies
 
   private
 
-  def check_print_job; end
+  # TODO: we may well be double checking everything
+  def check_print_job
+    return if print_job.blank?
+    return if print_job.valid?
 
-  # def check_printer
-  #   errors.add(:printer, 'does not exist') if printer.blank?
-  # end
+    print_job.errors.each do |k, v|
+      errors.add(k, v)
+    end
+  end
 end
diff --git a/spec/models/print_job_wrapper_spec.rb b/spec/models/print_job_wrapper_spec.rb
index 11bd766..9553e33 100644
--- a/spec/models/print_job_wrapper_spec.rb
+++ b/spec/models/print_job_wrapper_spec.rb
@@ -58,165 +58,46 @@
     
     context 'Toshiba' do
 
+      # TODO: at the moment we are passing in the labels as the correct format for Toshiba
+      # They need to be in the same format for both
       let(:toshiba_labels) { label_template.dummy_labels.to_h }
       
-      xit 'creates a new print job of the correct type' do
+      it 'creates a new print job of the correct type' do
         print_job_wrapper = PrintJobWrapper.new(attributes.merge(labels: toshiba_labels))
         expect(print_job_wrapper.print_job).to be_instance_of(LabelPrinter::PrintJob::LPD)
       end
 
-      xit 'is not valid if the print job is not valid' do
+      it 'is not valid if the print job is not valid' do
         print_job_wrapper = PrintJobWrapper.new(attributes.except(:labels))
-        expect(print_job_wrapper.print_job).to_not be_valid
+        expect(print_job_wrapper).to_not be_valid
       end
 
-      xit 'will execute the print job if it is valid' do
-        print_job_wrapper = PrintJobWrapper.new(attributes.merge(printer_name: squix_printer.name))
+      it 'will execute the print job if it is valid' do
+        print_job_wrapper = PrintJobWrapper.new(attributes)
         allow(print_job_wrapper.print_job).to receive(:execute).and_return(true)
-        expect(print_job_wrapper.execute).to be_truthy
-        expect(print_job_wrapper.print_job).to receive(:execute)
+        expect(print_job_wrapper).to be_valid
+        expect(print_job_wrapper.print).to be_truthy
       end
     end
 
     context 'Squix' do
-      xit 'creates a new print job of the correct type' do
+      it 'creates a new print job of the correct type' do
         print_job_wrapper = PrintJobWrapper.new(attributes.merge(printer_name: squix_printer.name))
         expect(print_job_wrapper.print_job).to be_instance_of(Squix::PrintJob)
       end
 
-      xit 'is not valid if the print job is not valid' do
+      it 'is not valid if the print job is not valid' do
         print_job_wrapper = PrintJobWrapper.new(attributes.except(:labels))
         expect(print_job_wrapper.print_job).to_not be_valid
       end
 
-      xit 'will execute the print job if it is valid' do
+      it 'will execute the print job if it is valid' do
         print_job_wrapper = PrintJobWrapper.new(attributes.merge(printer_name: squix_printer.name))
         allow(print_job_wrapper.print_job).to receive(:execute).and_return(true)
-        expect(print_job_wrapper.execute).to be_truthy
-        expect(print_job_wrapper.print_job).to receive(:execute)
+        expect(print_job_wrapper).to be_valid
+        expect(print_job_wrapper.print).to be_truthy
       end
     end
   end
 
-  # describe '#print_job' do
-  #   it 'sets the print job for a Toshiba printer' do
-  #     body = { printer_name: toshiba_printer.name, label_template_id: label_template.id, labels: labels }
-  #     print_job_wrapper = PrintJobWrapper.new(body)
-  #     toshiba_print_job = LabelPrinter::PrintJob.build(body)
-
-  #     allow(print_job_wrapper).to receive(:toshiba_print_job).and_return(toshiba_print_job)
-  #     expect(print_job_wrapper.print_job).to eq print_job_wrapper.toshiba_print_job
-  #   end
-
-  #   it 'sets the print job for a Squix printer' do
-  #     body = { printer_name: squix_printer.name, label_template_name: label_template.name, labels: labels }
-  #     print_job_wrapper = PrintJobWrapper.new(body)
-  #     squix_print_job = Squix::PrintJob.new(body)
-
-  #     allow(print_job_wrapper).to receive(:squix_print_job).and_return(squix_print_job)
-  #     expect(print_job_wrapper.print_job).to eq print_job_wrapper.squix_print_job
-  #   end
-  # end
-
-  # describe '#print' do
-  #   context 'when printer_type is Toshiba' do
-  #     it 'validates the print job and calls execute when valid' do
-  #       body = { printer_name: toshiba_printer.name, label_template_id: label_template.id, labels: labels }
-
-  #       print_job_wrapper = PrintJobWrapper.new(body)
-  #       toshiba_print_job = LabelPrinter::PrintJob.build(body)
-
-  #       allow(print_job_wrapper).to receive(:toshiba_print_job).and_return(toshiba_print_job)
-  #       allow(toshiba_print_job).to receive(:execute)
-
-  #       print_job_wrapper.print
-  #       expect(toshiba_print_job.valid?).to eq true
-  #     end
-
-  #     it 'validates the print job and returns false when invalid' do
-  #       body_missing_attribute = { printer_name: toshiba_printer.name, labels: labels }
-
-  #       print_job_wrapper = PrintJobWrapper.new(body_missing_attribute)
-  #       toshiba_print_job = LabelPrinter::PrintJob.build(body_missing_attribute)
-
-  #       allow(print_job_wrapper).to receive(:toshiba_print_job).and_return(toshiba_print_job)
-  #       allow(toshiba_print_job).to receive(:execute)
-
-  #       print_job_wrapper.print
-  #       expect(toshiba_print_job.valid?).to eq false
-  #       expect(toshiba_print_job.errors.messages).to eq({ label_template: ["does not exist"] })
-  #     end
-  #   end
-
-  #   context 'when printer_type is Squix' do
-  #     it 'validates the print job and calls execute when valid' do
-  #       body = { printer_name: squix_printer.name, label_template_name: label_template.name, labels: labels }
-  #       print_job_wrapper = PrintJobWrapper.new(body)
-  #       squix_print_job = Squix::PrintJob.new(body)
-
-  #       allow(print_job_wrapper).to receive(:squix_print_job).and_return(squix_print_job)
-  #       allow(squix_print_job).to receive(:execute)
-
-  #       print_job_wrapper.print
-  #       expect(squix_print_job.valid?).to eq true
-  #     end
-
-  #     it 'validates the print job and returns false when invalid' do
-  #       body_missing_attribute = { printer_name: squix_printer.name, labels: labels }
-  #       print_job_wrapper = PrintJobWrapper.new(body_missing_attribute)
-  #       squix_print_job = Squix::PrintJob.new(body_missing_attribute)
-
-  #       allow(print_job_wrapper).to receive(:squix_print_job).and_return(squix_print_job)
-  #       allow(squix_print_job).to receive(:execute)
-
-  #       print_job_wrapper.print
-  #       expect(squix_print_job.valid?).to eq false
-  #       expect(squix_print_job.errors.messages).to eq({ label_template_name: ["can't be blank"] })
-  #     end
-  #   end
-  # end
-
-  # describe '#toshiba_print_job' do
-  #   it 'builds a LabelPrinter::PrintJob when label_template_id is provided' do
-  #     body = { printer_name: toshiba_printer.name, label_template_id: label_template.id, labels: labels }
-  #     print_job_wrapper = PrintJobWrapper.new(body)
-  #     expect(print_job_wrapper.toshiba_print_job).to be_instance_of(LabelPrinter::PrintJob::LPD)
-  #   end
-
-  #   it 'builds a LabelPrinter::PrintJob when label_template_name is provided' do
-  #     body = { printer_name: toshiba_printer.name, label_template_name: label_template.name, labels: labels }
-  #     print_job_wrapper = PrintJobWrapper.new(body)
-  #     expect(print_job_wrapper.toshiba_print_job).to be_instance_of(LabelPrinter::PrintJob::LPD)
-  #   end
-  # end
-
-  # describe '#squix_print_job' do
-  #   it 'builds a Squix::PrintJob' do
-  #     body = { printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels }
-  #     print_job_wrapper = PrintJobWrapper.new(body)
-  #     expect(print_job_wrapper.squix_print_job).to be_instance_of(Squix::PrintJob)
-  #   end
-  # end
-
-  # describe '#print_job_body' do
-  #   it 'sets the print_job_body for a Toshiba printer' do
-  #     body = { printer_name: toshiba_printer.name, label_template_name: label_template.name, labels: labels }
-  #     print_job_wrapper = PrintJobWrapper.new(body)
-  #     expect(print_job_wrapper.print_job_body.keys).to eq [:printer_name, :label_template_id, :labels]
-  #   end
-
-  #   it 'sets the print_job_body for a Squix printer' do
-  #     body = { printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels }
-  #     print_job_wrapper = PrintJobWrapper.new(body)
-  #     expect(print_job_wrapper.print_job_body.keys).to eq [:printer_name, :label_template_name, :labels, :copies]
-  #   end
-  # end
-
-  # describe '#printer' do
-  #   it 'returns the correct printer' do
-  #     body = { printer_name: squix_printer.name, label_template_name: label_template.name, label_template_id: label_template.id, labels: labels }
-  #     print_job_wrapper = PrintJobWrapper.new(body)
-  #     expect(print_job_wrapper.printer).to eq squix_printer
-  #   end
-  # end
 end

From 494bc9f69c4f1fe88c3b37d20171fe7198b3e20b Mon Sep 17 00:00:00 2001
From: Stephen Inglis <si5@sanger.ac.uk>
Date: Mon, 1 Feb 2021 10:23:07 +0000
Subject: [PATCH 10/18] Updated.

---
 app/controllers/v2/print_jobs_controller.rb | 12 ++++++++++++
 app/models/print_job_wrapper.rb             |  8 +++++++-
 spec/models/print_job_wrapper_spec.rb       |  8 +++++---
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/app/controllers/v2/print_jobs_controller.rb b/app/controllers/v2/print_jobs_controller.rb
index a569535..5c05153 100644
--- a/app/controllers/v2/print_jobs_controller.rb
+++ b/app/controllers/v2/print_jobs_controller.rb
@@ -15,10 +15,22 @@ def create
 
     private
 
+    # TODO: get this working in one go.
+    # def print_job_params
+    #   params.require(:print_job).require(:printer_name, :label_template_name, :copies)
+    #         .tap do |allow_listed|
+    #           allow_listed[:labels] = []
+    #             params[:print_job][:labels].each do |label|
+    #               allow_listed[:labels] << label.permit!.to_h
+    #             end
+    #       end
+    # end
+
     def print_job_params
       p1 = params.permit(
         print_job: %i[
           printer_name
+
           label_template_name
           label_template_id
           copies
diff --git a/app/models/print_job_wrapper.rb b/app/models/print_job_wrapper.rb
index a60ad46..45f4506 100644
--- a/app/models/print_job_wrapper.rb
+++ b/app/models/print_job_wrapper.rb
@@ -3,7 +3,12 @@
 # PrintJobWrapper
 
 # What did I do:
-#
+# added validation for required attributes as well as print job
+# added validation for print job
+# added print job creation in single method
+# added print job body creation method
+# made copies optional and create them if they are not passed
+# created printer and label template as attribute readers
 class PrintJobWrapper
   include ActiveModel::Model
 
@@ -22,6 +27,7 @@ def print
   def print_job
     @print_job ||= case printer.try(:printer_type)
                    when 'toshiba'
+                     # TODO: we need to convert the labels to the correct format
                      LabelPrinter::PrintJob.build(print_job_body.except(
                                                     :label_template_name, :copies
                                                   ))
diff --git a/spec/models/print_job_wrapper_spec.rb b/spec/models/print_job_wrapper_spec.rb
index 9553e33..6f4cab1 100644
--- a/spec/models/print_job_wrapper_spec.rb
+++ b/spec/models/print_job_wrapper_spec.rb
@@ -67,9 +67,10 @@
         expect(print_job_wrapper.print_job).to be_instance_of(LabelPrinter::PrintJob::LPD)
       end
 
-      it 'is not valid if the print job is not valid' do
+      it 'will not execute if the print job is not valid' do
         print_job_wrapper = PrintJobWrapper.new(attributes.except(:labels))
         expect(print_job_wrapper).to_not be_valid
+        expect(print_job_wrapper.print).to be_falsy
       end
 
       it 'will execute the print job if it is valid' do
@@ -86,9 +87,10 @@
         expect(print_job_wrapper.print_job).to be_instance_of(Squix::PrintJob)
       end
 
-      it 'is not valid if the print job is not valid' do
-        print_job_wrapper = PrintJobWrapper.new(attributes.except(:labels))
+      it 'will not execute if the print job is not valid' do
+        print_job_wrapper = PrintJobWrapper.new(attributes.except(:labels).merge(printer_name: squix_printer.name))
         expect(print_job_wrapper.print_job).to_not be_valid
+        expect(print_job_wrapper.print).to be_falsy
       end
 
       it 'will execute the print job if it is valid' do

From d65068185d35728a2f7ab1ea405299932278eb75 Mon Sep 17 00:00:00 2001
From: Harriet Craven <hc6@sanger.ac.uk>
Date: Mon, 1 Feb 2021 17:24:04 +0000
Subject: [PATCH 11/18] Added v2 label conversion

---
 app/label_printer/label_printer/print_job.rb | 19 +++++++++++++++
 app/models/print_job_wrapper.rb              | 18 +++++++-------
 spec/label_printer/print_job_spec.rb         | 25 ++++++++++++++++++--
 spec/models/print_job_wrapper_spec.rb        | 13 ++++------
 4 files changed, 56 insertions(+), 19 deletions(-)

diff --git a/app/label_printer/label_printer/print_job.rb b/app/label_printer/label_printer/print_job.rb
index 464e026..ebee531 100644
--- a/app/label_printer/label_printer/print_job.rb
+++ b/app/label_printer/label_printer/print_job.rb
@@ -39,5 +39,24 @@ def self.build(attributes)
         LabelPrinter::PrintJob::Base.new(attributes)
       end
     end
+
+    def self.build_from_v2(attributes)
+      labels = attributes[:labels]
+      printer = Printer.find_by(name: attributes[:printer_name])
+
+      if printer.present?
+        "LabelPrinter::PrintJob::#{printer.protocol}"
+          .constantize.new(attributes.merge(printer: printer, labels: convert_labels(labels)))
+      else
+        LabelPrinter::PrintJob::Base.new(attributes)
+      end
+    end
+
+    def self.convert_labels(labels)
+      return if labels.nil?
+
+      labels_with_location = labels.map { |label| { location: label } }
+      { body: labels_with_location }
+    end
   end
 end
diff --git a/app/models/print_job_wrapper.rb b/app/models/print_job_wrapper.rb
index 45f4506..c250527 100644
--- a/app/models/print_job_wrapper.rb
+++ b/app/models/print_job_wrapper.rb
@@ -27,20 +27,22 @@ def print
   def print_job
     @print_job ||= case printer.try(:printer_type)
                    when 'toshiba'
-                     # TODO: we need to convert the labels to the correct format
-                     LabelPrinter::PrintJob.build(print_job_body.except(
-                                                    :label_template_name, :copies
-                                                  ))
+                     LabelPrinter::PrintJob.build_from_v2(print_job_body.except(
+                                                            :label_template_name, :copies
+                                                          ))
                    when 'squix'
                      Squix::PrintJob.new(print_job_body.except(:label_template_id))
                    end
   end
 
   def print_job_body
-    @print_job_body ||= { printer_name: printer_name, label_template_id:
-                          label_template.try(:id),
-                          label_template_name: label_template.try(:name),
-                          labels: labels, copies: copies }
+    @print_job_body ||= {
+      printer_name: printer_name,
+      label_template_id: label_template.try(:id),
+      label_template_name: label_template.try(:name),
+      labels: labels,
+      copies: copies
+    }
   end
 
   def printer
diff --git a/spec/label_printer/print_job_spec.rb b/spec/label_printer/print_job_spec.rb
index be61c68..8d8a4b2 100644
--- a/spec/label_printer/print_job_spec.rb
+++ b/spec/label_printer/print_job_spec.rb
@@ -3,8 +3,10 @@
 require 'rails_helper'
 
 RSpec.describe LabelPrinter::PrintJob, type: :model do
-  let!(:printer)            { create(:printer) }
-  let!(:label_template)     { create(:label_template) }
+  let!(:printer)                { create(:printer) }
+  let!(:label_template)         { create(:label_template) }
+  let!(:labels_for_v2)          { [{ "barcode": "aBarcode1", "parent_location": "aParentLocation1", "location": "location1"}, {"barcode": "aBarcode2", "parent_location": "aParentLocation2", "location": "location2"}] }
+  let!(:expected_labels_for_v2) { { "body": [{"location": {"barcode": "aBarcode1", "parent_location": "aParentLocation1", "location": "location1"}}, {"location": {"barcode": "aBarcode2", "parent_location": "aParentLocation2", "location"=>"location2"}}] }.with_indifferent_access }
 
   it 'should have a printer' do
     expect(build(:print_job, printer_name: printer.name)).to be_valid
@@ -53,4 +55,23 @@
     print_job = LabelPrinter::PrintJob.build(printer_name: create(:printer, protocol: 'IPP').name, label_template_id: label_template.id, labels: label_template.dummy_labels.to_h)
     expect(print_job).to be_IPP
   end
+
+  describe '#convert_labels' do
+    it 'converts the labels as expected' do
+      output = LabelPrinter::PrintJob.convert_labels(labels_for_v2)
+
+      expect(output[:body]).to be_present
+      expect(output[:body].length).to eq 2
+      expect(output[:body][0][:location]).to be_present
+      expect(output[:body][1][:location]).to be_present
+    end
+  end
+
+  describe '#build_from_v2' do
+    it 'calls convert_labels' do
+      print_job = LabelPrinter::PrintJob.build_from_v2(printer_name: printer.name, label_template_id: label_template.id, labels: labels_for_v2)
+      expect(print_job.labels.with_indifferent_access).to eq expected_labels_for_v2
+      expect(print_job).to be_valid
+    end
+  end
 end
diff --git a/spec/models/print_job_wrapper_spec.rb b/spec/models/print_job_wrapper_spec.rb
index 6f4cab1..531aa85 100644
--- a/spec/models/print_job_wrapper_spec.rb
+++ b/spec/models/print_job_wrapper_spec.rb
@@ -25,10 +25,10 @@
     it 'has the correct instance variables' do
       print_job_wrapper = PrintJobWrapper.new(attributes)
 
-      expect(print_job_wrapper.printer).to eq(toshiba_printer) 
+      expect(print_job_wrapper.printer).to eq(toshiba_printer)
       expect(print_job_wrapper.label_template).to eq(label_template)
       expect(print_job_wrapper.labels).to eq(labels)
-      expect(print_job_wrapper.copies).to eq(copies) 
+      expect(print_job_wrapper.copies).to eq(copies)
     end
 
     it 'will be valid with the correct instance variables' do
@@ -55,15 +55,10 @@
   end
 
   describe '#print_job' do
-    
-    context 'Toshiba' do
 
-      # TODO: at the moment we are passing in the labels as the correct format for Toshiba
-      # They need to be in the same format for both
-      let(:toshiba_labels) { label_template.dummy_labels.to_h }
-      
+    context 'Toshiba' do
       it 'creates a new print job of the correct type' do
-        print_job_wrapper = PrintJobWrapper.new(attributes.merge(labels: toshiba_labels))
+        print_job_wrapper = PrintJobWrapper.new(attributes.merge(labels: labels))
         expect(print_job_wrapper.print_job).to be_instance_of(LabelPrinter::PrintJob::LPD)
       end
 

From b71443a6e1191c2125a4dcf42e851cfcb254e1d8 Mon Sep 17 00:00:00 2001
From: Harriet Craven <hc6@sanger.ac.uk>
Date: Tue, 2 Feb 2021 16:38:37 +0000
Subject: [PATCH 12/18] Added label_name for toshiba print jobs

---
 app/controllers/v2/print_jobs_controller.rb  | 1 -
 app/label_printer/label_printer/print_job.rb | 5 ++++-
 app/models/print_job_wrapper.rb              | 9 ++++++++-
 app/models/squix/print_job.rb                | 2 ++
 spec/label_printer/print_job_spec.rb         | 8 ++++----
 spec/models/print_job_wrapper_spec.rb        | 2 +-
 6 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/app/controllers/v2/print_jobs_controller.rb b/app/controllers/v2/print_jobs_controller.rb
index 5c05153..fab7485 100644
--- a/app/controllers/v2/print_jobs_controller.rb
+++ b/app/controllers/v2/print_jobs_controller.rb
@@ -30,7 +30,6 @@ def print_job_params
       p1 = params.permit(
         print_job: %i[
           printer_name
-
           label_template_name
           label_template_id
           copies
diff --git a/app/label_printer/label_printer/print_job.rb b/app/label_printer/label_printer/print_job.rb
index ebee531..ae474a6 100644
--- a/app/label_printer/label_printer/print_job.rb
+++ b/app/label_printer/label_printer/print_job.rb
@@ -55,7 +55,10 @@ def self.build_from_v2(attributes)
     def self.convert_labels(labels)
       return if labels.nil?
 
-      labels_with_location = labels.map { |label| { location: label } }
+      labels_with_location = labels.map do |label|
+        label_name = label[:label_name]
+        { label_name => label.except(:label_name) }
+      end
       { body: labels_with_location }
     end
   end
diff --git a/app/models/print_job_wrapper.rb b/app/models/print_job_wrapper.rb
index c250527..4b94b00 100644
--- a/app/models/print_job_wrapper.rb
+++ b/app/models/print_job_wrapper.rb
@@ -31,7 +31,7 @@ def print_job
                                                             :label_template_name, :copies
                                                           ))
                    when 'squix'
-                     Squix::PrintJob.new(print_job_body.except(:label_template_id))
+                     Squix::PrintJob.new(squix_print_job_body)
                    end
   end
 
@@ -72,4 +72,11 @@ def check_print_job
       errors.add(k, v)
     end
   end
+
+  def squix_print_job_body
+    return if labels.nil?
+
+    squix_labels = print_job_body[:labels].collect { |l| l.except(:label_name) }
+    print_job_body.except(:label_template_id).merge(labels: squix_labels)
+  end
 end
diff --git a/app/models/squix/print_job.rb b/app/models/squix/print_job.rb
index fcd20d4..d29dbec 100644
--- a/app/models/squix/print_job.rb
+++ b/app/models/squix/print_job.rb
@@ -6,6 +6,8 @@ module Squix
   #   "right_text"=>"DN9000003B",
   #   "left_text"=>"DN9000003B",
   #   "barcode"=>"DN9000003B",
+  # },
+  # {
   #   "extra_right_text"=>"DN9000003B  LTHR-384 RT",
   #   "extra_left_text"=>"10-NOV-2020"
   # }]
diff --git a/spec/label_printer/print_job_spec.rb b/spec/label_printer/print_job_spec.rb
index 8d8a4b2..0ebba9b 100644
--- a/spec/label_printer/print_job_spec.rb
+++ b/spec/label_printer/print_job_spec.rb
@@ -5,8 +5,8 @@
 RSpec.describe LabelPrinter::PrintJob, type: :model do
   let!(:printer)                { create(:printer) }
   let!(:label_template)         { create(:label_template) }
-  let!(:labels_for_v2)          { [{ "barcode": "aBarcode1", "parent_location": "aParentLocation1", "location": "location1"}, {"barcode": "aBarcode2", "parent_location": "aParentLocation2", "location": "location2"}] }
-  let!(:expected_labels_for_v2) { { "body": [{"location": {"barcode": "aBarcode1", "parent_location": "aParentLocation1", "location": "location1"}}, {"location": {"barcode": "aBarcode2", "parent_location": "aParentLocation2", "location"=>"location2"}}] }.with_indifferent_access }
+  let!(:labels_for_v2)          { [{ "barcode": "aBarcode1", "parent_location": "aParentLocation1", "location": "location1", "label_name": "location_label_name_1"}, {"barcode": "aBarcode2", "parent_location": "aParentLocation2", "location": "location2", "label_name": "location_label_name_2"}] }
+  let!(:expected_labels_for_v2) { { "body": [{"location_label_name_1": {"barcode": "aBarcode1", "parent_location": "aParentLocation1", "location": "location1"}}, {"location_label_name_2": {"barcode": "aBarcode2", "parent_location": "aParentLocation2", "location"=>"location2"}}] }.with_indifferent_access }
 
   it 'should have a printer' do
     expect(build(:print_job, printer_name: printer.name)).to be_valid
@@ -62,8 +62,8 @@
 
       expect(output[:body]).to be_present
       expect(output[:body].length).to eq 2
-      expect(output[:body][0][:location]).to be_present
-      expect(output[:body][1][:location]).to be_present
+      expect(output[:body][0]['location_label_name_1']).to be_present
+      expect(output[:body][1]['location_label_name_2']).to be_present
     end
   end
 
diff --git a/spec/models/print_job_wrapper_spec.rb b/spec/models/print_job_wrapper_spec.rb
index 531aa85..a35742b 100644
--- a/spec/models/print_job_wrapper_spec.rb
+++ b/spec/models/print_job_wrapper_spec.rb
@@ -17,7 +17,7 @@
   let!(:squix_printer)      { create(:squix_printer) }
   let!(:toshiba_printer)    { create(:toshiba_printer) }
   let(:label_template)      { create(:label_template) }
-  let(:labels)              { [{ 'test_attr' => 'test', 'barcode' => '12345' }] }
+  let(:labels)              { [{ 'test_attr' => 'test', 'barcode' => '11111', 'label_name': 'location' }, { 'test_attr' => 'test2', 'barcode' => '22222', 'label_name': 'location2' }] }
   let(:copies)              { 2 }
   let(:attributes)          { { printer_name: toshiba_printer.name, label_template_name: label_template.name, labels: labels, copies: copies } }
 

From 29bc4cc19e070f1e9ba3043d813507fb7d458ba0 Mon Sep 17 00:00:00 2001
From: Stephen Inglis <si5@sanger.ac.uk>
Date: Wed, 3 Feb 2021 10:34:48 +0000
Subject: [PATCH 13/18] Added some comments.

---
 app/label_printer/label_printer/print_job.rb | 41 +++++++++++++++-----
 app/models/squix/print_job.rb                |  2 +
 spec/models/print_job_wrapper_spec.rb        | 10 -----
 spec/models/printer_spec.rb                  |  6 ---
 spec/models/squix/print_job_spec.rb          |  6 ---
 5 files changed, 34 insertions(+), 31 deletions(-)

diff --git a/app/label_printer/label_printer/print_job.rb b/app/label_printer/label_printer/print_job.rb
index ae474a6..3b989b3 100644
--- a/app/label_printer/label_printer/print_job.rb
+++ b/app/label_printer/label_printer/print_job.rb
@@ -40,23 +40,46 @@ def self.build(attributes)
       end
     end
 
+    # If the data is coming from v2 then we need to convert the labels
+    # into a compatible format
     def self.build_from_v2(attributes)
-      labels = attributes[:labels]
-      printer = Printer.find_by(name: attributes[:printer_name])
-
-      if printer.present?
-        "LabelPrinter::PrintJob::#{printer.protocol}"
-          .constantize.new(attributes.merge(printer: printer, labels: convert_labels(labels)))
-      else
-        LabelPrinter::PrintJob::Base.new(attributes)
-      end
+      build(attributes.merge(labels: convert_labels(attributes[:labels])))
     end
 
+    # Example labels from v2:
+    # labels:
+    # [{
+    #   "right_text"=>"DN9000003B",
+    #   "left_text"=>"DN9000003B",
+    #   "barcode"=>"DN9000003B",
+    #   "label_name"=>"main_label"
+    # },
+    # {
+    #   "extra_right_text"=>"DN9000003B  LTHR-384 RT",
+    #   "extra_left_text"=>"10-NOV-2020"
+    #   "label_name"=>"extra_label"
+    # }]
+    # and v1 equivalent
+    # {
+    #  "body" => [{
+    #   "main_label" => {
+    #   "right_text"=>"DN9000003B",
+    #   "left_text"=>"DN9000003B",
+    #   "barcode"=>"DN9000003B",
+    #   
+    # }},
+    # { "extra_label" => {
+    #   "extra_right_text"=>"DN9000003B  LTHR-384 RT",
+    #   "extra_left_text"=>"10-NOV-2020"
+    # }}]
+    # }
     def self.convert_labels(labels)
       return if labels.nil?
 
       labels_with_location = labels.map do |label|
         label_name = label[:label_name]
+        
+        # we need to remove the label_name otherwise it will cause a failure
         { label_name => label.except(:label_name) }
       end
       { body: labels_with_location }
diff --git a/app/models/squix/print_job.rb b/app/models/squix/print_job.rb
index d29dbec..49d3fac 100644
--- a/app/models/squix/print_job.rb
+++ b/app/models/squix/print_job.rb
@@ -6,10 +6,12 @@ module Squix
   #   "right_text"=>"DN9000003B",
   #   "left_text"=>"DN9000003B",
   #   "barcode"=>"DN9000003B",
+  #   "label_name"=>"main_label"
   # },
   # {
   #   "extra_right_text"=>"DN9000003B  LTHR-384 RT",
   #   "extra_left_text"=>"10-NOV-2020"
+  #   "label_name"=>"extra_label"
   # }]
 
   # Squix::PrintJob
diff --git a/spec/models/print_job_wrapper_spec.rb b/spec/models/print_job_wrapper_spec.rb
index a35742b..64c5bb0 100644
--- a/spec/models/print_job_wrapper_spec.rb
+++ b/spec/models/print_job_wrapper_spec.rb
@@ -2,16 +2,6 @@
 
 require 'rails_helper'
 
-# What I did:
-# used the printer type factories
-# renamed print_job_wrapper to print_job_wrapper feels better semantically
-# added tests to check validity of print_job_wrapper
-# modified tests which seem to be mocking out the print jobs as we need to check the validity of the print_jobs themselves.
-# changed labels to what should be expected
-# changed assert_equal to rspec expect syntax
-# took copies out of everything except first test
-# did a bit of stuff with copies as it should not be required
-# changed copies to a different number as from what I understand it defaults to 1
 RSpec.describe PrintJobWrapper do
 
   let!(:squix_printer)      { create(:squix_printer) }
diff --git a/spec/models/printer_spec.rb b/spec/models/printer_spec.rb
index 0574e42..4861804 100644
--- a/spec/models/printer_spec.rb
+++ b/spec/models/printer_spec.rb
@@ -2,12 +2,6 @@
 
 require 'rails_helper'
 
-# what I did:
-# removed extra tests that were testing the same thing twice
-# renamed some tests to reflect what they were testing
-# checked printer type values against their enums rather than using strings
-# checked validity of printer types within creating printer
-# added more descriptive factories for different printer types
 RSpec.describe Printer, type: :model do
   it 'should not be valid without a name' do
     expect(build(:printer, name: nil)).to_not be_valid
diff --git a/spec/models/squix/print_job_spec.rb b/spec/models/squix/print_job_spec.rb
index f19efd8..aae0f79 100644
--- a/spec/models/squix/print_job_spec.rb
+++ b/spec/models/squix/print_job_spec.rb
@@ -2,12 +2,6 @@
 
 require 'rails_helper'
 
-# what I did:
-# Renamed the file as it did not have spec at the end. This means tests will not be run when the bundle exec rspec
-# Renamed pj to print_job
-# Modified setup so that you can reuse attributes
-# Added tests to check that print job was valid
-# modified label setup as it didnt seem to be setup to what sprint client was expecting
 RSpec.describe Squix::PrintJob do
 
   let!(:printer)            { create(:printer) }

From 4bbf6a9eea136064e06ac7c969bee70037b48019 Mon Sep 17 00:00:00 2001
From: Stephen Inglis <si5@sanger.ac.uk>
Date: Wed, 3 Feb 2021 10:35:31 +0000
Subject: [PATCH 14/18] Rubocopped.

---
 app/label_printer/label_printer/print_job.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/label_printer/label_printer/print_job.rb b/app/label_printer/label_printer/print_job.rb
index 3b989b3..1dd6fe8 100644
--- a/app/label_printer/label_printer/print_job.rb
+++ b/app/label_printer/label_printer/print_job.rb
@@ -66,7 +66,7 @@ def self.build_from_v2(attributes)
     #   "right_text"=>"DN9000003B",
     #   "left_text"=>"DN9000003B",
     #   "barcode"=>"DN9000003B",
-    #   
+    #
     # }},
     # { "extra_label" => {
     #   "extra_right_text"=>"DN9000003B  LTHR-384 RT",
@@ -78,7 +78,7 @@ def self.convert_labels(labels)
 
       labels_with_location = labels.map do |label|
         label_name = label[:label_name]
-        
+
         # we need to remove the label_name otherwise it will cause a failure
         { label_name => label.except(:label_name) }
       end

From 1fd1552e7a39ab96770a5ff55af144c343d71b43 Mon Sep 17 00:00:00 2001
From: Harriet Craven <hc6@sanger.ac.uk>
Date: Wed, 3 Feb 2021 10:51:30 +0000
Subject: [PATCH 15/18] added label conversion for squix

---
 app/controllers/v2/print_jobs_controller.rb |  1 -
 app/models/print_job_wrapper.rb             |  9 +--------
 app/models/squix/print_job.rb               |  8 +++++++-
 spec/models/squix/print_job_spec.rb         | 18 ++++++++++--------
 4 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/app/controllers/v2/print_jobs_controller.rb b/app/controllers/v2/print_jobs_controller.rb
index fab7485..9bba7ca 100644
--- a/app/controllers/v2/print_jobs_controller.rb
+++ b/app/controllers/v2/print_jobs_controller.rb
@@ -31,7 +31,6 @@ def print_job_params
         print_job: %i[
           printer_name
           label_template_name
-          label_template_id
           copies
         ]
       )[:print_job].to_h
diff --git a/app/models/print_job_wrapper.rb b/app/models/print_job_wrapper.rb
index 4b94b00..c250527 100644
--- a/app/models/print_job_wrapper.rb
+++ b/app/models/print_job_wrapper.rb
@@ -31,7 +31,7 @@ def print_job
                                                             :label_template_name, :copies
                                                           ))
                    when 'squix'
-                     Squix::PrintJob.new(squix_print_job_body)
+                     Squix::PrintJob.new(print_job_body.except(:label_template_id))
                    end
   end
 
@@ -72,11 +72,4 @@ def check_print_job
       errors.add(k, v)
     end
   end
-
-  def squix_print_job_body
-    return if labels.nil?
-
-    squix_labels = print_job_body[:labels].collect { |l| l.except(:label_name) }
-    print_job_body.except(:label_template_id).merge(labels: squix_labels)
-  end
 end
diff --git a/app/models/squix/print_job.rb b/app/models/squix/print_job.rb
index 49d3fac..f0afa5d 100644
--- a/app/models/squix/print_job.rb
+++ b/app/models/squix/print_job.rb
@@ -31,7 +31,13 @@ def execute
     end
 
     def merge_fields_list
-      labels * copies
+      converted_labels * copies
+    end
+
+    def converted_labels
+      return if labels.nil?
+
+      labels.collect { |l| l.except(:label_name) }
     end
   end
 end
diff --git a/spec/models/squix/print_job_spec.rb b/spec/models/squix/print_job_spec.rb
index aae0f79..ad0aff4 100644
--- a/spec/models/squix/print_job_spec.rb
+++ b/spec/models/squix/print_job_spec.rb
@@ -6,19 +6,20 @@
 
   let!(:printer)            { create(:printer) }
   let(:label_template)      { create(:label_template) }
-  let(:label)               { { 'test_attr' => 'test', 'barcode' => '12345' } }
-  let(:labels)              { [label] }
+  let(:label1)               { { 'test_attr': 'test1', 'barcode': '11111', 'label_name': 'location1' } }
+  let(:label2)               { { 'test_attr': 'test2', 'barcode': '22222', 'label_name': 'location2' } }
+  let(:labels)              { [label1, label2] }
   let(:copies)              { 1 }
-  let(:attributes)          { { printer_name: printer.name, label_template_name: label_template.name, labels: [label], copies: copies } }
+  let(:attributes)          { { printer_name: printer.name, label_template_name: label_template.name, labels: labels, copies: copies } }
 
   describe 'new' do
     it 'will have the correct instance variables' do
       print_job = Squix::PrintJob.new(attributes)
 
-      expect(print_job.printer_name).to eq(printer.name) 
-      expect(print_job.label_template_name).to eq(label_template.name) 
-      expect(print_job.labels).to eq(labels) 
-      expect(print_job.copies).to eq(copies) 
+      expect(print_job.printer_name).to eq(printer.name)
+      expect(print_job.label_template_name).to eq(label_template.name)
+      expect(print_job.labels).to eq(labels)
+      expect(print_job.copies).to eq(copies)
     end
 
     it 'will not be valid without the printer name' do
@@ -48,7 +49,8 @@
   describe '#merge_fields_list' do
     it 'will return the correct number of merge fields when copies is more than 1' do
       print_job = Squix::PrintJob.new(attributes.merge(copies: 2))
-      expect(print_job.merge_fields_list).to eq([label] * 2)
+      expected = print_job.converted_labels
+      expect(print_job.merge_fields_list).to eq(expected * 2)
     end
   end
 

From 4ebe7564ca464f2b53d64c8cecba3afadfc42799 Mon Sep 17 00:00:00 2001
From: Harriet Craven <hc6@sanger.ac.uk>
Date: Thu, 4 Feb 2021 11:45:48 +0000
Subject: [PATCH 16/18] added valdiation for label names

---
 app/models/print_job_wrapper.rb | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/app/models/print_job_wrapper.rb b/app/models/print_job_wrapper.rb
index c250527..d984d36 100644
--- a/app/models/print_job_wrapper.rb
+++ b/app/models/print_job_wrapper.rb
@@ -16,7 +16,7 @@ class PrintJobWrapper
 
   validates :label_template, :printer, :labels, presence: true
 
-  validate :check_print_job
+  validate :check_print_job, :check_label_names
 
   def print
     return false unless valid?
@@ -72,4 +72,13 @@ def check_print_job
       errors.add(k, v)
     end
   end
+
+  def check_label_names
+    expected_label_names = label_template.labels.pluck(:name)
+    received_label_names = labels.map { |l| l["label_name"] }.uniq
+
+    # each item in received_label_names has to exist in expected_label_names
+    return if expected_label_names & received_label_names == received_label_names
+    errors.add(:label_name, 'does not match label template label names')
+  end
 end

From b77fc9835e5a4febe58765db55d6d7ecd1b2981d Mon Sep 17 00:00:00 2001
From: Stephen Inglis <si5@sanger.ac.uk>
Date: Thu, 4 Feb 2021 12:22:00 +0000
Subject: [PATCH 17/18] Added test to ensure label names are correct.

---
 app/models/print_job_wrapper.rb       | 12 +++++++++++-
 spec/factories/label_templates.rb     |  8 ++++++++
 spec/models/print_job_wrapper_spec.rb | 11 +++++++++--
 spec/v2/print_jobs_spec.rb            |  4 ++--
 4 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/app/models/print_job_wrapper.rb b/app/models/print_job_wrapper.rb
index d984d36..681f1e8 100644
--- a/app/models/print_job_wrapper.rb
+++ b/app/models/print_job_wrapper.rb
@@ -73,12 +73,22 @@ def check_print_job
     end
   end
 
+  # There is a possibility that some of the label names will not match the
+  # label names in the label template this would cause an encode error
+  # which is hard to debug which we found out to our cost
+  # this will tell you to check!
+  # TODO: fix rubocop
+  # rubocop:disable Metrics/AbcSize
   def check_label_names
+    return if labels.nil? || label_template.nil?
+
     expected_label_names = label_template.labels.pluck(:name)
-    received_label_names = labels.map { |l| l["label_name"] }.uniq
+    received_label_names = labels.map { |l| l['label_name'] }.uniq
 
     # each item in received_label_names has to exist in expected_label_names
     return if expected_label_names & received_label_names == received_label_names
+
     errors.add(:label_name, 'does not match label template label names')
   end
+  # rubocop:enable Metrics/AbcSize
 end
diff --git a/spec/factories/label_templates.rb b/spec/factories/label_templates.rb
index 00fa3bc..76977da 100644
--- a/spec/factories/label_templates.rb
+++ b/spec/factories/label_templates.rb
@@ -10,4 +10,12 @@
        FactoryBot.create(:label_with_drawings, name: 'footer')].flatten
     end
   end
+
+  factory :label_template_simple, class: LabelTemplate do
+    sequence(:name) { |n| "label_template_#{n}" }
+    label_type
+    labels do
+      [FactoryBot.create(:label_with_drawings)]
+    end
+  end
 end
diff --git a/spec/models/print_job_wrapper_spec.rb b/spec/models/print_job_wrapper_spec.rb
index 64c5bb0..fae7cc7 100644
--- a/spec/models/print_job_wrapper_spec.rb
+++ b/spec/models/print_job_wrapper_spec.rb
@@ -6,8 +6,10 @@
 
   let!(:squix_printer)      { create(:squix_printer) }
   let!(:toshiba_printer)    { create(:toshiba_printer) }
-  let(:label_template)      { create(:label_template) }
-  let(:labels)              { [{ 'test_attr' => 'test', 'barcode' => '11111', 'label_name': 'location' }, { 'test_attr' => 'test2', 'barcode' => '22222', 'label_name': 'location2' }] }
+  let(:label_template)      { create(:label_template_simple) }
+  let(:dodgy_labels)        { [{ 'test_attr' => 'test', 'barcode' => '11111', 'label_name': 'location' }, { 'test_attr' => 'test2', 'barcode' => '22222', 'label_name': 'location2' }] }
+  # TODO: move this to a method
+  let(:labels)              { label_template.dummy_labels.to_h[:body].collect { |label| label.collect { |k,v| v.merge(label_name: k)}}.flatten}
   let(:copies)              { 2 }
   let(:attributes)          { { printer_name: toshiba_printer.name, label_template_name: label_template.name, labels: labels, copies: copies } }
 
@@ -42,6 +44,11 @@
       expect(print_job_wrapper).to be_valid
       expect(print_job_wrapper.copies).to be_present
     end
+
+    it 'will not be valid if the label names dont match' do
+      print_job_wrapper = PrintJobWrapper.new(attributes.merge(labels: dodgy_labels))
+      expect(print_job_wrapper).to_not be_valid
+    end
   end
 
   describe '#print_job' do
diff --git a/spec/v2/print_jobs_spec.rb b/spec/v2/print_jobs_spec.rb
index 6757ad6..6412b60 100644
--- a/spec/v2/print_jobs_spec.rb
+++ b/spec/v2/print_jobs_spec.rb
@@ -6,8 +6,8 @@
 
   let!(:squix_printer)    { create(:printer, printer_type: :squix) }
   let!(:toshiba_printer)  { create(:printer, printer_type: :toshiba) }
-  let!(:label_template)   { create(:label_template) }
-  let(:labels)            { [{ 'test_attr' => 'test', 'barcode' => '12345' }] }
+  let!(:label_template)   { create(:label_template_simple) }
+  let(:labels)            { label_template.dummy_labels.to_h[:body].collect { |label| label.collect { |k,v| v.merge(label_name: k)}}.flatten }
   let(:copies)            { '1' }
 
   describe 'On success' do

From 2f5923de1fc5d812a44dbeb59872dbc52b0c7efa Mon Sep 17 00:00:00 2001
From: Harriet Craven <hc6@sanger.ac.uk>
Date: Thu, 4 Feb 2021 13:00:04 +0000
Subject: [PATCH 18/18] added error handling

---
 app/models/squix/print_job.rb             |  7 +++++-
 config/sprint/label_templates/labwhere_1d | 30 +++++++++++++++++++++++
 spec/models/squix/print_job_spec.rb       | 22 ++++++++++++++---
 spec/v2/print_jobs_spec.rb                | 18 ++++++++++++--
 4 files changed, 70 insertions(+), 7 deletions(-)
 create mode 100644 config/sprint/label_templates/labwhere_1d

diff --git a/app/models/squix/print_job.rb b/app/models/squix/print_job.rb
index f0afa5d..79466ad 100644
--- a/app/models/squix/print_job.rb
+++ b/app/models/squix/print_job.rb
@@ -23,11 +23,16 @@ class PrintJob
     validates :printer_name, :label_template_name, :labels, :copies, presence: true
 
     def execute
-      SPrintClient.send_print_request(
+      response = SPrintClient.send_print_request(
         printer_name,
         label_template_name,
         merge_fields_list
       )
+
+      # Response contains error message, if required
+      return false unless response.code == '200'
+
+      true
     end
 
     def merge_fields_list
diff --git a/config/sprint/label_templates/labwhere_1d b/config/sprint/label_templates/labwhere_1d
new file mode 100644
index 0000000..9d3934a
--- /dev/null
+++ b/config/sprint/label_templates/labwhere_1d
@@ -0,0 +1,30 @@
+[
+  {
+    "barcodeFields": [
+      {
+        "x": 30,
+        "y": 1,
+        "cellWidth": 0.2,
+        "barcodeType": "code128",
+        "value": "<%= merge_fields[:barcode] %>",
+        "height": 5
+      }
+    ],
+    "textFields": [
+      {
+        "x": 3,
+        "y": 3,
+        "value": "<%= merge_fields[:location] %>",
+        "font": "proportional",
+        "fontSize": 1.7
+      },
+      {
+        "x": 3,
+        "y": 6,
+        "value": "<%= merge_fields[:parent_location] %>",
+        "font": "proportional",
+        "fontSize": 1.7
+      }
+    ]
+  }
+]
\ No newline at end of file
diff --git a/spec/models/squix/print_job_spec.rb b/spec/models/squix/print_job_spec.rb
index ad0aff4..3edf39f 100644
--- a/spec/models/squix/print_job_spec.rb
+++ b/spec/models/squix/print_job_spec.rb
@@ -39,10 +39,24 @@
   end
 
   describe '#execute' do
-    it 'sends a print request' do
-      print_job = Squix::PrintJob.new(attributes)
-      expect(SPrintClient).to receive(:send_print_request).with(print_job.printer_name, print_job.label_template_name, print_job.merge_fields_list)
-      print_job.execute
+    context 'success' do
+      it 'sends a print request and returns true' do
+        print_job = Squix::PrintJob.new(attributes)
+        allow(SPrintClient).to receive(:send_print_request).and_return Net::HTTPResponse.new('1.1', "200", "")
+        expect(SPrintClient).to receive(:send_print_request).with(print_job.printer_name, print_job.label_template_name, print_job.merge_fields_list)
+        result = print_job.execute
+        expect(result).to eq true
+      end
+    end
+
+    context 'failure' do
+      it 'sends a print request and returns false' do
+        print_job = Squix::PrintJob.new(attributes)
+        allow(SPrintClient).to receive(:send_print_request).and_return Net::HTTPResponse.new('1.1', "422", "an error")
+        expect(SPrintClient).to receive(:send_print_request).with(print_job.printer_name, print_job.label_template_name, print_job.merge_fields_list)
+        result = print_job.execute
+        expect(result).to eq false
+      end
     end
   end
 
diff --git a/spec/v2/print_jobs_spec.rb b/spec/v2/print_jobs_spec.rb
index 6412b60..b505a73 100644
--- a/spec/v2/print_jobs_spec.rb
+++ b/spec/v2/print_jobs_spec.rb
@@ -9,12 +9,14 @@
   let!(:label_template)   { create(:label_template_simple) }
   let(:labels)            { label_template.dummy_labels.to_h[:body].collect { |label| label.collect { |k,v| v.merge(label_name: k)}}.flatten }
   let(:copies)            { '1' }
+  let(:success_response)  { Net::HTTPResponse.new('1.1', "200", "") }
+  let(:failure_response)  { Net::HTTPResponse.new('1.1', "422", "An error") }
 
   describe 'On success' do
     context 'When printer type is Squix' do
       it 'should send_print_request to SPrintClient ' do
         body = { printer_name: squix_printer.name, label_template_name: label_template.name, labels: labels, copies: copies }
-        allow(SPrintClient).to receive(:send_print_request).and_return(true)
+        allow(SPrintClient).to receive(:send_print_request).and_return(success_response)
 
         post v2_print_jobs_path, params: { print_job: body }
 
@@ -42,7 +44,19 @@
 
   describe 'On failure' do
     context 'when wrapper is not valid' do
-      it 'should return an error if the printer name is missing' do
+      it 'fails when label_template name is missing' do
+        body = { printer_name: squix_printer.name, label_template_name: '', labels: labels }
+
+        post v2_print_jobs_path, params: { print_job: body }
+        expect(response).to have_http_status(:unprocessable_entity)
+
+        json = ActiveSupport::JSON.decode(response.body)
+
+        expect(json['errors']).not_to be_empty
+        expect(find_attribute_error_details(json, 'label_template')).to include("can't be blank")
+      end
+
+      it 'fails when printer name is missing' do
         body = { printer_name: '', label_template_name: label_template.name, labels: labels }
 
         post v2_print_jobs_path, params: { print_job: body }