Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save deleted_by user when deleting products #12431

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/api/v0/products_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def destroy
authorize! :delete, Spree::Product
@product = product_finder.find_product
authorize! :delete, @product
@product.destroy
@product.destroy(deleted_by: current_api_user)
render json: @product, serializer: Api::Admin::ProductSerializer, status: :no_content
end

Expand Down
6 changes: 6 additions & 0 deletions app/models/spree/product.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class Product < ApplicationRecord
searchable_scopes :active, :with_properties

belongs_to :supplier, class_name: 'Enterprise', optional: false, touch: true
belongs_to :deleted_by, class_name: "Spree::User", optional: true

has_one :image, class_name: "Spree::Image", as: :viewable, dependent: :destroy

Expand Down Expand Up @@ -260,6 +261,11 @@ def import_date
variants.map(&:import_date).compact.max
end

def destroy(deleted_by: nil)
update_attribute(:deleted_by, deleted_by) if deleted_by.present?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking we shouldn't have a default for deleted_by to enforce recording a user when deleting a product. But I am pretty sure it would blow up in various unexpected places.

super()
end

def destruction
transaction do
touch_distributors
Expand Down
2 changes: 1 addition & 1 deletion app/reflexes/products_reflex.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def delete_product
product = product_finder(id).find_product
authorize! :delete, product

if product.destroy
if product.destroy(deleted_by: current_user)
flash[:success] = I18n.t('admin.products_v3.delete_product.success')
else
flash[:error] = I18n.t('admin.products_v3.delete_product.error')
Expand Down
6 changes: 6 additions & 0 deletions db/migrate/20240502011542_add_deleted_by_to_spree_products.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class AddDeletedByToSpreeProducts < ActiveRecord::Migration[7.0]
def change
add_column :spree_products, :deleted_by_id, :integer, null: true
add_foreign_key :spree_products, :spree_users, column: :deleted_by_id
end
end
12 changes: 7 additions & 5 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: fix up discrepancies here

# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.0].define(version: 2024_04_30_075133) do
ActiveRecord::Schema[7.0].define(version: 2024_05_02_011542) do
# These are extensions that must be enabled in order to support this database
enable_extension "pg_stat_statements"
enable_extension "plpgsql"
Expand Down Expand Up @@ -101,7 +101,7 @@
t.datetime "terms_and_conditions_accepted_at", precision: nil
t.string "first_name", default: "", null: false
t.string "last_name", default: "", null: false
t.boolean "created_manually", default: false
t.boolean "created_manually", default: false, null: false
t.index ["bill_address_id"], name: "index_customers_on_bill_address_id"
t.index ["created_manually"], name: "index_customers_on_created_manually"
t.index ["email"], name: "index_customers_on_email"
Expand Down Expand Up @@ -220,9 +220,9 @@
t.string "email_address", limit: 255
t.boolean "require_login", default: false, null: false
t.boolean "allow_guest_orders", default: true, null: false
t.boolean "allow_order_changes", default: false, null: false
t.text "invoice_text"
t.boolean "display_invoice_logo", default: false
t.boolean "allow_order_changes", default: false, null: false
t.boolean "enable_subscriptions", default: false, null: false
t.integer "business_address_id"
t.boolean "show_customer_names_to_suppliers", default: false, null: false
Expand Down Expand Up @@ -301,7 +301,7 @@
t.bigint "order_id"
t.integer "number"
t.jsonb "data"
t.date "date", default: -> { "CURRENT_TIMESTAMP" }
t.date "date", default: -> { "now()" }
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.boolean "cancelled", default: false, null: false
Expand Down Expand Up @@ -701,6 +701,7 @@
t.integer "primary_taxon_id"
t.boolean "inherits_properties", default: true, null: false
t.string "sku", limit: 255, default: "", null: false
t.integer "deleted_by_id"
t.index ["deleted_at"], name: "index_products_on_deleted_at"
t.index ["name"], name: "index_products_on_name"
t.index ["primary_taxon_id"], name: "index_spree_products_on_primary_taxon_id"
Expand Down Expand Up @@ -1109,7 +1110,7 @@

create_table "vouchers", force: :cascade do |t|
t.string "code", limit: 255, null: false
t.datetime "expiry_date"
t.datetime "expiry_date", precision: nil
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.bigint "enterprise_id"
Expand Down Expand Up @@ -1203,6 +1204,7 @@
add_foreign_key "spree_products", "spree_shipping_categories", column: "shipping_category_id", name: "spree_products_shipping_category_id_fk"
add_foreign_key "spree_products", "spree_tax_categories", column: "tax_category_id", name: "spree_products_tax_category_id_fk"
add_foreign_key "spree_products", "spree_taxons", column: "primary_taxon_id", name: "spree_products_primary_taxon_id_fk"
add_foreign_key "spree_products", "spree_users", column: "deleted_by_id"
add_foreign_key "spree_return_authorizations", "spree_orders", column: "order_id", name: "spree_return_authorizations_order_id_fk"
add_foreign_key "spree_roles_users", "spree_roles", column: "role_id", name: "spree_roles_users_role_id_fk"
add_foreign_key "spree_roles_users", "spree_users", column: "user_id", name: "spree_roles_users_user_id_fk"
Expand Down
1 change: 1 addition & 0 deletions spec/controllers/api/v0/products_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
expect(response.status).to eq(204)
expect { product.reload }.not_to raise_error
expect(product.deleted_at).not_to be_nil
expect(product.deleted_by).to eq current_api_user
end

it "is denied access to deleting another enterprises' product" do
Expand Down
10 changes: 10 additions & 0 deletions spec/models/spree/product_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@
expect(product.deleted_at).not_to be_nil
expect(product.variants.all? { |v| !v.deleted_at.nil? }).to be_truthy
end

it "should set deleted_by user if provided" do
user = create(:user)
product.destroy(deleted_by: user)
expect(product.reload.deleted_by).to eq user
end
end
end

Expand Down Expand Up @@ -340,6 +346,10 @@
it "removes variants from order cycles" do
expect { product.destroy }.to change { ExchangeVariant.count }
end

#todo: it "aborts if exchange variants failed to delete"

Check warning on line 350 in spec/models/spree/product_spec.rb

View workflow job for this annotation

GitHub Actions / runner / rubocop

[rubocop] reported by reviewdog 🐶 Missing space after `#`. Raw Output: spec/models/spree/product_spec.rb:350:9: C: Layout/LeadingCommentSpace: Missing space after `#`.
# expect(product.deleted_at).to be_nil
# expect(product.deleted_by).to be_nil
end

it "updates units when saved change to variant unit" do
Expand Down
1 change: 1 addition & 0 deletions spec/reflexes/products_reflex_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
subject.run(action_name)
product.reload
expect(product.deleted_at).not_to be_nil
expect(product.deleted_by).to eq current_user
expect(flash[:success]).to eq('Successfully deleted the product')
end

Expand Down
Loading