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.

Sign up for GitHub

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

NTP Sponsored Images schemaVersion support #4433

Merged
merged 2 commits into from
Jan 29, 2020
Merged

NTP Sponsored Images schemaVersion support #4433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83a35b7
NTP Sponsored Images only accepts remote component with known schemaV…
petemill Jan 27, 2020
4c515d1
Fix NTPSponsoredImagesServiceTest failure
simonhong Jan 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions components/ntp_sponsored_images/browser/ntp_sponsored_images_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@
#include "brave/components/ntp_sponsored_images/browser/ntp_sponsored_images_service.h"

#include <algorithm>
#include <string>

#include "base/bind.h"
#include "base/command_line.h"
#include "base/files/file_util.h"
#include "base/json/json_reader.h"
#include "base/optional.h"
#include "base/task/post_task.h"
#include "base/values.h"
#include "brave/components/brave_ads/browser/locale_helper.h"
Expand All @@ -34,6 +36,9 @@ constexpr char kLogoCompanyNamePath[] = "logo.companyName";
constexpr char kLogoDestinationURLPath[] = "logo.destinationUrl";
constexpr char kWallpapersPath[] = "wallpapers";
constexpr char kWallpaperImageURLPath[] = "imageUrl";
constexpr char kSchemaVersionPath[] = "schemaVersion";

constexpr int kExpectedSchemaVersion = 1;

std::string ReadPhotosManifest(const base::FilePath& photos_manifest_path) {
std::string contents;
Expand Down Expand Up @@ -119,11 +124,28 @@ void NTPSponsoredImagesService::OnGetPhotoJsonData(
const std::string& photo_json) {
base::Optional<base::Value> photo_value = base::JSONReader::Read(photo_json);
if (photo_value) {
images_data_.reset(new NTPSponsoredImagesData);

// Resources are stored with json file in the same directory.
base::FilePath base_dir = photos_manifest_path_.DirName();

base::Optional<int> incomingSchemaVersion =
photo_value->FindIntPath(kSchemaVersionPath);
const bool schemaVersionIsValid = incomingSchemaVersion &&
*incomingSchemaVersion == kExpectedSchemaVersion;
if (!schemaVersionIsValid) {
LOG(ERROR) <<
"Incoming NTP Sponsored images component data was not valid."
"Schema version was " <<
(incomingSchemaVersion
? std::to_string(*incomingSchemaVersion)
: "missing") <<
", but we expected " << kExpectedSchemaVersion;
images_data_.reset(nullptr);
Copy link
Member

Choose a reason for hiding this comment

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

nit: images_data_.reset() also works.

NotifyObservers();
return;
}

images_data_.reset(new NTPSponsoredImagesData);

if (auto* logo_image_url = photo_value->FindStringPath(kLogoImageURLPath)) {
images_data_->logo_image_file =
base_dir.AppendASCII(*logo_image_url);
Expand Down
42 changes: 41 additions & 1 deletion components/ntp_sponsored_images/browser/ntp_sponsored_images_service_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,18 @@ TEST(NTPSponsoredImagesServiceTest, InternalDataTest) {
NTPSponsoredImagesService service(nullptr);
service.AddObserver(&observer);

// Check with json file with empty object.
// Check with json file w/o schema version with empty object.
service.ResetImagesDataForTest();
service.OnGetPhotoJsonData("{}");
EXPECT_EQ(nullptr, service.GetSponsoredImagesData());

// Check with json file with empty object.
const std::string test_empty_json_string = R"(
{
"schemaVersion": 1
})";
service.ResetImagesDataForTest();
service.OnGetPhotoJsonData(test_empty_json_string);
auto* data = service.GetSponsoredImagesData();
EXPECT_NE(data, nullptr);
EXPECT_FALSE(data->IsValid());
Expand All @@ -50,6 +59,7 @@ TEST(NTPSponsoredImagesServiceTest, InternalDataTest) {

const std::string test_json_string = R"(
{
"schemaVersion": 1,
"logo": {
"imageUrl": "logo.png",
"alt": "Technikke: For music lovers",
Expand Down Expand Up @@ -84,5 +94,35 @@ TEST(NTPSponsoredImagesServiceTest, InternalDataTest) {
EXPECT_TRUE(observer.called_);
EXPECT_FALSE(observer.data_->logo_alt_text.empty());

// Invalid schema version
const std::string test_json_string_higher_schema = R"(
{
"schemaVersion": 2,
"logo": {
"imageUrl": "logo.png",
"alt": "Technikke: For music lovers",
"destinationUrl": "https://www.brave.com/",
"companyName": "Technikke"
},
"wallpapers": [
{
"imageUrl": "background-1.jpg",
"focalPoint": {}
},
{
"imageUrl": "background-2.jpg",
"focalPoint": {}
},
{
"imageUrl": "background-3.jpg",
"focalPoint": {}
}
]
})";
service.ResetImagesDataForTest();
service.OnGetPhotoJsonData(test_json_string_higher_schema);
data = service.GetSponsoredImagesData();
EXPECT_FALSE(data);

service.RemoveObserver(&observer);
}