Merge pull request #660 from crossroads/GCW-2433-make-mobile-field-op…

…tional-for-user-creation-from-stock-app

Feb2019Release1

app/controllers/api/v1/organisations_users_controller.rb

@@ -29,7 +29,7 @@ class OrganisationsUsersController < Api::V1::ApiController
       api :POST, "/v1/organisations_user", "Create a package"
       param_group :organisations_user
       def create
-        builder = OrganisationsUserBuilder.new(params['organisations_user'].to_hash).build
+        builder = OrganisationsUserBuilder.new(params['organisations_user'].to_hash).build(is_stock_app?)
         if builder['result']
           save_and_render_object_with_errors(builder['organisations_user'])
         else

app/jobs/twilio_job.rb

@@ -3,8 +3,10 @@ class TwilioJob < ActiveJob::Base
 
   # e.g. options = { to: @user.mobile, body: body }
   def perform(options)
-    twilio_conf = Rails.application.secrets.twilio
-    client = Twilio::REST::Client.new(twilio_conf['account_sid'], twilio_conf['auth_token'])
-    client.account.messages.create({ from: twilio_conf['phone_number'] }.merge(options))
+    if options[:to].present?
+      twilio_conf = Rails.application.secrets.twilio
+      client = Twilio::REST::Client.new(twilio_conf['account_sid'], twilio_conf['auth_token'])
+      client.account.messages.create({ from: twilio_conf['phone_number'] }.merge(options))
+    end
   end
 end

app/models/user.rb

@@ -27,7 +27,9 @@ class User < ActiveRecord::Base
 
   accepts_nested_attributes_for :address, allow_destroy: true
 
-  validates :mobile, presence: true, uniqueness: true, format: { with: Mobile::HONGKONGMOBILEREGEXP }
+  validates :mobile, format: { with: Mobile::HONGKONGMOBILEREGEXP }, unless: :request_from_stock_without_mobile?
+
+  validates :mobile, presence: true, uniqueness: true, unless: :request_from_stock_without_mobile?
 
   validates :email, uniqueness: true, allow_nil: true,
     format: { with: /\A[^@\s]+@[^@\s]+\Z/ }
@@ -47,6 +49,9 @@ class User < ActiveRecord::Base
   # used when reviewer is logged into donor app
   attr :treat_user_as_donor
 
+  #added to allow sign_up without mobile number from stock app.
+  attr_accessor :request_from_stock
+
   # If user exists, ignore data and just send_verification_pin
   # Otherwise, create new user and send pin
   def self.creation_with_auth(user_params, app_name)
@@ -185,6 +190,10 @@ def create_or_remove_user_roles(role_ids)
 
   private
 
+  def request_from_stock_without_mobile?
+    request_from_stock && mobile.blank?
+  end
+
   def remove_user_roles(role_ids)
     role_ids_to_remove = roles.pluck(:id) - role_ids
     user_roles.where("role_id IN(?)", role_ids_to_remove).destroy_all

db/migrate/20190227162128_remove_uniq_index_constraint_for_user_mobile.rb

@@ -0,0 +1,6 @@
+class RemoveUniqIndexConstraintForUserMobile < ActiveRecord::Migration
+  def change
+    remove_index :users, :mobile
+    add_index :users, :mobile
+  end
+end

db/schema.rb

@@ -11,7 +11,8 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20190201084112) do
+ActiveRecord::Schema.define(version: 20190227162128) do
+
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
   enable_extension "btree_gin"
@@ -207,6 +208,7 @@
     t.integer  "created_by_id"
     t.datetime "created_at",      null: false
     t.datetime "updated_at",      null: false
+    t.text     "item_specifics"
   end
 
   add_index "goodcity_requests", ["created_by_id"], name: "index_goodcity_requests_on_created_by_id", using: :btree
@@ -287,10 +289,12 @@
     t.datetime "deleted_at"
     t.integer  "offer_id"
     t.integer  "item_id"
+    t.integer  "order_id"
   end
 
   add_index "messages", ["item_id"], name: "index_messages_on_item_id", using: :btree
   add_index "messages", ["offer_id"], name: "index_messages_on_offer_id", using: :btree
+  add_index "messages", ["order_id"], name: "index_messages_on_order_id", using: :btree
   add_index "messages", ["sender_id"], name: "index_messages_on_sender_id", using: :btree
 
   create_table "offers", force: :cascade do |t|
@@ -364,7 +368,7 @@
     t.integer  "stockit_organisation_id"
     t.integer  "stockit_id"
     t.datetime "created_at"
-    t.datetime "updated_at",                          null: false
+    t.datetime "updated_at",                           null: false
     t.text     "description"
     t.integer  "stockit_activity_id"
     t.integer  "country_id"
@@ -389,8 +393,9 @@
     t.integer  "address_id"
     t.integer  "district_id"
     t.text     "cancellation_reason"
-    t.integer  "authorised_by_id"
     t.integer  "booking_type_id"
+    t.integer  "authorised_by_id"
+    t.string   "staff_note",              default: ""
   end
 
   add_index "orders", ["address_id"], name: "index_orders_on_address_id", using: :btree
@@ -427,6 +432,14 @@
   add_index "orders_packages", ["package_id"], name: "index_orders_packages_on_package_id", using: :btree
   add_index "orders_packages", ["updated_by_id"], name: "index_orders_packages_on_updated_by_id", using: :btree
 
+  create_table "orders_process_checklists", force: :cascade do |t|
+    t.integer "order_id"
+    t.integer "process_checklist_id"
+  end
+
+  add_index "orders_process_checklists", ["order_id"], name: "index_orders_process_checklists_on_order_id", using: :btree
+  add_index "orders_process_checklists", ["process_checklist_id"], name: "index_orders_process_checklists_on_process_checklist_id", using: :btree
+
   create_table "orders_purposes", force: :cascade do |t|
     t.integer "order_id"
     t.integer "purpose_id"
@@ -546,6 +559,7 @@
     t.string   "case_number"
     t.boolean  "allow_web_publish"
     t.integer  "received_quantity"
+    t.boolean  "last_allow_web_published"
   end
 
   add_index "packages", ["allow_web_publish"], name: "index_packages_on_allow_web_publish", using: :btree
@@ -599,6 +613,14 @@
     t.datetime "updated_at"
   end
 
+  create_table "process_checklists", force: :cascade do |t|
+    t.string  "text_en"
+    t.string  "text_zh_tw"
+    t.integer "booking_type_id"
+  end
+
+  add_index "process_checklists", ["booking_type_id"], name: "index_process_checklists_on_booking_type_id", using: :btree
+
   create_table "purposes", force: :cascade do |t|
     t.string   "name_en"
     t.string   "name_zh_tw"
@@ -700,9 +722,15 @@
     t.integer "user_id"
     t.integer "message_id"
     t.string  "state"
+    t.integer "order_id"
   end
 
+  add_index "subscriptions", ["message_id"], name: "index_subscriptions_on_message_id", using: :btree
   add_index "subscriptions", ["offer_id", "user_id", "message_id"], name: "offer_user_message", unique: true, using: :btree
+  add_index "subscriptions", ["offer_id"], name: "index_subscriptions_on_offer_id", using: :btree
+  add_index "subscriptions", ["order_id"], name: "index_subscriptions_on_order_id", using: :btree
+  add_index "subscriptions", ["state"], name: "index_subscriptions_on_state", using: :btree
+  add_index "subscriptions", ["user_id"], name: "index_subscriptions_on_user_id", using: :btree
 
   create_table "territories", force: :cascade do |t|
     t.string   "name_en"
@@ -745,7 +773,7 @@
   end
 
   add_index "users", ["image_id"], name: "index_users_on_image_id", using: :btree
-  add_index "users", ["mobile"], name: "index_users_on_mobile", unique: true, using: :btree
+  add_index "users", ["mobile"], name: "index_users_on_mobile", using: :btree
   add_index "users", ["permission_id"], name: "index_users_on_permission_id", using: :btree
 
   create_table "versions", force: :cascade do |t|
@@ -772,9 +800,14 @@
   add_foreign_key "beneficiaries", "identity_types"
   add_foreign_key "goodcity_requests", "orders"
   add_foreign_key "goodcity_requests", "package_types"
+  add_foreign_key "messages", "orders"
+  add_foreign_key "orders_process_checklists", "orders"
+  add_foreign_key "orders_process_checklists", "process_checklists"
   add_foreign_key "organisations", "countries"
   add_foreign_key "organisations", "districts"
   add_foreign_key "organisations", "organisation_types"
   add_foreign_key "organisations_users", "organisations"
   add_foreign_key "organisations_users", "users"
+  add_foreign_key "process_checklists", "booking_types"
+  add_foreign_key "subscriptions", "orders"
 end

lib/classes/organisations_user_builder.rb

@@ -21,8 +21,8 @@ def initialize(params)
     fail_with_error(I18n.t('organisations_user_builder.user.mobile.blank')) unless @mobile
   end
 
-  def build
-    @user = User.where(mobile: @mobile).first_or_create(@user_attributes)
+  def build(is_stock_app)
+    @user = build_user(is_stock_app)
     return fail_with_error(@user.errors) unless @user.valid?
     return fail_with_error(I18n.t('organisations_user_builder.organisation.not_found')) unless organisation
     if !user_belongs_to_organisation(@user)
@@ -35,6 +35,13 @@ def build
     end
   end
 
+  def build_user(is_stock_app)
+    @user = User.where(mobile: @mobile).first_or_initialize(@user_attributes)
+    @user.request_from_stock = is_stock_app
+    @user.save 
+    @user
+  end
+
   def update
     update_user
     @organisations_user.update(position: @position)

spec/controllers/api/v1/authentication_controller_spec.rb

@@ -38,7 +38,7 @@
 
     it "with blank mobile number" do
       post :signup, format: 'json', user_auth: { mobile: "", first_name: "Jake", last_name: "Deamon", address_attributes: {district_id: '1', address_type: 'Profile'} }
-      expect(parsed_body["errors"]).to eq( "Mobile can't be blank. Mobile is invalid" )
+      expect(parsed_body["errors"]).to eq("Mobile is invalid. Mobile can't be blank")
     end
 
   end

spec/lib/classes/organisations_user_builder_spec.rb

@@ -16,6 +16,10 @@
     FactoryBot.attributes_for(:user, :with_email, mobile:'+85252345678')
   end
 
+  let(:user_attributes_without_mobile) do 
+    FactoryBot.attributes_for(:user, :with_email)
+  end
+
   let(:update_user_attributes) do
     FactoryBot.attributes_for(:user, :with_email, mobile:'+85252345678', last_name: "Cooper")
   end
@@ -25,6 +29,10 @@
     FactoryBot.attributes_for(:organisations_user, organisation_id: "#{organisation.id}", position: "#{position}" , user_attributes: user_attributes)
   end
 
+  let(:organisation_user_params_without_mobile) do 
+    FactoryBot.attributes_for(:organisations_user, organisation_id: "#{organisation.id}", position: "#{position}" , user_attributes: user_attributes_without_mobile)
+  end
+
   let(:update_organisations_user_params) do
     FactoryBot.attributes_for(:organisations_user, id: "#{organisations_user.id}", position: "Updated position", user_attributes: update_user_attributes)
   end
@@ -34,6 +42,7 @@
   let(:position) { 'Admin' }
 
   let(:organisations_user_builder) { OrganisationsUserBuilder.new(organisations_user_params.stringify_keys) }
+  let(:organisations_user_builder_without_mobile) { OrganisationsUserBuilder.new(organisation_user_params_without_mobile.stringify_keys) }
   let(:update_organisations_user_builder) { OrganisationsUserBuilder.new(update_organisations_user_params.stringify_keys) }
   let(:mobile) { '+85251111111' }
   let!(:role) { create :charity_role }
@@ -53,21 +62,29 @@
       User.current_user = user
     }
 
+    context 'for stock app' do 
+      it "adds new user if mobile is blank and associates it with organisation" do
+        expect{
+          organisations_user_builder_without_mobile.build(true)
+        }.to change{User.count}.by(1).and change{OrganisationsUser.count}.by(1)
+      end
+    end
+
     it "adds new user if mobile does not exist and associates it with organisation" do
       expect{
-        organisations_user_builder.build
+        organisations_user_builder.build(false)
       }.to change{User.count}.by(1).and change{OrganisationsUser.count}.by(1)
     end
 
     it "do not add user to organisation if mobile number already in organisation" do
       organisations_user1 = create :organisations_user, user: user1, organisation: organisation
-      expect(organisations_user_builder.build).to eq({ 'result' => false, 'errors' => "Mobile has already been taken" })
+      expect(organisations_user_builder.build(false)).to eq({ 'result' => false, 'errors' => "Mobile has already been taken" })
       expect(OrganisationsUser.count).to eq(1)
     end
 
     it "associates charity_role to user if user added in organisation" do
       expect{
-        organisations_user_builder.build
+        organisations_user_builder.build(false)
       }.to change{OrganisationsUser.count}.by(1)
       expect(OrganisationsUser.last.user.roles).to include(role)
     end

spec/models/user_spec.rb

@@ -50,6 +50,26 @@
           expect(user.tap(&:valid?).errors[:mobile]).to include("has already been taken")
         end
       end
+
+      context "for stock" do 
+        it 'allows blank'do 
+          user = User.new(mobile: '')
+          user.request_from_stock = true
+          expect(user.valid?).to be_truthy
+        end
+
+        it 'allows valid mobile number' do
+          user = User.new(mobile: '+85251234567')
+          user.request_from_stock = true
+          expect(user.valid?).to be_truthy 
+        end
+
+        it 'do not allows invalid hk number' do 
+          user = User.new(mobile: '+44123456675')
+          user.request_from_stock = true
+          expect(user.valid?).to be_falsey
+        end
+      end
       it { is_expected.to allow_value('+85251234567').for(:mobile) }
       it { is_expected.to allow_value('+85261234567').for(:mobile) }
       it { is_expected.to allow_value('+85291234567').for(:mobile) }