Skip to content

Commit

Permalink
[M116][PriceInsights] Update ProductInfo mojo definition
Browse files Browse the repository at this point in the history
This CL adds product cluster_id to ProductInfo mojo struct and exposes
BookmarkProductInfo in multiple APIs. The cluster_id will be used in
the later CL to help decide if certain change is related to the
product in the current page or not by comparing the cluster_id.

(cherry picked from commit 63019af)

Bug: 1456420
Change-Id: I913f074b257e75909c49aea50daa4ef316884673
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4632074
Reviewed-by: John Lee <johntlee@chromium.org>
Commit-Queue: Yue Zhang <yuezhanggg@chromium.org>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1163256}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4657004
Reviewed-by: Chris Bookholt <bookholt@chromium.org>
Cr-Commit-Position: refs/branch-heads/5845@{#274}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
  • Loading branch information
Yue Zhang authored and Chromium LUCI CQ committed Jun 30, 2023
1 parent e008449 commit 953aee0
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 35 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,11 @@ export class ShoppingListElement extends PolymerElement {
(product: BookmarkProductInfo) =>
this.onBookmarkPriceTracked(product)),
callbackRouter.priceUntrackedForBookmark.addListener(
(bookmarkId: bigint) => this.onBookmarkPriceUntracked(bookmarkId)),
(product: BookmarkProductInfo) =>
this.onBookmarkPriceUntracked(product)),
callbackRouter.operationFailedForBookmark.addListener(
(bookmarkId: bigint, attemptedTrack: boolean) =>
this.onBookmarkOperationFailed(bookmarkId, attemptedTrack)),
(product: BookmarkProductInfo, attemptedTrack: boolean) =>
this.onBookmarkOperationFailed(product, attemptedTrack)),
);
try {
this.open_ =
Expand Down Expand Up @@ -215,9 +216,9 @@ export class ShoppingListElement extends PolymerElement {
}
}

private onBookmarkPriceUntracked(bookmarkId: bigint) {
private onBookmarkPriceUntracked(product: BookmarkProductInfo) {
const untrackedItem =
this.productInfos.find(item => item.bookmarkId === bookmarkId);
this.productInfos.find(item => item.bookmarkId === product.bookmarkId);
if (untrackedItem == null) {
return;
}
Expand Down Expand Up @@ -255,12 +256,12 @@ export class ShoppingListElement extends PolymerElement {
}

private onBookmarkOperationFailed(
bookmarkId: bigint, attemptedTrack: boolean) {
product: BookmarkProductInfo, attemptedTrack: boolean) {
this.retryOperationCallback_ = () => {
if (attemptedTrack) {
this.shoppingListApi_.trackPriceForBookmark(bookmarkId);
this.shoppingListApi_.trackPriceForBookmark(product.bookmarkId);
} else {
this.shoppingListApi_.untrackPriceForBookmark(bookmarkId);
this.shoppingListApi_.untrackPriceForBookmark(product.bookmarkId);
}
};
this.$.errorToast.show();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ suite('SidePanelBookmarksListTest', () => {
productUrl: {url: 'https://foo.com/product'},
currentPrice: '$12',
previousPrice: '$34',
clusterId: BigInt(12345),
},
}];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ suite('SidePanelShoppingListTest', () => {
productUrl: {url: 'https://foo.com/product'},
currentPrice: '$12',
previousPrice: '$34',
clusterId: BigInt(12345),
},
},
{
Expand All @@ -50,6 +51,7 @@ suite('SidePanelShoppingListTest', () => {
productUrl: {url: 'https://foo.com/product'},
currentPrice: '$15',
previousPrice: '',
clusterId: BigInt(12345),
},
},
];
Expand Down Expand Up @@ -292,6 +294,7 @@ suite('SidePanelShoppingListTest', () => {
productUrl: {url: 'https://baz.com/product'},
currentPrice: '$56',
previousPrice: '$78',
clusterId: BigInt(12345),
},
};

Expand All @@ -310,7 +313,7 @@ suite('SidePanelShoppingListTest', () => {
}

shoppingListApi.getCallbackRouterRemote().priceUntrackedForBookmark(
newProduct.bookmarkId);
newProduct);
await flushTasks();
checkActionButtonStatus(actionButtons[0]!, true);
checkActionButtonStatus(actionButtons[1]!, true);
Expand All @@ -333,7 +336,7 @@ suite('SidePanelShoppingListTest', () => {
checkActionButtonStatus(actionButtonA, true);

shoppingListApi.getCallbackRouterRemote().priceUntrackedForBookmark(
product.bookmarkId);
product);
await flushTasks();
checkActionButtonStatus(actionButtonA, false);

Expand All @@ -360,6 +363,7 @@ suite('SidePanelShoppingListTest', () => {
productUrl: {url: 'https://baz.com/product'},
currentPrice: '$56',
previousPrice: '$78',
clusterId: BigInt(12345),
},
};
shoppingListApi.getCallbackRouterRemote().priceTrackedForBookmark(
Expand Down Expand Up @@ -393,7 +397,7 @@ suite('SidePanelShoppingListTest', () => {

test('ShowErrorToastWhenTrackAndUntrackFailed', async () => {
shoppingListApi.getCallbackRouterRemote().operationFailedForBookmark(
products[0]!.bookmarkId, true);
products[0]!, true);
await flushTasks();

assertTrue(shoppingList.$.errorToast.open);
Expand All @@ -403,7 +407,7 @@ suite('SidePanelShoppingListTest', () => {
assertFalse(shoppingList.$.errorToast.open);

shoppingListApi.getCallbackRouterRemote().operationFailedForBookmark(
products[1]!.bookmarkId, false);
products[1]!, false);
await flushTasks();

assertTrue(shoppingList.$.errorToast.open);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export class TestShoppingListApiProxy extends TestBrowserProxy implements
productUrl: {url: 'https://foo.com/product'},
currentPrice: '$12',
previousPrice: '$34',
clusterId: BigInt(12345),
};
private priceInsights_: PriceInsightsInfo = {
clusterId: BigInt(123),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ suite('ShoppingInsightsAppTest', () => {
productUrl: {url: 'https://foo.com/product'},
currentPrice: '$12',
previousPrice: '$34',
clusterId: BigInt(12345),
};
const priceInsights1: PriceInsightsInfo = {
clusterId: BigInt(123),
Expand Down
19 changes: 12 additions & 7 deletions components/commerce/core/mojom/shopping_list.mojom
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ struct ProductInfo {
// This should include both the amount and the currency
// symbol.
string previous_price;

// The product cluster id.
int64 cluster_id;
};

struct BookmarkProductInfo {
Expand Down Expand Up @@ -113,13 +116,15 @@ interface Page {
// pass along the `BookmarkProductInfo` constructed from it.
PriceTrackedForBookmark(BookmarkProductInfo bookmark_product);

// Callback when a bookmark with `bookmark_id` is observed to stop being
// price tracked.
PriceUntrackedForBookmark(int64 bookmark_id);
// Callback when a bookmark is observed to stop being price tracked and
// pass along the `BookmarkProductInfo` constructed from it.
PriceUntrackedForBookmark(BookmarkProductInfo bookmark_product);

// Callback to notify the WebUI to show error UI when a track/untrack
// attempt for bookmark with `bookmark_id` failed. `attempted_track` is
// true when the failed operation is to track price, false when the
// failed operation is to untrack price.
OperationFailedForBookmark(int64 bookmark_id, bool attempted_track);
// attempt is failed and pass along the `BookmarkProductInfo`
// constructed from it. `attempted_track` is true when the failed
// operation is to track price, false when the failed operation is to
// untrack price.
OperationFailedForBookmark(BookmarkProductInfo bookmark_product,
bool attempted_track);
};
19 changes: 11 additions & 8 deletions components/commerce/core/webui/shopping_list_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ shopping_list::mojom::BookmarkProductInfoPtr BookmarkNodeToMojoProduct(

bookmark_info->info->product_url = node->url();
bookmark_info->info->image_url = GURL(meta->lead_image().url());
bookmark_info->info->cluster_id = specifics.product_cluster_id();

const power_bookmarks::ProductPrice price = specifics.current_price();
std::string currency_code = price.currency_code();
Expand Down Expand Up @@ -311,11 +312,11 @@ void ShoppingListHandler::HandleSubscriptionChange(
std::vector<const bookmarks::BookmarkNode*> bookmarks =
GetBookmarksWithClusterId(bookmark_model_, cluster_id);
for (auto* node : bookmarks) {
auto product = BookmarkNodeToMojoProduct(*bookmark_model_, node, locale_);
if (is_tracking) {
remote_page_->PriceTrackedForBookmark(
BookmarkNodeToMojoProduct(*bookmark_model_, node, locale_));
remote_page_->PriceTrackedForBookmark(std::move(product));
} else {
remote_page_->PriceUntrackedForBookmark(node->id());
remote_page_->PriceUntrackedForBookmark(std::move(product));
}
}
}
Expand Down Expand Up @@ -344,17 +345,19 @@ void ShoppingListHandler::onPriceTrackResult(int64_t bookmark_id,
// with, we assume success. In the event it failed, we switch things back.
// So in this case, if we were trying to untrack and that action failed, set
// the UI back to "tracking".
auto* node = bookmarks::GetBookmarkNodeByID(bookmark_model_, bookmark_id);
auto product = BookmarkNodeToMojoProduct(*bookmark_model_, node, locale_);

if (!is_tracking) {
auto* node = bookmarks::GetBookmarkNodeByID(bookmark_model_, bookmark_id);
remote_page_->PriceTrackedForBookmark(
BookmarkNodeToMojoProduct(*model, node, locale_));
remote_page_->PriceTrackedForBookmark(std::move(product));
} else {
remote_page_->PriceUntrackedForBookmark(bookmark_id);
remote_page_->PriceUntrackedForBookmark(std::move(product));
}
// Pass in whether the failed operation was to track or untrack price. It
// should be the reverse of the current tracking status since the operation
// failed.
remote_page_->OperationFailedForBookmark(bookmark_id, is_tracking);
remote_page_->OperationFailedForBookmark(
BookmarkNodeToMojoProduct(*bookmark_model_, node, locale_), is_tracking);
}

void ShoppingListHandler::GetProductInfoForCurrentUrl(
Expand Down
29 changes: 21 additions & 8 deletions components/commerce/core/webui/shopping_list_handler_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,11 @@ class MockPage : public shopping_list::mojom::Page {

MOCK_METHOD1(PriceTrackedForBookmark,
void(shopping_list::mojom::BookmarkProductInfoPtr product));
MOCK_METHOD1(PriceUntrackedForBookmark, void(int64_t bookmark_id));
MOCK_METHOD1(PriceUntrackedForBookmark,
void(shopping_list::mojom::BookmarkProductInfoPtr product));
MOCK_METHOD2(OperationFailedForBookmark,
void(int64_t bookmark_id, bool is_tracked));
void(shopping_list::mojom::BookmarkProductInfoPtr product,
bool is_tracked));
};

class MockDelegate : public ShoppingListHandler::Delegate {
Expand Down Expand Up @@ -222,7 +224,9 @@ TEST_F(ShoppingListHandlerTest, TestUntrackProductSuccess) {
EXPECT_CALL(*shopping_service_,
Unsubscribe(VectorHasSubscriptionWithId("123"), testing::_))
.Times(1);
EXPECT_CALL(page_, PriceUntrackedForBookmark(product->id())).Times(1);
EXPECT_CALL(page_,
PriceUntrackedForBookmark(MojoBookmarkInfoWithId(product->id())))
.Times(1);
EXPECT_CALL(page_, OperationFailedForBookmark(testing::_, testing::_))
.Times(0);

Expand All @@ -245,9 +249,13 @@ TEST_F(ShoppingListHandlerTest, TestTrackProductFailure) {
shopping_service_->SetUnsubscribeCallbackValue(false);

// "untrack" should be called once to undo the "track" change in the UI.
EXPECT_CALL(page_, PriceUntrackedForBookmark(product->id())).Times(1);
EXPECT_CALL(page_,
PriceUntrackedForBookmark(MojoBookmarkInfoWithId(product->id())))
.Times(1);
EXPECT_CALL(page_, PriceTrackedForBookmark(testing::_)).Times(0);
EXPECT_CALL(page_, OperationFailedForBookmark(product->id(), true)).Times(1);
EXPECT_CALL(page_, OperationFailedForBookmark(
MojoBookmarkInfoWithId(product->id()), true))
.Times(1);

handler_->TrackPriceForBookmark(product->id());

Expand All @@ -269,8 +277,12 @@ TEST_F(ShoppingListHandlerTest, TestUntrackProductFailure) {

// "track" should be called once to undo the "untrack" change in the UI.
EXPECT_CALL(page_, PriceTrackedForBookmark(testing::_)).Times(1);
EXPECT_CALL(page_, PriceUntrackedForBookmark(product->id())).Times(0);
EXPECT_CALL(page_, OperationFailedForBookmark(product->id(), false)).Times(1);
EXPECT_CALL(page_,
PriceUntrackedForBookmark(MojoBookmarkInfoWithId(product->id())))
.Times(0);
EXPECT_CALL(page_, OperationFailedForBookmark(
MojoBookmarkInfoWithId(product->id()), false))
.Times(1);

handler_->UntrackPriceForBookmark(product->id());

Expand All @@ -285,7 +297,8 @@ TEST_F(ShoppingListHandlerTest, PageUpdateForPriceTrackChange) {
bookmark_model_.get(), u"product 1", GURL("http://example.com/1"), 123L,
true, 1230000, "usd");

EXPECT_CALL(page_, PriceUntrackedForBookmark(product->id()));
EXPECT_CALL(page_,
PriceUntrackedForBookmark(MojoBookmarkInfoWithId(product->id())));

// Assume the plumbing for subscriptions works and fake an unsubscribe event.
handler_->OnUnsubscribe(CreateUserTrackedSubscription(123L), true);
Expand Down

0 comments on commit 953aee0

Please sign in to comment.