taxjar / taxjar-magento2-extension

Magento 2 Sales Tax Extension by TaxJar
http://www.taxjar.com/guides/integrations/magento2/
Open Software License 3.0
22 stars 29 forks source link

ISSUE 341: Fix order grid filter error #343

Closed sethobey closed 1 year ago

sethobey commented 1 year ago

Context

Closes #341

Customer identified regression in sales order grid when filtering by date. Magento core code does not prefix created_at filter value with table alias leading to ambiguous query.

Description

Performance

Adds overhead of additional type-checking operation when querying for any UI collections, but should still only introduce negligible latency.

Testing

Order grid

  1. Navigate to sales_order_grid
  2. Add filter on Purchase Date (created_at) and/or Synced To TaxJar (tj_salestax_sync_date)
  3. Apply filter
  4. Observe filter is successfully applied, grid re-loaded

Creditmemo grid

  1. Navigate to sales_creditmemo_grid
  2. Add filter on Created (created_at) and/or Synced To TaxJar (tj_salestax_sync_date)
  3. Apply filter
  4. Observe filter is successfully applied, grid re-loaded

Sales Order Creditmemo grid

  1. Navigate to Sales Order > Creditmemos
  2. Add creditmemo filter on Created (created_at) and/or Synced To TaxJar (tj_salestax_sync_date)
  3. Apply filter
  4. Observe filter is successfully applied, grid re-loaded
image

Versions

sethobey commented 1 year ago

Should we also remove any unnecessary code from AddTjSyncDateToGrid.php. Specifically are the addFilterToMap method calls still necessary?

@dallendalton Good call out!

In consideration of your comment I refactored the AddTjSyncDateToGrid::class to remove duplicated code, then wrote some integration tests to validate the joins. In the process, I identified another potential ambiguous query (order_id) when filtering the sales-order-creditmemo grid which is also now addressed in BeforeApplyFilters::class plugin.

While I was at it, I updated our sales-related ui_component XML overrides to use Magento's default date column types, allowing us to remove 3 (unnecessary) custom UI component column definitions. This also now enables filtering on TJ sync date values.