Skip to content

fix: log exceptions during source distance calculation #3422

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

fix: log exceptions during source distance calculation #3422

wants to merge 7 commits into from

Conversation

SamJUK
Copy link

@SamJUK SamJUK commented Apr 24, 2025

Log exceptions encountered during In Stock Pickup source distance calculation instead of silently discarding them.

Fixes: #3420

@engcom-Hotel
Copy link
Collaborator

@magento run all tests

Copilot
Copilot AI reviewed Jul 30, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds error logging to the source distance calculation functionality in the In Store Pickup module. When an exception occurs during address-to-coordinates conversion, it will now be logged instead of silently failing.

  • Adds logging of LocalizedException during source distance calculation
  • Introduces LoggerInterface dependency injection with fallback to ObjectManager

InventoryInStorePickup/Model/SearchRequest/Area/GetDistanceToSources.php Outdated
* @param GetOrderedDistanceToSources $getOrderedDistanceToSources
* @param AddressInterfaceFactory $addressInterfaceFactory
* @param Pipeline $searchTermPipeline
* @param Logger $logger
Copy link
Preview

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

The @param documentation shows 'Logger' but the actual parameter type is 'LoggerInterface'. This should be '@param LoggerInterface $logger' to match the type hint.

Suggested change
* @param Logger $logger
* @param LoggerInterface $logger

Copilot uses AI. Check for mistakes.

@magento magento deleted a comment from Copilot AI Jul 30, 2025
engcom-Hotel
engcom-Hotel requested changes Jul 30, 2025
View reviewed changes
Copy link
Collaborator

@engcom-Hotel engcom-Hotel left a comment

Choose a reason for hiding this comment

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

Hello @SamJUK,

Thanks for the contribution!

Please look into the below review comments. And also check the review comments made by the Copilot.

The failed tests seems flaky to me, will re-run to be double sure.

Thanks

InventoryInStorePickup/Model/SearchRequest/Area/GetDistanceToSources.php Outdated
AddressInterfaceFactory $addressInterfaceFactory,
Pipeline $searchTermPipeline
Pipeline $searchTermPipeline,
LoggerInterface $logger
Copy link
Collaborator

Choose a reason for hiding this comment

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

$logger should be null by default

Suggested change
LoggerInterface $logger
LoggerInterface $logger=null

@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests B2B, Functional Tests CE, Integration Tests, Static Tests, Unit Tests

@engcom-Hotel engcom-Hotel moved this to Changes Requested in Inventory - Pull Request Progress Sep 3, 2025
@engcom-Hotel
Copy link
Collaborator

@magento run all tests

@engcom-Hotel
Copy link
Collaborator

@magento run all tests

@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE

engcom-Hotel
engcom-Hotel approved these changes Sep 4, 2025
View reviewed changes
Copy link
Collaborator

@engcom-Hotel engcom-Hotel left a comment

Choose a reason for hiding this comment

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

Failed tests seems flaky to me, hence approving the PR.

@engcom-Hotel engcom-Hotel moved this from Changes Requested to Reviewer Approved in Inventory - Pull Request Progress Sep 4, 2025
@engcom-Hotel engcom-Hotel added Progress: approved Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it and removed Progress: needs update labels Sep 4, 2025
@engcom-Hotel engcom-Hotel self-assigned this Sep 8, 2025
@engcom-Hotel
Copy link
Collaborator

@magento run all tests

@engcom-Hotel engcom-Hotel moved this from Reviewer Approved to Acceptance Testing in Inventory - Pull Request Progress Sep 16, 2025
@engcom-Hotel
Copy link
Collaborator

Hello @SamJUK,

Please find below the QA feedback for this PR:

✔️ QA Passed

Preconditions:

  1. Google API as Distance Provider
  2. Google API Key with invalid permissions (eg, referrer restrictions applied).

Manual testing scenario:

  1. Configure source selection as preconditions
  2. Navigate to checkout, and attempt a pick up at store address lookup

Actual Result: ✔️

  • Error message in logs outlining, when the lookup failed

Before

  • Error logs are empty, requiring a debugger/source changes to identify the failure reason

After

  • Error message in logs outlining, when the lookup failed

Since some automated tests are failing, hence checking this PR further.

Thanks

@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, Integration Tests

@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE

@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests B2B

@engcom-Hotel
Copy link
Collaborator

After investigating the B2B functional test failure, I found that the test is flaky.

Known Issues:

Flaky Issues:

  • B2B-1505

Hence moving forward with this PR.

@engcom-Hotel engcom-Hotel moved this from Acceptance Testing to Ready for Delivery in Inventory - Pull Request Progress Sep 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@engcom-Hotel engcom-Hotel engcom-Hotel approved these changes

Assignees

@engcom-Hotel engcom-Hotel

Labels
Progress: accept Progress: approved Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it
Projects
Inventory - Pull Request Progress
Status: Ready for Delivery
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

In store pickup silently discards exceptions
2 participants
@SamJUK @engcom-Hotel