Skip to content
This repository was archived by the owner on Dec 19, 2019. It is now read-only.

graphQl-961: ShippingAddressInput.postcode: String, is not required by Schema #969

Merged
merged 13 commits into from
Nov 6, 2019

Conversation

kisroman
Copy link
Contributor

Description (*)

Fixed Issues (if relevant)

  1. ShippingAddressInput.postcode: String, is not required by Schema #961

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@kisroman kisroman requested a review from paliarush as a code owner September 27, 2019 10:19
@TomashKhamlai TomashKhamlai added the Progress: ready for qa Add this in any case when you need some feedback, even if automated tests are failing label Sep 27, 2019
@lenaorobei lenaorobei added the Event: mageconf19 MageConf 2019 Contribution Day label Sep 27, 2019
@kisroman kisroman added Progress: dev in progress and removed Progress: ready for qa Add this in any case when you need some feedback, even if automated tests are failing labels Sep 27, 2019
@kisroman kisroman added the Progress: ready for qa Add this in any case when you need some feedback, even if automated tests are failing label Sep 29, 2019
@TomashKhamlai TomashKhamlai added QA in progress We are checking QA passed and removed Progress: ready for qa Add this in any case when you need some feedback, even if automated tests are failing QA in progress We are checking labels Sep 30, 2019
) {
$this->quoteAddressFactory = $quoteAddressFactory;
$this->getCustomerAddress = $getCustomerAddress;
$this->addressHelper = $addressHelper;
$this->countryInformationAcquirer = $countryInformationAcquirer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

* @param array $errors
* @return string
*/
private function getAddressErrors(array $errors): string
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add also test case with multiple errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@lenaorobei lenaorobei left a comment

Choose a reason for hiding this comment

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

Please address comments.

@TomashKhamlai TomashKhamlai added QA failed and removed QA in progress We are checking labels Oct 24, 2019
@lenaorobei
Copy link
Contributor

@TomashKhamlai query you are trying to test has syntax issues. shipping_addresses should be array.

shipping_addresses: [
        {
          address: {
            street: ["Streer"]
            country_code: "US"
            firstname: "John"
            lastname: "Doubledo"
            telephone: ""
            save_in_address_book: true
            city: "Culver city"
          }
        }
      ]

@lenaorobei
Copy link
Contributor

@magento run Functional Tests

@magento-engcom-team
Copy link
Contributor

Hi @lenaorobei, thank you for the review.
ENGCOM-6170 has been created to process this Pull Request
✳️ @lenaorobei, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@TomashKhamlai TomashKhamlai added QA in progress We are checking and removed QA failed labels Oct 28, 2019
@TomashKhamlai
Copy link
Contributor

@lenaorobei, I've tried with array syntax also (usually both variants work as expected)

Request:

mutation {
  setShippingAddressesOnCart(
    input: {
      cart_id: "02GYdVCktyqGo8LfEkoBwPbkCLxt3lL9"
      shipping_addresses: [
        {
          address: {
            country_code: "US"
            firstname: "John"
            lastname: "Doubledo"
            telephone: ""
            save_in_address_book: true
            city: "Culver city"
          }
        }
      ]
    }
  ) {
    cart {
      email
      shipping_addresses {
        postcode
        firstname
        lastname
        telephone
        selected_shipping_method {
          carrier_code
          method_code
        }
        available_shipping_methods {
          carrier_code
          method_code
        }
      }
    }
  }
}

Response:

{
  "errors": [
    {
      "debugMessage": "Notice: Undefined index: street in /var/www/html/graphqlce/app/code/Magento/QuoteGraphQl/Model/Cart/QuoteAddressFactory.php on line 93",
      "message": "Internal server error",
      "extensions": {
        "category": "internal"
      },
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "setShippingAddressesOnCart"
      ]
    }
  ],
  "data": {
    "setShippingAddressesOnCart": null
  }
}

@TomashKhamlai TomashKhamlai added QA failed and removed QA in progress We are checking labels Oct 28, 2019
$addressInput['region_code'] = $addressInput['region'];
}
}

$maxAllowedLineCount = $this->addressHelper->getStreetLines();
if (is_array($addressInput['street']) && count($addressInput['street']) > $maxAllowedLineCount) {
Copy link
Contributor

@TomashKhamlai TomashKhamlai Oct 28, 2019

Choose a reason for hiding this comment

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

I think that $addressInput['street'] can be empty and this should be checked here or somewhere else before these lines can be executed.

@lenaorobei
Copy link
Contributor

@magento run Unit Tests

@TomashKhamlai
Copy link
Contributor

TomashKhamlai commented Oct 28, 2019

Created a separate issue #1033 for the case when the street parameter was omitted.

@ghost
Copy link

ghost commented Nov 6, 2019

Hi @kisroman, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

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

Successfully merging this pull request may close these issues.

4 participants