qa fixes

app/assets/stylesheets/spree/backend/spree_digital_assets.css

@@ -3,3 +3,7 @@ Placeholder manifest file.
 the installer will append this file to the app vendored assets here: 'vendor/assets/stylesheets/spree/backend/all.css'
  *= require spree/backend/spree_digital_assets/style
 */
+
+.note {
+  font-size: 10px;
+}

app/controllers/spree/admin/banners_controller.rb

@@ -1,6 +1,5 @@
 module Spree
   module Admin
-
     class BannersController < ResourceController
 
       before_action :load_banner, only: [:edit, :update, :destroy, :toggle_banner_active_status]
@@ -22,10 +21,10 @@ def index
       end
 
       def toggle_banner_active_status
-        if @banner.toggle!(:active)
+        if @banner.change_active_status
           flash[:success] = Spree.t(:successfully_updated_banner)
         else
-          flash[:danger] = Spree.t(:error)
+          flash[:notice] = @banner.errors.full_messages.join
         end
         redirect_to admin_banners_path
       end

app/models/spree/banner.rb

@@ -1,26 +1,59 @@
 module Spree
   class Banner < Spree::Base
+
+    URL_VALIDATION_REGEX = /^(http|https):\/\/[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?$/ix
+
     has_attached_file :attachment, styles: { mini: '48x48>', small: '100x100>', product: '240x240>', large: '600x600>' },
                       url: '/spree/banner/:id/:style/:basename.:extension',
                       path: ':rails_root/public/spree/banner/:id/:style/:basename.:extension'
 
+
     validates_attachment :attachment, content_type: { content_type: ["image/jpg", "image/jpeg", "image/png", "image/gif"] }
-    validate :only_one_mobile_banner, if: :mobile_banner?
+    validate :only_one_mobile_banner
 
     with_options presence: true do
       validates :title, uniqueness: true
       validates :link
       validates :attachment
     end
 
+    validates_format_of :link, with: URL_VALIDATION_REGEX, multiline: true, allow_blank: true
+
     scope :active, -> { where(active: true) }
-    scope :mobile_banner, -> { where(mobile_banner: true) }
+    scope :mobile_active_banner, -> { where(mobile_banner: true, active: true) }
+
+    before_destroy :restrict_if_active
 
-    def only_one_mobile_banner
-      if Banner.mobile_banner.present?
-        errors.add(:mobile_banner, Spree.t(:only_one_mobile_banner))
+    def change_active_status
+      if active?
+        update_columns(active: false, mobile_banner: false)
+      else
+        update_column(:active, true)
       end
     end
 
+    private
+
+      def only_one_mobile_banner
+        if mobile_banner?
+          if !active?
+            errors.add(:active, Spree.t(:only_active_mobile_banner))
+          elsif other_mobile_banner_active?
+            errors.add(:base, Spree.t(:only_one_mobile_banner))
+          end
+        end
+      end
+
+      def other_mobile_banner_active?
+        (Banner.mobile_active_banner - [self]).length > 0
+      end
+
+      def restrict_if_active
+        if active?
+          errors.add(:base, Spree.t(:cannot_delete_active_banner))
+          throw(:abort)
+        end
+      end
+
   end
 end

app/views/spree/admin/banners/_form.html.erb

@@ -3,22 +3,20 @@
     <%= f.field_container :title do %>
       <%= f.label :title %> <span class="required">*</span><br/>
       <%= f.text_field :title, class: 'form-control' %>
-      <%= f.error_message_on :title %>
     <% end %>
   </div>
   <div class="col-md-5">
     <%= f.field_container :link do %>
       <%= f.label :link %> <span class="required">*</span><br/>
       <%= f.text_field :link, class: 'form-control' %>
-      <%= f.error_message_on :link %>
     <% end %>
   </div>
 
   <div class="col-md-5">
     <%= f.field_container :attachment do %>
       <%= f.label :attachment %><span class="required">*</span><br/>
       <%= f.file_field :attachment, class: 'form-control select-file btn btn-default' %>
-      <%= f.error_message_on :attachment %>
+      <p class="note text-muted">Upload pic with approximate 1022 × 60 dimensions</p>
     <% end %>
   </div>
 
@@ -33,7 +31,6 @@
     <%= f.field_container :mobile_banner do %>
       <%= f.label :mobile_banner %>
       <%= f.check_box :mobile_banner %>
-      <%= f.error_message_on :mobile_banner %>
     <% end %>
   </div>
 

app/views/spree/admin/banners/edit.html.erb

@@ -6,6 +6,8 @@
   <%= button_link_to Spree.t(:back_to_banner_list), spree.admin_banners_path, icon: 'arrow-left', class: 'btn btn-primary' %>
 <% end %>
 
+<%= render partial: 'spree/shared/error_messages', locals: { target: @banner } %>
+
 <%= form_for [:admin, @banner] do |f| %>
   <fieldset class="no-border-top">
     <%= render partial: 'form', locals: { f: f } %>

app/views/spree/admin/banners/index.html.erb

@@ -6,29 +6,36 @@
   <%= button_link_to Spree.t(:new_banner), spree.new_admin_banner_path, icon: 'arrow-left', class: 'btn btn-primary' %>
 <% end %>
 
+
 <table class="index table table-hover">
   <thead>
     <tr>
       <th><%= Spree.t(:Title) %></th>
       <th><%= Spree.t(:Link) %></th>
       <th><%= Spree.t(:image) %></th>
       <th><%= Spree.t(:active) %></th>
+      <th><%= Spree.t(:mobile_banner) %></th>
       <th class="actions" width="100"></th>
     </tr>
   </thead>
   <tbody>
     <% @banners.each do |banner| %>
-      <tr id="<%= spree_dom_id banner %>">
+      <tr id="<%= spree_dom_id banner %>" data-hook="admin_banners_index_rows">
         <td><%= banner.title %></td>
-        <td><%= link_to banner.link, banner.link %></td>
+        <td><%= link_to banner.link, banner.link, target: :_blank %></td>
         <td><%= image_tag banner.attachment(:mini) %></td>
         <td><%= link_to toggle_banner_active_status_admin_banner_path(banner), method: :patch, class: "btn #{banner_active(banner)} btn-site-primary text-uppercase" do %>
             <span class='glyphicon glyphicon-pencil'> <%= banner.active %> </span>
           <% end %>
         </td>
+        <td><%= banner.mobile_banner %></td>
         <td class="actions">
-          <%= link_to_edit banner, no_text: true if can?(:edit, banner) %>
-          &nbsp;
+          <%= link_to_edit banner, no_text: true if can? :edit, banner %>
+          <% if banner.active? %>
+            <%= link_to_with_icon('delete', '', '#', disabled: true, no_text: true, class: 'btn btn-danger btn-sm') %>
+          <% else %>
+            <%= link_to_delete banner, no_text: true %>
+          <% end %>
         </td>
       </tr>
     <% end %>

app/views/spree/admin/banners/new.html.erb

@@ -6,6 +6,8 @@
   <%= button_link_to Spree.t(:back_to_banner_list), spree.admin_banners_path, icon: 'arrow-left', class: 'btn btn-primary' %>
 <% end %>
 
+<%= render partial: 'spree/shared/error_messages', locals: { target: @banner } %>
+
 <%= form_for [:admin, @banner] do |f| %>
   <fieldset class="no-border-top">
     <%= render partial: 'form', locals: { f: f } %>

app/views/spree/layouts/_banner_div.html.erb

@@ -1,4 +1,4 @@
-<% banners = (request.variant.include?(:phone)) ? Spree::Banner.mobile_banner : Spree::Banner.active %>
+<% banners = (request.variant.include?(:phone)) ? Spree::Banner.mobile_active_banner : Spree::Banner.active %>
 <% if banners.present? %>
   <div class="banners-block banner-left banner-list-flex">
     <% banners.each do |banner| %>

config/locales/en.yml

@@ -14,6 +14,9 @@ en:
     back_to_banner_list: Back To Banner List
     successfully_created_banner: Banner is successfully created
     banners: Banners
-    successfully_updated_banner: Successfully Updated Banner
-    error: Banner couldn't be updated. Please try again
-    only_one_mobile_banner: Only one Mobile banner is allowed. Disable other to enable this.
+    successfully_updated_banner: Banner updated Successfully
+    error: Banner couldn't be deleted. Please try again
+    only_one_mobile_banner: Only one mobile banner is allowed. Disable other to enable this
+    successfully_deleted_banner: Banner deleted Successfully
+    cannot_delete_active_banner: Cannot delete Active Banner. Disable first to delete.
+    only_active_mobile_banner: banner can be mobile banner.

db/migrate/20180711131600_create_spree_banner.rb

@@ -2,7 +2,7 @@ class CreateSpreeBanner < ActiveRecord::Migration[5.1]
   def change
     create_table :spree_banners do |t|
       t.string :title
-      t.boolean :active, default: false
+      t.boolean :active, default: false, null: false
       t.string :link
       t.attachment :attachment
       t.timestamps

db/migrate/20180731080848_add_mobile_banner_to_spree_banners.rb

@@ -1,5 +1,5 @@
 class AddMobileBannerToSpreeBanners < ActiveRecord::Migration[5.1]
   def change
-    add_column :spree_banners, :mobile_banner, :boolean, default: false
+    add_column :spree_banners, :mobile_banner, :boolean, default: false, null: false
   end
 end