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

#VP-2481 add shipment item select #157

Merged
merged 6 commits into from Jun 11, 2020

Conversation

kostyrin
Copy link
Contributor

No description provided.

toolbarCommands: [
{
name: "orders.commands.add-selected", icon: 'fa fa-plus',
executeMethod: function (blade) {
Copy link
Contributor

Choose a reason for hiding this comment

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

MAJOR "blade" hides or potentially hides a variable declared in an outer scope at line 3. rule

blade.toolbarCommands = [
{
name: "orders.commands.add-item", icon: 'fa fa-plus',
executeMethod: function (blade) {
Copy link
Contributor

Choose a reason for hiding this comment

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

MAJOR "blade" hides or potentially hides a variable declared in an outer scope at line 4. rule

Copy link
Contributor

@yecli yecli left a comment

Choose a reason for hiding this comment

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

Could we add more than 1 line item to the shipment through the interface?
If so, could more than 1 line item be saved in one shipment item?
As I understand, one shipmentItem has only one LineItemId

@kostyrin
Copy link
Contributor Author

kostyrin commented May 28, 2020

yes, we could add more then 1 item to shipment items.

{
ModelLineItem = shipmentItem.LineItem;
}
ModelLineItem = shipmentItem.LineItem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you test these scenarios to be confident that you don't introduce regress bugs?

  • save new order with the new shipment with items
  • update exists order shipment with new shipment items
  • update exists order with the new shipment with items

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll check it.

Copy link
Contributor Author

@kostyrin kostyrin Jun 8, 2020

Choose a reason for hiding this comment

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

I guess, there is no scenario - save new order with the new shipment with items.
How to do it? Without exists LineItemId?
there are no links with Order.LineItemId.
Microsoft.Data.SqlClient.SqlException (0x80131904): Cannot insert the value NULL into column 'LineItemId', table 'VirtoCommerce3.dbo.OrderShipmentItem'; column does not allow nulls. INSERT fails.

@akak1977 saving new shipment - fixed

Copy link
Contributor

@akak1977 akak1977 left a comment

Choose a reason for hiding this comment

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

This really does not working with newly created shipments.
image
Create shipment, error occires while order still not saved.

@vc-ci
Copy link
Contributor

vc-ci commented Jun 11, 2020

SonarQube analysis reported 4 issues

  • CRITICAL 1 critical
  • MAJOR 2 major
  • MINOR 1 minor

Watch the comments in this conversation to review them.

2 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. CRITICAL CustomerOrderEntity.cs#L259: Rename parameter 'operation' to 'target' to match the base class declaration. rule
  2. MINOR CustomerOrderEntity.cs#L259: Refactor this method to reduce its Cognitive Complexity from 22 to the 15 allowed. rule

@akak1977 akak1977 merged commit 9390ebf into dev Jun 11, 2020
@akak1977 akak1977 deleted the feature/3.0.0/VP-2481-add-shipment-item-select branch June 11, 2020 05:42
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.

None yet

5 participants