Merge pull request #47 from brave-intl/save-bundle

restore SaveBundle method

include/bat/ads/ads_client.h

@@ -78,6 +78,10 @@ class ADS_EXPORT AdsClient {
       const std::string& name,
       OnResetCallback callback) = 0;
 
+  virtual void SaveBundleState(
+      std::unique_ptr<BUNDLE_STATE> bundle_state,
+      OnSaveCallback callback) = 0;
+
   // Gets available ads based upon the winning category
   virtual void GetCategory(
       const std::string& winning_category,

src/ads_impl.cc

@@ -291,13 +291,6 @@ void AdsImpl::SaveCachedInfo() {
   // class
   ads_client_->Save("client.json", client_->ToJson(),
     std::bind(&AdsImpl::OnClientSaved, this, _1));
-
-  // TODO(Brian Johnson): The following change breaks the code as the bundle
-  // should not be saved as json and should be passed as a data structure and
-  // stored in a database so that fetching of results is optimized for both
-  // memory consumption and performance
-  ads_client_->Save("bundle.json", bundle_->ToJson(),
-    std::bind(&AdsImpl::OnBundleSaved, this, _1));
 }
 
 void AdsImpl::RecordMediaPlaying(
@@ -506,15 +499,6 @@ void AdsImpl::OnUserModelLoaded(const Result result, const std::string& json) {
   }
 }
 
-void AdsImpl::OnBundleSaved(const Result result) {
-  if (result == Result::FAILED) {
-    LOG(ads_client_, LogLevel::ERROR) << "Failed to save bundle";
-    return;
-  }
-
-  LOG(ads_client_, LogLevel::INFO) << "Successfully saved bundle";
-}
-
 void AdsImpl::OnBundleReset(const Result result) {
   if (result == Result::FAILED) {
     LOG(ads_client_, LogLevel::ERROR) << "Failed to reset bundle";
@@ -561,16 +545,7 @@ void AdsImpl::Deinitialize() {
 
   last_page_classification_ = "";
 
-  // TODO(Brian Johnson): ads_client_->ResetCatalog(); was moved from here to
-  // ads_serve::Reset however ads serve is only responsible for downloading
-  // catalogs, so this should be re-implemented please
-
   bundle_->Reset();
-  // TODO(Brian Johnson): ads_client_->Reset call below should be moved to
-  // bundle::Reset (i.e. inside the above bundle_->Reset() function call),
-  // could also be moved into bundle_->Reset() function
-  ads_client_->Reset("bundle.json",
-    std::bind(&AdsImpl::OnBundleSaved, this, _1));
 
   user_model_.reset();
 

src/ads_serve.cc

@@ -108,15 +108,9 @@ void AdsServe::UpdateNextCatalogCheck() {
 //////////////////////////////////////////////////////////////////////////////
 
 void AdsServe::ProcessCatalog(const std::string& json) {
-  // TODO(Brian Johnson): I have added the catalog class back to the project
-  // can you please change the code below to instantiate a catalog which
-  // should be responsible for the catalog state as there is custom logic
-  // in the catalog class concerning catalog_id's and I am sure further logic
-  // will be added in the future, the catalog class does have a shared_ptr
-  // which will also need changing
-  CATALOG_STATE catalog_state;
-  auto jsonSchema = ads_client_->Load("catalog-schema.json");
-  if (!catalog_state.LoadFromJson(json, jsonSchema)) {
+  Catalog catalog(bundle_, ads_client_);
+
+  if (!catalog.FromJson(json)) {
     // TODO(Terry Mancey): Implement Log (#44)
     // 'Failed to parse catalog'
 
@@ -129,17 +123,7 @@ void AdsServe::ProcessCatalog(const std::string& json) {
   // { status: 'processed', campaigns: underscore.keys(campaigns).length,
   // creativeSets: underscore.keys(creativeSets).length
 
-  auto current_catalog_version = bundle_->GetCatalogVersion();
-  if (current_catalog_version != 0 &&
-      current_catalog_version <= catalog_state.version) {
-    // TODO(Terry Mancey): Implement Log (#44)
-    // 'Current catalog is the same or a newer version'
-
-    RetryDownloadingCatalog();
-    return;
-  }
-
-  if (!bundle_->GenerateFromCatalog(catalog_state)) {
+  if (!bundle_->UpdateFromCatalog(catalog)) {
     // TODO(Terry Mancey): Implement Log (#44)
     // 'Failed to generate bundle'
 
@@ -150,12 +134,6 @@ void AdsServe::ProcessCatalog(const std::string& json) {
   // TODO(Terry Mancey): Implement Log (#44)
   // 'Generated bundle'
 
-  // TODO(Brian Johnson): Saving of the catalog should be changed back to
-  // saving in the catalog class as it is not the responsibility of the ads
-  // serve class
-  ads_client_->Save("catalog.json", json,
-    std::bind(&AdsServe::OnCatalogSaved, this, _1));
-
   ads_->SaveCachedInfo();
 
   UpdateNextCatalogCheck();

src/bundle.cc

@@ -2,7 +2,11 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+#include "bat/ads/bundle_state.h"
 #include "bundle.h"
+#include "catalog.h"
+#include "json_helper.h"
+#include "logging.h"
 #include "string_helper.h"
 #include "static_values.h"
 
@@ -12,33 +16,19 @@ namespace ads {
 
 Bundle::Bundle(AdsClient* ads_client) :
     ads_client_(ads_client),
-    bundle_state_(new BUNDLE_STATE()) {
+    catalog_id_(""),
+    catalog_version_(0),
+    catalog_ping_(0) {
 }
 
 Bundle::~Bundle() = default;
 
-bool Bundle::FromJson(const std::string& json) {
-  BUNDLE_STATE state;
-  auto jsonSchema = ads_client_->Load("bundle-schema.json");
-  if (!LoadFromJson(state, json.c_str(), jsonSchema)) {
-    return false;
-  }
-
-  bundle_state_.reset(new BUNDLE_STATE(state));
-  return true;
-}
-
-const std::string Bundle::ToJson() {
-  std::string json;
-  SaveToJson(*bundle_state_, json);
-  return json;
-}
-
-bool Bundle::GenerateFromCatalog(
-    const CATALOG_STATE& catalog_state) {
+bool Bundle::UpdateFromCatalog(
+    const Catalog& catalog) {
+  // TODO(bridiver) - add callback for this method
   std::map<std::string, std::vector<AdInfo>> categories;
 
-  for (const auto& campaign : catalog_state.campaigns) {
+  for (const auto& campaign : catalog.GetCampaigns()) {
     std::vector<std::string> heirarchy = {};
 
     for (const auto& creative_set : campaign.creative_sets) {
@@ -91,34 +81,107 @@ bool Bundle::GenerateFromCatalog(
     }
   }
 
-  BUNDLE_STATE state;
-  state.catalog_id = catalog_state.catalog_id;
-  state.catalog_version = catalog_state.version;
-  state.catalog_ping = catalog_state.ping;
-  state.categories = categories;
+  auto state = std::make_unique<BUNDLE_STATE>();
+  state->catalog_id = catalog.GetId();
+  state->catalog_version = catalog.GetVersion();
+  state->catalog_ping = catalog.GetPing();
+  state->categories = categories;
 
-  bundle_state_.reset(new BUNDLE_STATE(state));
+  InitializeFromBundleState(std::move(state));
 
   return true;
 }
 
 void Bundle::Reset() {
-  bundle_state_.reset(new BUNDLE_STATE());
-  // TODO(Brian Johnson): Save() was removed from here, this is important
-  // otherwise the client will have a previous bundle, see commit #e82bda44
-  // where Save() was removed
+  // TODO(bridiver) - should we set to null here instead?
+  InitializeFromBundleState(nullptr);
 }
 
-std::string Bundle::GetCatalogId() const {
-  return bundle_state_->catalog_id;
+const std::string Bundle::GetCatalogId() const {
+  return catalog_id_;
 }
 
 uint64_t Bundle::GetCatalogVersion() const {
-  return bundle_state_->catalog_version;
+  return catalog_version_;
 }
 
 uint64_t Bundle::GetCatalogPing() const {
-  return bundle_state_->catalog_ping / kMillisecondsInASecond;
+  return catalog_ping_ / kMillisecondsInASecond;
+}
+
+void Bundle::InitializeFromBundleState(std::unique_ptr<BUNDLE_STATE> state) {
+  if (!state) {
+    // ads_client_->ResetBundleState(
+    //     std::bind(&Bundle::OnBundleStateReset, this, _1));
+    ads_client_->Reset("bundle.json",
+        std::bind(&Bundle::OnBundleSavedForTesting, this, _1));
+    return;
+  }
+
+  // TODO(bridiver) - this is for debugging only so maybe we should just
+  // do a VLOG or something instead of saving to disk?
+  ToJsonForTesting(*state);
+
+  ads_client_->SaveBundleState(std::move(state),
+      std::bind(&Bundle::OnBundleStateSaved, this,
+          state->catalog_id, state->catalog_version, state->catalog_ping, _1));
+}
+
+void Bundle::OnBundleStateSaved(const std::string& catalog_id,
+                                const uint64_t& catalog_version,
+                                const uint64_t& catalog_ping,
+                                Result result) {
+  if (result == Result::FAILED) {
+    LOG(ads_client_, LogLevel::ERROR) << "Failed to save bundle";
+    // TODO(bridiver) - seems like we should do more than just log here?
+    return;
+  }
+
+  catalog_id_ = catalog_id;
+  catalog_version_ = catalog_version;
+  catalog_ping_ = catalog_ping;
+
+  LOG(ads_client_, LogLevel::INFO) << "Bundle saved";
+}
+
+void Bundle::OnBundleStateReset(Result result) {
+  if (result == Result::FAILED) {
+    LOG(ads_client_, LogLevel::ERROR) << "Bundle reset failed";
+    return;
+  }
+
+  LOG(ads_client_, LogLevel::ERROR) << "Bundle reset";
+  catalog_id_ = "";
+  catalog_version_ = 0;
+  catalog_ping_ = 0;
+}
+
+void Bundle::ToJsonForTesting(const BUNDLE_STATE& state) {
+  std::string json;
+  SaveToJson(state, json);
+  ads_client_->Save("bundle.json", json,
+    std::bind(&Bundle::OnBundleSavedForTesting, this, _1));
+}
+
+void Bundle::OnBundleSavedForTesting(const Result result) {
+  if (result == Result::FAILED) {
+    LOG(ads_client_, LogLevel::ERROR) << "Save json bundle failed";
+    return;
+  }
+
+  LOG(ads_client_, LogLevel::INFO) << "Bundle json saved";
+}
+
+// see comment above about VLOG vs saving to disk
+bool Bundle::FromJsonForTesting(const std::string& json) {
+  auto state = std::make_unique<BUNDLE_STATE>();
+  auto jsonSchema = ads_client_->Load("bundle-schema.json");
+  if (!LoadFromJson(*state, json.c_str(), jsonSchema)) {
+    return false;
+  }
+
+  InitializeFromBundleState(std::move(state));
+  return true;
 }
 
 }  // namespace ads

src/bundle.h

@@ -8,30 +8,43 @@
 #include <memory>
 
 #include "bat/ads/ads_client.h"
-#include "bat/ads/bundle_state.h"
-#include "catalog_state.h"
+#include "catalog.h"
 
 namespace ads {
 
+struct BUNDLE_STATE;
+
 class Bundle {
  public:
   explicit Bundle(AdsClient* ads_client);
   ~Bundle();
 
-  bool FromJson(const std::string& json);  // Deserialize
-  const std::string ToJson();  // Serialize
-
-  bool GenerateFromCatalog(const CATALOG_STATE& catalog_state);
+  bool UpdateFromCatalog(const Catalog& catalog);
 
   void Reset();
 
-  std::string GetCatalogId() const;
+  const std::string GetCatalogId() const;
   uint64_t GetCatalogVersion() const;
   uint64_t GetCatalogPing() const;
 
+  bool FromJsonForTesting(const std::string& json);  // Deserialize
+
  private:
+  void InitializeFromBundleState(std::unique_ptr<BUNDLE_STATE> state);
+  void OnBundleStateSaved(const std::string& catalog_id,
+                          const uint64_t& catalog_version,
+                          const uint64_t& catalog_ping,
+                          Result result);
+  void OnBundleStateReset(Result result);
+  void ToJsonForTesting(const BUNDLE_STATE& state);
+  void OnBundleSavedForTesting(Result result);
+
   AdsClient* ads_client_;  // NOT OWNED
 
+  std::string catalog_id_;
+  uint64_t catalog_version_;
+  uint64_t catalog_ping_;
+
   std::unique_ptr<BUNDLE_STATE> bundle_state_;
 };