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

Fix error when creating new product on Admin panel #6250

Closed

Conversation

tsara27
Copy link
Contributor

@tsara27 tsara27 commented Oct 26, 2020

What? Why?

Closes #6117

I am adding conditional to check variants first before updating the stock items when creating a product.

What should we test?

Test the new product creation on admin panel.

How to do:

Login as an admin.
Go to products.
Create a new product.
Fill other required details, leave the unit value empty.

Changelog Category: Fixed

@tsara27 tsara27 marked this pull request as draft October 26, 2020 04:54
@andrewpbrett
Copy link
Contributor

Hi @tsara27, thanks for submitting this. It looks like some tests now fail as a result, so we'll need those to pass before we can consider merging this.

The on_hand and on_demand fields probably don't need to be changed to fix this, since those have to do with variant stock. I think the fix will be somewhere in the product model or controller.

@tsara27 tsara27 force-pushed the fix-invalid-unit-value branch 2 times, most recently from 97444e2 to c2d130e Compare October 31, 2020 07:32
@tsara27 tsara27 marked this pull request as ready for review October 31, 2020 07:33
@tsara27 tsara27 force-pushed the fix-invalid-unit-value branch from 5c8ebf7 to 054abc5 Compare October 31, 2020 07:40
@tsara27 tsara27 marked this pull request as draft October 31, 2020 13:16
@tsara27 tsara27 force-pushed the fix-invalid-unit-value branch from f8931e7 to 31a8fd7 Compare October 31, 2020 13:42
@tsara27 tsara27 marked this pull request as ready for review October 31, 2020 13:42
There are multiple forms of pluralisation here, with different translations for each.

перевод сложен !
@andrewpbrett
Copy link
Contributor

Hi @tsara27 - this is looking better now! It looks like some specs are still failing because the factory creates a product without specifying a unit_value and so the object fails validation. You could take a look at the product factory and feel free to update it to provide a unit_value and I think that would take care of a few of them.

I did checkout this branch locally and confirm that the tests fail locally for me and not just on Semaphore.

…slavic-plurals

Add missing translation keys for Eastern Slavic plurals
@tsara27
Copy link
Contributor Author

tsara27 commented Nov 1, 2020

Hi @tsara27 - this is looking better now! It looks like some specs are still failing because the factory creates a product without specifying a unit_value and so the object fails validation. You could take a look at the product factory and feel free to update it to provide a unit_value and I think that would take care of a few of them.

I did checkout this branch locally and confirm that the tests fail locally for me and not just on Semaphore.

Thank you Andrew! I am fixing all specs now.

andrewpbrett and others added 24 commits November 20, 2020 10:41
SIMPLE: added scripts for running test suite in quiet mode
…d-truncate-html-gem

Remove unused truncate_html gem
Move Spree::Admin::BaseController to Admin::BaseController
Add task to print Subscriptions debug info
…vert-6277-controllers

Revert "Move Spree::Admin::BaseController to Admin::BaseController"
@@ -38,7 +38,7 @@

describe "and a credit card is provided" do
before do
params[:order][:payments_attributes].first[:source_attributes] = {number: "4444333322221111"}
params[:order][:payments_attributes].first[:source_attributes] = { number: "4444333322221111" }
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [105/100]

let!(:order_dist_1_complete) { create(:order, distributor: order_dist_1.distributor, state: 'complete', completed_at: Time.zone.today - 7.days) }
let!(:order_dist_1_credit_owed) { create(:order, distributor: order_dist_1.distributor, payment_state: 'credit_owed', completed_at: Time.zone.today) }
let!(:order_dist_1_complete) { create(:order, distributor: order_dist_1.distributor, state: 'complete', completed_at: Time.zone.today - 7.days) }
let!(:order_dist_1_credit_owed) { create(:order, distributor: order_dist_1.distributor, payment_state: 'credit_owed', completed_at: Time.zone.today) }
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [160/100]

}
}
context "when there are four orders with different properties set" do
let!(:order_dist_1) { create(:order_with_distributor, email: "[email protected]") }
let!(:order_dist_2) { create(:order_with_totals_and_distribution) }
let!(:order_dist_1_complete) { create(:order, distributor: order_dist_1.distributor, state: 'complete', completed_at: Time.zone.today - 7.days) }
let!(:order_dist_1_credit_owed) { create(:order, distributor: order_dist_1.distributor, payment_state: 'credit_owed', completed_at: Time.zone.today) }
let!(:order_dist_1_complete) { create(:order, distributor: order_dist_1.distributor, state: 'complete', completed_at: Time.zone.today - 7.days) }
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [155/100]

context "price parsing" do
before(:each) do
I18n.locale = I18n.default_locale
I18n.backend.store_translations(:de, { number: { currency: { format: { delimiter: '.', separator: ',' } } } })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.
Metrics/LineLength: Line is too long. [118/100]

Spree::Property.where(name: 'foo').first_or_create!(presentation: "Foo's Presentation Name")
product.set_property('foo', 'value1')
product.set_property('bar', 'value2')
expect(Spree::Property.where(name: 'foo').first.presentation).to eq "Foo's Presentation Name"
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [101/100]

@@ -103,8 +103,8 @@

it 'should destroy backordered units first' do
allow(shipment).to receive_messages(inventory_units_for: [build(:inventory_unit, variant_id: variant.id, state: 'backordered'),
build(:inventory_unit, variant_id: variant.id, state: 'on_hand'),
build(:inventory_unit, variant_id: variant.id, state: 'backordered')])
build(:inventory_unit, variant_id: variant.id, state: 'on_hand'),
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [131/100]

@@ -60,7 +60,7 @@
it "cannot transition to address without any line items" do
expect(order.line_items).to be_blank
expect(lambda { order.next! }).to raise_error(StateMachine::InvalidTransition,
/#{Spree.t(:there_are_no_items_for_this_order)}/)
/#{Spree.t(:there_are_no_items_for_this_order)}/)
Copy link

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [101/100]

end

it "doesn't convert an empty array to nil" do
expect(parse_query_parameters('key[]')).to eq({ "key" => [] })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

it "strips nils from nested arrays" do
expect(
parse_query_parameters('key1[key2][]=value&key1[key2][]')
).to eq({ "key1" => { "key2" => ["value"] } })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.


describe ActionDispatch::Request do
it "strips nils from arrays" do
expect(parse_query_parameters('key[]=value&key[]')).to eq({ "key" => ["value"] })
Copy link

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

@andrewpbrett
Copy link
Contributor

Hi @tsara27 - I tried to rebase this branch to resolve the conflicts from removing spree and I'm afraid I made a big mess of it. I think it happened because I merged master in instead of replaying these changes on top of the latest... and now I'm not able to undo the merge. I made a fresh PR instead. Sorry about that! I'm going to close this issue so we can use the new one instead.

@tsara27
Copy link
Contributor Author

tsara27 commented Dec 1, 2020

Hello @andrewpbrett,

No problem. Thank you for helping me recreating a fresh PR. 🙂

I did not have time for few weeks to work on this project because I got project from my full time work to finish everything before the holiday. 😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid unit_value on product creation returns 500 instead of 422