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

Core Data: Replace unnecessary fetch requests for upserting orders and order search results #14128

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from
8 changes: 8 additions & 0 deletions Storage/Storage/Tools/StorageType+Extensions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ public extension StorageType {

// MARK: - Orders

/// Retrieves the Stored Orders given the IDs.
///
func loadOrders(siteID: Int64, orderIDs: [Int64]) -> [Order] {
let predicate = NSPredicate(format: "orderID in %@", orderIDs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to add the siteID to the predicate? With the current predicate, I am afraid that we will end up loading orders from other sites with the same order ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I updated the predicate in a27533c.

let descriptor = NSSortDescriptor(keyPath: \Order.orderID, ascending: false)
return allObjects(ofType: Order.self, matching: predicate, sortedBy: [descriptor])
}

/// Retrieves the Stored Order.
///
func loadOrder(siteID: Int64, orderID: Int64) -> Order? {
Expand Down
24 changes: 24 additions & 0 deletions Storage/StorageTests/Tools/StorageTypeExtensionsTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,30 @@ final class StorageTypeExtensionsTests: XCTestCase {
XCTAssertEqual(site, storedSite)
}

func test_loadOrders_list_by_siteID_and_orderIDs() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: We could update this test by adding an order with a different site ID other than sampleSiteID to validate that only orders from the given siteID are loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the test in a27533c.

// Given
let orderID1: Int64 = 123
let order1 = storage.insertNewObject(ofType: Order.self)
order1.siteID = sampleSiteID
order1.orderID = orderID1

let orderID2: Int64 = 125
let order2 = storage.insertNewObject(ofType: Order.self)
order2.siteID = sampleSiteID
order2.orderID = orderID2

let orderID3: Int64 = 126
let order3 = storage.insertNewObject(ofType: Order.self)
order3.siteID = sampleSiteID
order3.orderID = orderID3

// When
let storedOrders = storage.loadOrders(siteID: sampleSiteID, orderIDs: [orderID1, orderID3])

// Then
XCTAssertEqual(storedOrders, [order3, order1])
}

func test_loadOrder_by_siteID_and_orderID() throws {
// Given
let orderID: Int64 = 123
Expand Down
20 changes: 8 additions & 12 deletions Yosemite/Yosemite/Stores/Order/OrdersUpsertUseCase.swift
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,10 @@ struct OrdersUpsertUseCase {
///
private func handleOrderItems(_ readOnlyOrder: Networking.Order, _ storageOrder: Storage.Order, _ storage: StorageType) {
var storageItem: Storage.OrderItem
let siteID = readOnlyOrder.siteID
let orderID = readOnlyOrder.orderID

// Upsert the items from the read-only order
for readOnlyItem in readOnlyOrder.items {
if let existingStorageItem = storage.loadOrderItem(siteID: siteID, orderID: orderID, itemID: readOnlyItem.itemID) {
if let existingStorageItem = storageOrder.orderItemsArray.first(where: { $0.itemID == readOnlyItem.itemID }) {
existingStorageItem.update(with: readOnlyItem)
storageItem = existingStorageItem
} else {
Expand Down Expand Up @@ -137,11 +135,10 @@ struct OrdersUpsertUseCase {
/// Updates, inserts, or prunes the provided StorageOrderItem's taxes using the provided read-only OrderItem
///
private func handleOrderItemTaxes(_ readOnlyItem: Networking.OrderItem, _ storageItem: Storage.OrderItem, _ storage: StorageType) {
let itemID = readOnlyItem.itemID

// Upsert the taxes from the read-only orderItem
for readOnlyTax in readOnlyItem.taxes {
if let existingStorageTax = storage.loadOrderItemTax(itemID: itemID, taxID: readOnlyTax.taxID) {
if let existingStorageTax = storageItem.taxes?.first(where: { $0.taxID == readOnlyTax.taxID }) {
existingStorageTax.update(with: readOnlyTax)
} else {
let newStorageTax = storage.insertNewObject(ofType: Storage.OrderItemTax.self)
Expand All @@ -164,7 +161,7 @@ struct OrdersUpsertUseCase {
private func handleOrderCoupons(_ readOnlyOrder: Networking.Order, _ storageOrder: Storage.Order, _ storage: StorageType) {
// Upsert the coupons from the read-only order
for readOnlyCoupon in readOnlyOrder.coupons {
if let existingStorageCoupon = storage.loadOrderCoupon(siteID: readOnlyOrder.siteID, couponID: readOnlyCoupon.couponID) {
if let existingStorageCoupon = storageOrder.coupons?.first(where: { $0.couponID == readOnlyCoupon.couponID }) {
existingStorageCoupon.update(with: readOnlyCoupon)
} else {
let newStorageCoupon = storage.insertNewObject(ofType: Storage.OrderCoupon.self)
Expand All @@ -187,7 +184,7 @@ struct OrdersUpsertUseCase {
private func handleOrderFees(_ readOnlyOrder: Networking.Order, _ storageOrder: Storage.Order, _ storage: StorageType) {
// Upsert the coupons from the read-only order
for readOnlyFee in readOnlyOrder.fees {
if let existingStorageFee = storage.loadOrderFeeLine(siteID: readOnlyOrder.siteID, feeID: readOnlyFee.feeID) {
if let existingStorageFee = storageOrder.fees?.first(where: { $0.feeID == readOnlyFee.feeID }) {
existingStorageFee.update(with: readOnlyFee)
} else {
let newStorageFee = storage.insertNewObject(ofType: Storage.OrderFeeLine.self)
Expand All @@ -210,7 +207,7 @@ struct OrdersUpsertUseCase {
private func handleOrderRefundsCondensed(_ readOnlyOrder: Networking.Order, _ storageOrder: Storage.Order, _ storage: StorageType) {
// Upsert the refunds from the read-only order
for readOnlyRefund in readOnlyOrder.refunds {
if let existingStorageRefund = storage.loadOrderRefundCondensed(siteID: readOnlyOrder.siteID, refundID: readOnlyRefund.refundID) {
if let existingStorageRefund = storageOrder.refunds?.first(where: { $0.refundID == readOnlyRefund.refundID }) {
existingStorageRefund.update(with: readOnlyRefund)
} else {
let newStorageRefund = storage.insertNewObject(ofType: Storage.OrderRefundCondensed.self)
Expand All @@ -233,7 +230,7 @@ struct OrdersUpsertUseCase {
private func handleOrderShippingLines(_ readOnlyOrder: Networking.Order, _ storageOrder: Storage.Order, _ storage: StorageType) {
// Upsert the shipping lines from the read-only order
for readOnlyShippingLine in readOnlyOrder.shippingLines {
if let existingStorageShippingLine = storage.loadOrderShippingLine(siteID: readOnlyOrder.siteID, shippingID: readOnlyShippingLine.shippingID) {
if let existingStorageShippingLine = storageOrder.shippingLines?.first(where: { $0.shippingID == readOnlyShippingLine.shippingID }) {
existingStorageShippingLine.update(with: readOnlyShippingLine)
handleShippingLineTaxes(readOnlyShippingLine, existingStorageShippingLine, storage)
} else {
Expand All @@ -256,11 +253,10 @@ struct OrdersUpsertUseCase {
/// Updates, inserts, or prunes the provided StorageShippingLine's taxes using the provided read-only ShippingLine
///
private func handleShippingLineTaxes(_ readOnlyItem: Networking.ShippingLine, _ storageItem: Storage.ShippingLine, _ storage: StorageType) {
let shippingID = readOnlyItem.shippingID

// Upsert the taxes from the read-only orderItem
for readOnlyTax in readOnlyItem.taxes {
if let existingStorageTax = storage.loadShippingLineTax(shippingID: shippingID, taxID: readOnlyTax.taxID) {
if let existingStorageTax = storageItem.taxes?.first(where: { $0.taxID == readOnlyTax.taxID }) {
existingStorageTax.update(with: readOnlyTax)
} else {
let newStorageTax = storage.insertNewObject(ofType: Storage.ShippingLineTax.self)
Expand All @@ -283,7 +279,7 @@ struct OrdersUpsertUseCase {
private func handleOrderTaxes(_ readOnlyOrder: Networking.Order, _ storageOrder: Storage.Order, _ storage: StorageType) {
// Upsert the `taxes` from the `readOnlyOrder`
readOnlyOrder.taxes.forEach { readOnlyTax in
if let existingStorageTax = storage.loadOrderTaxLine(siteID: readOnlyOrder.siteID, taxID: readOnlyTax.taxID) {
if let existingStorageTax = storageOrder.taxes?.first(where: { $0.taxID == readOnlyTax.taxID }) {
existingStorageTax.update(with: readOnlyTax)
} else {
let newStorageTax = storage.insertNewObject(ofType: Storage.OrderTaxLine.self)
Expand Down
18 changes: 10 additions & 8 deletions Yosemite/Yosemite/Stores/OrderStore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ private extension OrderStore {
return
}

self?.upsertSearchResultsInBackground(keyword: keyword, readOnlyOrders: orders) {
self?.upsertSearchResultsInBackground(for: siteID, keyword: keyword, readOnlyOrders: orders) {
onCompletion(nil)
}
}
Expand Down Expand Up @@ -579,8 +579,8 @@ extension OrderStore {

/// Unit Testing Helper: Updates or Inserts a given Search Results page
///
func upsertStoredResults(keyword: String, readOnlyOrder: Networking.Order, in storage: StorageType) {
upsertStoredResults(keyword: keyword, readOnlyOrders: [readOnlyOrder], in: storage)
func upsertStoredResults(for siteID: Int64, keyword: String, readOnlyOrder: Networking.Order, in storage: StorageType) {
upsertStoredResults(for: siteID, keyword: keyword, readOnlyOrders: [readOnlyOrder], in: storage)
}
}

Expand Down Expand Up @@ -609,22 +609,24 @@ private extension OrderStore {

/// Upserts the Orders, and associates them to the SearchResults Entity (in Background)
///
func upsertSearchResultsInBackground(keyword: String, readOnlyOrders: [Networking.Order], onCompletion: @escaping () -> Void) {
func upsertSearchResultsInBackground(for siteID: Int64, keyword: String, readOnlyOrders: [Networking.Order], onCompletion: @escaping () -> Void) {
storageManager.performAndSave({ [weak self] derivedStorage in
guard let self else { return }
upsertStoredOrders(readOnlyOrders: readOnlyOrders, insertingSearchResults: true, in: derivedStorage)
upsertStoredResults(keyword: keyword, readOnlyOrders: readOnlyOrders, in: derivedStorage)
upsertStoredResults(for: siteID, keyword: keyword, readOnlyOrders: readOnlyOrders, in: derivedStorage)
}, completion: onCompletion, on: .main)
}

/// Upserts the Orders, and associates them to the Search Results Entity (in the specified Storage)
///
func upsertStoredResults(keyword: String, readOnlyOrders: [Networking.Order], in storage: StorageType) {
func upsertStoredResults(for siteID: Int64, keyword: String, readOnlyOrders: [Networking.Order], in storage: StorageType) {
let searchResults = storage.loadOrderSearchResults(keyword: keyword) ?? storage.insertNewObject(ofType: Storage.OrderSearchResults.self)
searchResults.keyword = keyword

for readOnlyOrder in readOnlyOrders {
guard let storedOrder = storage.loadOrder(siteID: readOnlyOrder.siteID, orderID: readOnlyOrder.orderID) else {
let orderIDs = readOnlyOrders.map { $0.orderID }
let storedOrders = storage.loadOrders(siteID: siteID, orderIDs: orderIDs)
for orderID in orderIDs {
guard let storedOrder = storedOrders.first(where: { $0.orderID == orderID }) else {
continue
}

Expand Down
2 changes: 1 addition & 1 deletion Yosemite/YosemiteTests/Stores/OrderStoreTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,7 @@ final class OrderStoreTests: XCTestCase {
let remoteOrder = sampleOrder()

orderStore.upsertStoredOrder(readOnlyOrder: remoteOrder, insertingSearchResults: true, in: viewStorage)
orderStore.upsertStoredResults(keyword: defaultSearchKeyword, readOnlyOrder: remoteOrder, in: viewStorage)
orderStore.upsertStoredResults(for: sampleSiteID, keyword: defaultSearchKeyword, readOnlyOrder: remoteOrder, in: viewStorage)

let storageSearchResults = viewStorage.loadOrderSearchResults(keyword: defaultSearchKeyword)
let storageOrder = viewStorage.loadOrder(siteID: sampleSiteID, orderID: remoteOrder.orderID)
Expand Down