Skip to content

Commit

Permalink
[Subscriptions] Don't call server if user tracks existing products
Browse files Browse the repository at this point in the history 
Currently if user tracks existing products or untracks non-existing
products, we don't call server to add or remove subscriptions but still
fetch all remote subscriptions and sync with local cache.
This CL skips this unnecessary step to reduce server calls.

(cherry picked from commit ad230b5)

Bug: 1383382
Change-Id: Id9d927e175026cacbc844941235627f32ca7119b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4023352
Commit-Queue: Zhiyuan Cai <zhiyuancai@chromium.org>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1070508}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4022088
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Auto-Submit: Zhiyuan Cai <zhiyuancai@chromium.org>
Reviewed-by: Ayman Almadhoun <ayman@chromium.org>
Cr-Commit-Position: refs/branch-heads/5414@{#30}
Cr-Branched-From: 4417ee5-refs/heads/main@{#1070088}
  • Loading branch information
Zhiyuan Cai authored and Chromium LUCI CQ committed Nov 14, 2022
1 parent 35a45b8 commit fc2424f
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 0 deletions.
15 changes: 15 additions & 0 deletions components/commerce/core/subscriptions/subscriptions_manager.cc
View file
Expand Up @@ -4,6 +4,7 @@

#include "components/commerce/core/subscriptions/subscriptions_manager.h"
#include "base/metrics/histogram_functions.h"
#include "base/threading/sequenced_task_runner_handle.h"
#include "components/commerce/core/commerce_feature_list.h"
#include "components/commerce/core/subscriptions/commerce_subscription.h"
#include "components/commerce/core/subscriptions/subscriptions_server_proxy.h"
Expand Down Expand Up @@ -199,6 +200,13 @@ void SubscriptionsManager::ProcessSubscribeRequest(Request request) {
SubscriptionsRequestCallback callback,
std::unique_ptr<std::vector<CommerceSubscription>>
unique_subscriptions) {
if (unique_subscriptions->size() == 0) {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(std::move(callback),
SubscriptionsRequestStatus::kSuccess));
return;
}
manager->server_proxy_->Create(
std::move(unique_subscriptions),
base::BindOnce(
Expand All @@ -222,6 +230,13 @@ void SubscriptionsManager::ProcessUnsubscribeRequest(Request request) {
SubscriptionsRequestCallback callback,
std::unique_ptr<std::vector<CommerceSubscription>>
unique_subscriptions) {
if (unique_subscriptions->size() == 0) {
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(std::move(callback),
SubscriptionsRequestStatus::kSuccess));
return;
}
manager->server_proxy_->Delete(
std::move(unique_subscriptions),
base::BindOnce(
Expand Down
82 changes: 82 additions & 0 deletions components/commerce/core/subscriptions/subscriptions_manager_unittest.cc
View file
Expand Up @@ -167,6 +167,24 @@ class MockSubscriptionsStorage : public SubscriptionsStorage {
});
}

// Mock empty local fetch responses for Get* requests.
void MockEmptyGetResponses() {
ON_CALL(*this, GetUniqueNonExistingSubscriptions)
.WillByDefault([](std::unique_ptr<std::vector<CommerceSubscription>>
subscriptions,
GetLocalSubscriptionsCallback callback) {
std::move(callback).Run(
std::make_unique<std::vector<commerce::CommerceSubscription>>());
});
ON_CALL(*this, GetUniqueExistingSubscriptions)
.WillByDefault([](std::unique_ptr<std::vector<CommerceSubscription>>
subscriptions,
GetLocalSubscriptionsCallback callback) {
std::move(callback).Run(
std::make_unique<std::vector<commerce::CommerceSubscription>>());
});
}

// Mock the responses for UpdateStorage requests.
void MockUpdateResponses(bool succeeded) {
ON_CALL(*this, UpdateStorage)
Expand Down Expand Up @@ -528,6 +546,38 @@ TEST_F(SubscriptionsManagerTest, TestSubscribe_HasPendingUnsubscribeRequest) {
VerifyHasPendingRequests(false);
}

TEST_F(SubscriptionsManagerTest, TestSubscribe_ExistingSubscriptions) {
SetAccountStatus(true, true);
mock_server_proxy_->MockGetResponses("111");
mock_server_proxy_->MockManageResponses(true);
mock_storage_->MockEmptyGetResponses();
mock_storage_->MockUpdateResponses(true);

{
InSequence s;
EXPECT_CALL(*mock_storage_, DeleteAll);
EXPECT_CALL(*mock_server_proxy_, Get);
EXPECT_CALL(*mock_storage_,
UpdateStorage(_, _, AreExpectedSubscriptions("111")));
EXPECT_CALL(*mock_storage_, GetUniqueNonExistingSubscriptions(
AreExpectedSubscriptions("333"), _));
EXPECT_CALL(*mock_server_proxy_, Create).Times(0);
}

CreateManagerAndVerify(true);
base::RunLoop run_loop;
subscriptions_manager_->Subscribe(
BuildSubscriptions("333"),
base::BindOnce(
[](base::RunLoop* run_loop, bool succeeded) {
ASSERT_EQ(true, succeeded);
run_loop->Quit();
},
&run_loop));
// The callback should eventually quit the run loop.
run_loop.Run();
}

TEST_F(SubscriptionsManagerTest, TestUnsubscribe) {
SetAccountStatus(true, true);
mock_server_proxy_->MockGetResponses("111");
Expand Down Expand Up @@ -630,6 +680,38 @@ TEST_F(SubscriptionsManagerTest,
ASSERT_EQ(false, callback_executed);
}

TEST_F(SubscriptionsManagerTest, TestUnsubscribe_NonExistingSubscriptions) {
SetAccountStatus(true, true);
mock_server_proxy_->MockGetResponses("111");
mock_server_proxy_->MockManageResponses(true);
mock_storage_->MockEmptyGetResponses();
mock_storage_->MockUpdateResponses(true);

{
InSequence s;
EXPECT_CALL(*mock_storage_, DeleteAll);
EXPECT_CALL(*mock_server_proxy_, Get);
EXPECT_CALL(*mock_storage_,
UpdateStorage(_, _, AreExpectedSubscriptions("111")));
EXPECT_CALL(*mock_storage_, GetUniqueExistingSubscriptions(
AreExpectedSubscriptions("333"), _));
EXPECT_CALL(*mock_server_proxy_, Delete).Times(0);
}

CreateManagerAndVerify(true);
base::RunLoop run_loop;
subscriptions_manager_->Unsubscribe(
BuildSubscriptions("333"),
base::BindOnce(
[](base::RunLoop* run_loop, bool succeeded) {
ASSERT_EQ(true, succeeded);
run_loop->Quit();
},
&run_loop));
// The callback should eventually quit the run loop.
run_loop.Run();
}

TEST_F(SubscriptionsManagerTest, TestIdentityChange) {
SetAccountStatus(true, true);
mock_server_proxy_->MockGetResponses("111");
Expand Down

0 comments on commit fc2424f

Please sign in to comment.