0

Introduce lightweight ash/ui-friendly types for task lists and tasks

The motivation was to:
- separate `google_apis/tasks` types (heavier on logic,
  `JSONValueConverter` aware) with types used on UI;
- pre-process data only once to keep it in more UI-friendly format
  (for example, returned JSON uses `parent` property to define
  subtask-task relationship. Instead, we can create a tree-like
  structure).

Bug: b/269750741
Change-Id: I82e5d13b119decfd854f33a14e5e8857a5a6fee5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4342175
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Commit-Queue: Artsiom Mitrokhin <amitrokhin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1120705}
This commit is contained in:
Artsiom Mitrokhin
2023-03-22 19:05:14 +00:00
committed by Chromium LUCI CQ
parent 0822ff882c
commit 7dcff77045
7 changed files with 403 additions and 35 deletions

@ -607,6 +607,8 @@ component("ash") {
"glanceables/glanceables_welcome_label.cc",
"glanceables/glanceables_welcome_label.h",
"glanceables/tasks/glanceables_tasks_client.h",
"glanceables/tasks/glanceables_tasks_types.cc",
"glanceables/tasks/glanceables_tasks_types.h",
"host/ash_window_tree_host.cc",
"host/ash_window_tree_host.h",
"host/ash_window_tree_host_delegate.h",

@ -6,28 +6,34 @@
#define ASH_GLANCEABLES_TASKS_GLANCEABLES_TASKS_CLIENT_H_
#include <string>
#include <vector>
#include "ash/ash_export.h"
#include "base/functional/callback_forward.h"
#include "google_apis/tasks/tasks_api_requests.h"
namespace ash {
struct GlanceablesTask;
struct GlanceablesTaskList;
// Interface for the tasks browser client.
class ASH_EXPORT GlanceablesTasksClient {
public:
using GetTaskListsCallback = base::OnceCallback<void(
const std::vector<GlanceablesTaskList>& task_lists)>;
using GetTasksCallback =
base::OnceCallback<void(const std::vector<GlanceablesTask>& tasks)>;
// Fetches all the authenticated user's task lists and invokes `callback` when
// done.
// Returned `base::OnceClosure` can cancel the api call.
virtual base::OnceClosure GetTaskLists(
google_apis::tasks::ListTaskListsRequest::Callback callback) = 0;
virtual base::OnceClosure GetTaskLists(GetTaskListsCallback callback) = 0;
// Fetches all tasks in the specified task list (`task_list_id` must not be
// empty) and invokes `callback` when done.
// Returned `base::OnceClosure` can cancel the api call.
virtual base::OnceClosure GetTasks(
google_apis::tasks::ListTasksRequest::Callback callback,
const std::string& task_list_id) = 0;
virtual base::OnceClosure GetTasks(GetTasksCallback callback,
const std::string& task_list_id) = 0;
protected:
virtual ~GlanceablesTasksClient() = default;

@ -0,0 +1,29 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "ash/glanceables/tasks/glanceables_tasks_types.h"
namespace ash {
GlanceablesTaskList::GlanceablesTaskList(const std::string& id,
const std::string& title,
const base::Time& updated)
: id(id), title(title), updated(updated) {}
GlanceablesTaskList::~GlanceablesTaskList() = default;
// ----------------------------------------------------------------------------
// GlanceablesTask:
GlanceablesTask::GlanceablesTask(const std::string& id,
const std::string& title,
bool completed,
const std::vector<GlanceablesTask>& subtasks)
: id(id), title(title), completed(completed), subtasks(subtasks) {}
GlanceablesTask::GlanceablesTask(const GlanceablesTask&) = default;
GlanceablesTask::~GlanceablesTask() = default;
} // namespace ash

@ -0,0 +1,68 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef ASH_GLANCEABLES_TASKS_GLANCEABLES_TASKS_TYPES_H_
#define ASH_GLANCEABLES_TASKS_GLANCEABLES_TASKS_TYPES_H_
#include <string>
#include <vector>
#include "ash/ash_export.h"
#include "base/time/time.h"
namespace ash {
// Lightweight TaskList definition to separate API and ash/ui-friendly types.
// Contains information that describes a single task list. All values are from
// the API resource
// https://developers.google.com/tasks/reference/rest/v1/tasklists.
struct ASH_EXPORT GlanceablesTaskList {
GlanceablesTaskList() = delete;
GlanceablesTaskList(const std::string& id,
const std::string& title,
const base::Time& updated);
~GlanceablesTaskList();
// Task list identifier.
const std::string id;
// Title of the task list.
const std::string title;
// Last modification time of the task list.
const base::Time updated;
};
// Lightweight Task definition to separate API and ash/ui-friendly types.
// The most significant difference is that API tasks are flat (subtask-task
// relationship is expressed by parent id property), but here they are
// represented as a tree structure. All values are from the API resource
// https://developers.google.com/tasks/reference/rest/v1/tasks.
struct ASH_EXPORT GlanceablesTask {
GlanceablesTask() = delete;
GlanceablesTask(const std::string& id,
const std::string& title,
bool completed,
const std::vector<GlanceablesTask>& subtasks);
GlanceablesTask(const GlanceablesTask&);
~GlanceablesTask();
// Task identifier.
const std::string id;
// Title of the task.
const std::string title;
// Indicates whether the task is completed (has "status" field equals to
// "completed" on the API side).
bool completed;
// Subtasks of the task (pre-grouped tasks that have "parent" field equals to
// `id` on the API side).
const std::vector<GlanceablesTask> subtasks;
};
} // namespace ash
#endif // ASH_GLANCEABLES_TASKS_GLANCEABLES_TASKS_TYPES_H_

@ -9,8 +9,14 @@
#include <string>
#include <vector>
#include "ash/glanceables/tasks/glanceables_tasks_client.h"
#include "ash/glanceables/tasks/glanceables_tasks_types.h"
#include "base/check.h"
#include "base/containers/flat_map.h"
#include "base/functional/bind.h"
#include "base/functional/callback_forward.h"
#include "base/types/expected.h"
#include "google_apis/common/api_error_codes.h"
#include "google_apis/common/request_sender.h"
#include "google_apis/gaia/gaia_constants.h"
#include "google_apis/tasks/tasks_api_requests.h"
@ -20,8 +26,11 @@
namespace ash {
namespace {
using ::google_apis::ApiErrorCode;
using ::google_apis::tasks::ListTaskListsRequest;
using ::google_apis::tasks::ListTasksRequest;
using ::google_apis::tasks::Task;
using ::google_apis::tasks::TaskList;
using ::google_apis::tasks::TaskLists;
using ::google_apis::tasks::Tasks;
@ -54,6 +63,64 @@ constexpr net::NetworkTrafficAnnotationTag kTrafficAnnotationTag =
}
)");
std::vector<GlanceablesTaskList> ConvertTaskLists(
const std::vector<std::unique_ptr<TaskList>>& raw_items) {
std::vector<GlanceablesTaskList> converted_task_lists;
converted_task_lists.reserve(raw_items.size());
for (const auto& item : raw_items) {
converted_task_lists.emplace_back(item->id(), item->title(),
item->updated());
}
return converted_task_lists;
}
GlanceablesTask ConvertTask(
const Task* const task,
base::flat_map<std::string, std::vector<const Task*>>& grouped_subtasks) {
const auto iter = grouped_subtasks.find(task->id());
std::vector<GlanceablesTask> converted_subtasks;
if (iter != grouped_subtasks.end()) {
converted_subtasks.reserve(iter->second.size());
for (const auto* const subtask : iter->second) {
converted_subtasks.push_back(ConvertTask(subtask, grouped_subtasks));
}
grouped_subtasks.erase(iter);
}
return GlanceablesTask(task->id(), task->title(),
task->status() == Task::Status::kCompleted,
converted_subtasks);
}
std::vector<GlanceablesTask> ConvertTasks(
const std::vector<std::unique_ptr<Task>>& raw_items) {
// Find root level tasks and group all other subtasks by their parent id.
std::vector<const Task*> root_tasks;
base::flat_map<std::string, std::vector<const Task*>> grouped_subtasks;
for (const auto& item : raw_items) {
if (item->parent_id().empty()) {
root_tasks.push_back(item.get());
continue;
}
grouped_subtasks[item->parent_id()].push_back(item.get());
}
std::vector<GlanceablesTask> converted_tasks;
converted_tasks.reserve(root_tasks.size());
for (const auto* const root_task : root_tasks) {
converted_tasks.push_back(ConvertTask(root_task, grouped_subtasks));
}
if (!grouped_subtasks.empty()) {
// At this moment `grouped_subtasks` should be empty. If not - something is
// wrong with the returned data (some tasks point to invalid `parent_id()`).
return std::vector<GlanceablesTask>();
}
return converted_tasks;
}
} // namespace
GlanceablesTasksClientImpl::GlanceablesTasksClientImpl(
@ -64,21 +131,42 @@ GlanceablesTasksClientImpl::GlanceablesTasksClientImpl(
GlanceablesTasksClientImpl::~GlanceablesTasksClientImpl() = default;
base::OnceClosure GlanceablesTasksClientImpl::GetTaskLists(
ListTaskListsRequest::Callback callback) {
GlanceablesTasksClient::GetTaskListsCallback callback) {
EnsureRequestSenderExists();
return request_sender_->StartRequestWithAuthRetry(
std::make_unique<ListTaskListsRequest>(request_sender_.get(),
std::move(callback)));
std::make_unique<ListTaskListsRequest>(
request_sender_.get(),
base::BindOnce(&GlanceablesTasksClientImpl::OnTaskListsFetched,
weak_factory_.GetWeakPtr(), std::move(callback))));
}
base::OnceClosure GlanceablesTasksClientImpl::GetTasks(
ListTasksRequest::Callback callback,
GlanceablesTasksClient::GetTasksCallback callback,
const std::string& task_list_id) {
DCHECK(!task_list_id.empty());
EnsureRequestSenderExists();
return request_sender_->StartRequestWithAuthRetry(
std::make_unique<ListTasksRequest>(request_sender_.get(),
std::move(callback), task_list_id));
std::make_unique<ListTasksRequest>(
request_sender_.get(),
base::BindOnce(&GlanceablesTasksClientImpl::OnTasksFetched,
weak_factory_.GetWeakPtr(), std::move(callback)),
task_list_id));
}
void GlanceablesTasksClientImpl::OnTaskListsFetched(
GlanceablesTasksClient::GetTaskListsCallback callback,
base::expected<std::unique_ptr<TaskLists>, ApiErrorCode> result) const {
std::move(callback).Run(result.has_value()
? ConvertTaskLists(result.value()->items())
: std::vector<GlanceablesTaskList>());
}
void GlanceablesTasksClientImpl::OnTasksFetched(
GlanceablesTasksClient::GetTasksCallback callback,
base::expected<std::unique_ptr<Tasks>, ApiErrorCode> result) const {
std::move(callback).Run(result.has_value()
? ConvertTasks(result.value()->items())
: std::vector<GlanceablesTask>());
}
void GlanceablesTasksClientImpl::EnsureRequestSenderExists() {

@ -11,10 +11,16 @@
#include "ash/glanceables/tasks/glanceables_tasks_client.h"
#include "base/functional/callback_forward.h"
#include "base/memory/weak_ptr.h"
#include "base/types/expected.h"
#include "google_apis/tasks/tasks_api_requests.h"
namespace google_apis {
class RequestSender;
namespace tasks {
class TaskLists;
class Tasks;
} // namespace tasks
} // namespace google_apis
namespace net {
@ -42,12 +48,23 @@ class GlanceablesTasksClientImpl : public GlanceablesTasksClient {
// GlanceablesTasksClient:
base::OnceClosure GetTaskLists(
google_apis::tasks::ListTaskListsRequest::Callback callback) override;
base::OnceClosure GetTasks(
google_apis::tasks::ListTasksRequest::Callback callback,
const std::string& task_list_id) override;
GlanceablesTasksClient::GetTaskListsCallback callback) override;
base::OnceClosure GetTasks(GlanceablesTasksClient::GetTasksCallback callback,
const std::string& task_list_id) override;
private:
// Callback for `GetTaskLists()`. Transforms fetched items to ash-friendly
// types.
void OnTaskListsFetched(
GlanceablesTasksClient::GetTaskListsCallback callback,
base::expected<std::unique_ptr<google_apis::tasks::TaskLists>,
google_apis::ApiErrorCode> result) const;
// Callback for `GetTasks()`. Transforms fetched items to ash-friendly types.
void OnTasksFetched(GlanceablesTasksClient::GetTasksCallback callback,
base::expected<std::unique_ptr<google_apis::tasks::Tasks>,
google_apis::ApiErrorCode> result) const;
// Creates `request_sender_` by calling `create_request_sender_callback_` on
// demand.
void EnsureRequestSenderExists();
@ -58,6 +75,8 @@ class GlanceablesTasksClientImpl : public GlanceablesTasksClient {
// Helper class that sends requests, handles retries and authentication.
std::unique_ptr<google_apis::RequestSender> request_sender_;
base::WeakPtrFactory<GlanceablesTasksClientImpl> weak_factory_{this};
};
} // namespace ash

@ -7,25 +7,26 @@
#include <algorithm>
#include <memory>
#include <string>
#include <vector>
#include "ash/constants/ash_features.h"
#include "ash/glanceables/tasks/glanceables_tasks_types.h"
#include "base/functional/bind.h"
#include "base/functional/callback_forward.h"
#include "base/memory/scoped_refptr.h"
#include "base/test/bind.h"
#include "base/test/scoped_command_line.h"
#include "base/test/scoped_feature_list.h"
#include "base/test/task_environment.h"
#include "base/test/test_future.h"
#include "base/types/expected.h"
#include "content/public/test/browser_task_environment.h"
#include "google_apis/common/api_error_codes.h"
#include "google_apis/common/dummy_auth_service.h"
#include "google_apis/common/request_sender.h"
#include "google_apis/common/test_util.h"
#include "google_apis/common/time_util.h"
#include "google_apis/gaia/gaia_switches.h"
#include "google_apis/gaia/gaia_urls.h"
#include "google_apis/tasks/tasks_api_requests.h"
#include "google_apis/tasks/tasks_api_response_types.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
#include "net/test/embedded_test_server/http_request.h"
#include "net/test/embedded_test_server/http_response.h"
@ -38,8 +39,9 @@ namespace {
using ::base::test::TestFuture;
using ::google_apis::ApiErrorCode;
using ::google_apis::tasks::TaskLists;
using ::google_apis::tasks::Tasks;
using ::google_apis::util::FormatTimeAsString;
using ::net::test_server::HttpRequest;
using ::net::test_server::HttpResponse;
// Helper class to temporary override `GaiaUrls` singleton.
class GaiaUrlsOverrider {
@ -51,10 +53,29 @@ class GaiaUrlsOverrider {
GaiaUrls test_gaia_urls_;
};
std::unique_ptr<net::test_server::HttpResponse> CreateSuccessfulResponse(
const std::string& content) {
auto response = std::make_unique<net::test_server::BasicHttpResponse>();
response->set_code(net::HTTP_OK);
response->set_content(content);
response->set_content_type("application/json");
return response;
}
std::unique_ptr<net::test_server::HttpResponse> CreateFailedResponse() {
auto response = std::make_unique<net::test_server::BasicHttpResponse>();
response->set_code(net::HTTP_INTERNAL_SERVER_ERROR);
return response;
}
} // namespace
class GlanceablesTasksClientImplTest : public testing::Test {
public:
using GenerateResponseCallback =
base::RepeatingCallback<std::unique_ptr<HttpResponse>(
const HttpRequest& request)>;
void SetUp() override {
auto create_request_sender_callback = base::BindLambdaForTesting(
[&](const std::vector<std::string>& scopes,
@ -78,9 +99,8 @@ class GlanceablesTasksClientImplTest : public testing::Test {
test_server_.base_url().spec());
}
// Relative to "google_apis/test/data/".
void set_file_path_for_response(const std::string& path) {
file_path_for_response_ = path;
void set_generate_response_callback(const GenerateResponseCallback& cb) {
generate_response_callback_ = cb;
}
GlanceablesTasksClientImpl* client() { return client_.get(); }
@ -88,8 +108,7 @@ class GlanceablesTasksClientImplTest : public testing::Test {
private:
std::unique_ptr<net::test_server::HttpResponse> HandleDataFileRequest(
const net::test_server::HttpRequest& request) {
return google_apis::test_util::CreateHttpResponseFromFile(
google_apis::test_util::GetTestFilePath(file_path_for_response_));
return std::move(generate_response_callback_).Run(request);
}
content::BrowserTaskEnvironment task_environment_{
@ -102,33 +121,170 @@ class GlanceablesTasksClientImplTest : public testing::Test {
/*network_service=*/nullptr,
/*is_trusted=*/true);
std::unique_ptr<GaiaUrlsOverrider> gaia_urls_overrider_;
std::string file_path_for_response_;
GenerateResponseCallback generate_response_callback_;
std::unique_ptr<GlanceablesTasksClientImpl> client_;
};
TEST_F(GlanceablesTasksClientImplTest, GetTaskLists) {
set_file_path_for_response("tasks/task_lists.json");
set_generate_response_callback(
base::BindLambdaForTesting([](const HttpRequest& request) {
return CreateSuccessfulResponse(R"(
{
"kind": "tasks#taskLists",
"items": [
{
"id": "qwerty",
"title": "My Tasks 1",
"updated": "2023-01-30T22:19:22.812Z"
},
{
"id": "asdfgh",
"title": "My Tasks 2",
"updated": "2022-12-21T23:38:22.590Z"
}
]
}
)");
}));
TestFuture<base::expected<std::unique_ptr<TaskLists>, ApiErrorCode>> future;
TestFuture<const std::vector<GlanceablesTaskList>&> future;
auto cancel_closure = client()->GetTaskLists(future.GetCallback());
ASSERT_TRUE(future.Wait());
EXPECT_FALSE(cancel_closure.is_null());
EXPECT_TRUE(future.Get().has_value());
EXPECT_EQ(future.Get().value()->items().size(), 2u);
const auto& task_lists = future.Get();
EXPECT_EQ(task_lists.size(), 2u);
EXPECT_EQ(task_lists.at(0).id, "qwerty");
EXPECT_EQ(task_lists.at(0).title, "My Tasks 1");
EXPECT_EQ(FormatTimeAsString(task_lists.at(0).updated),
"2023-01-30T22:19:22.812Z");
EXPECT_EQ(task_lists.at(1).id, "asdfgh");
EXPECT_EQ(task_lists.at(1).title, "My Tasks 2");
EXPECT_EQ(FormatTimeAsString(task_lists.at(1).updated),
"2022-12-21T23:38:22.590Z");
}
TEST_F(GlanceablesTasksClientImplTest,
GetTaskListsReturnsEmptyVectorOnHttpError) {
set_generate_response_callback(base::BindLambdaForTesting(
[](const HttpRequest& request) { return CreateFailedResponse(); }));
TestFuture<const std::vector<GlanceablesTaskList>&> future;
auto cancel_closure = client()->GetTaskLists(future.GetCallback());
ASSERT_TRUE(future.Wait());
EXPECT_FALSE(cancel_closure.is_null());
const auto& task_lists = future.Get();
EXPECT_EQ(task_lists.size(), 0u);
}
TEST_F(GlanceablesTasksClientImplTest, GetTasks) {
set_file_path_for_response("tasks/tasks.json");
set_generate_response_callback(
base::BindLambdaForTesting([](const HttpRequest& request) {
return CreateSuccessfulResponse(R"(
{
"kind": "tasks#tasks",
"items": [
{
"id": "asd",
"title": "Parent task, level 1",
"status": "needsAction"
},
{
"id": "qwe",
"title": "Child task, level 2",
"parent": "asd",
"status": "needsAction"
},
{
"id": "zxc",
"title": "Child task, level 3",
"parent": "qwe",
"status": "completed"
}
]
}
)");
}));
TestFuture<base::expected<std::unique_ptr<Tasks>, ApiErrorCode>> future;
TestFuture<const std::vector<GlanceablesTask>&> future;
auto cancel_closure =
client()->GetTasks(future.GetCallback(), "test-task-list-id");
ASSERT_TRUE(future.Wait());
EXPECT_FALSE(cancel_closure.is_null());
EXPECT_TRUE(future.Get().has_value());
EXPECT_EQ(future.Get().value()->items().size(), 2u);
const auto& root_tasks = future.Get();
EXPECT_EQ(root_tasks.size(), 1u);
EXPECT_EQ(root_tasks.at(0).id, "asd");
EXPECT_EQ(root_tasks.at(0).title, "Parent task, level 1");
EXPECT_EQ(root_tasks.at(0).completed, false);
const auto& subtasks_level_2 = root_tasks.at(0).subtasks;
EXPECT_EQ(subtasks_level_2.size(), 1u);
EXPECT_EQ(subtasks_level_2.at(0).id, "qwe");
EXPECT_EQ(subtasks_level_2.at(0).title, "Child task, level 2");
EXPECT_EQ(subtasks_level_2.at(0).completed, false);
const auto& subtasks_level_3 = subtasks_level_2.at(0).subtasks;
EXPECT_EQ(subtasks_level_3.size(), 1u);
EXPECT_EQ(subtasks_level_3.at(0).id, "zxc");
EXPECT_EQ(subtasks_level_3.at(0).title, "Child task, level 3");
EXPECT_EQ(subtasks_level_3.at(0).completed, true);
}
TEST_F(GlanceablesTasksClientImplTest, GetTasksReturnsEmptyVectorOnHttpError) {
set_generate_response_callback(base::BindLambdaForTesting(
[](const HttpRequest& request) { return CreateFailedResponse(); }));
TestFuture<const std::vector<GlanceablesTask>&> future;
auto cancel_closure =
client()->GetTasks(future.GetCallback(), "test-task-list-id");
ASSERT_TRUE(future.Wait());
EXPECT_FALSE(cancel_closure.is_null());
const auto& root_tasks = future.Get();
EXPECT_EQ(root_tasks.size(), 0u);
}
TEST_F(GlanceablesTasksClientImplTest,
GetTasksReturnsEmptyVectorOnConversionError) {
set_generate_response_callback(
base::BindLambdaForTesting([](const HttpRequest& request) {
return CreateSuccessfulResponse(R"(
{
"kind": "tasks#tasks",
"items": [
{
"id": "asd",
"title": "Parent task",
"status": "needsAction"
},
{
"id": "qwe",
"title": "Child task",
"parent": "asd1",
"status": "needsAction"
}
]
}
)");
}));
TestFuture<const std::vector<GlanceablesTask>&> future;
auto cancel_closure =
client()->GetTasks(future.GetCallback(), "test-task-list-id");
ASSERT_TRUE(future.Wait());
EXPECT_FALSE(cancel_closure.is_null());
const auto& root_tasks = future.Get();
EXPECT_EQ(root_tasks.size(), 0u);
}
} // namespace ash