Only show G+ promo for users logged into G+

chrome/browser/web_resource/promo_resource_service_unittest.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/json/json_reader.h"
 #include "base/string_number_conversions.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
@@ skipping 85 matching lines @@
       "       {"
       "        \"name\": \"sign_in_promo\","
       "        \"question\": \"";
 
     std::string json_footer = "\""
       "       }"
       "    ]"
       "  }"
       "}";
 
-   scoped_ptr<DictionaryValue> test_json(static_cast<DictionaryValue*>(
+    scoped_ptr<DictionaryValue> test_json(static_cast<DictionaryValue*>(
         base::JSONReader::Read(json_header + question + json_footer, false)));
-   web_resource_service_->UnpackNTPSignInPromoSignal(*(test_json.get()));
+    web_resource_service_->UnpackNTPSignInPromoSignal(*(test_json.get()));
   }
 
   private:
     int notifications_received_;
     content::NotificationRegistrar registrar_;
 };
 
 // Verifies that custom dates read from a web resource server are written to
 // the preferences file.
 TEST_F(PromoResourceServiceTest, UnpackLogoSignal) {
@@ skipping 68 matching lines @@
   // Check that prefs are set correctly.
   web_resource_service_->UnpackLogoSignal(*(test_json.get()));
   logo_start = prefs->GetDouble(prefs::kNTPCustomLogoStart);
   EXPECT_EQ(logo_start, 0);  // date value reset to 0;
   logo_end = prefs->GetDouble(prefs::kNTPCustomLogoEnd);
   EXPECT_EQ(logo_end, 0);  // date value reset to 0;
 }
 
 class NotificationPromoTestDelegate : public NotificationPromo::Delegate {
  public:
-  explicit NotificationPromoTestDelegate(PrefService* prefs)
-      : prefs_(prefs),
+  explicit NotificationPromoTestDelegate(Profile* profile)
+      : profile_(profile),
+        prefs_(profile->GetPrefs()),
         notification_promo_(NULL),
         received_notification_(false),

[achuithb, 2011/11/27 07:48:26]

please initialize should_receive_notification_ here.

[Cait (Slow), 2011/11/28 19:06:25]

Done.

         build_targeted_(true),
         start_(0.0),
         end_(0.0),
         build_(PromoResourceService::NO_BUILD),
         time_slice_(0),
         max_group_(0),
         max_views_(0),
         platform_(NotificationPromo::PLATFORM_NONE),
         text_(),
         closed_(false),
-        current_platform_(NotificationPromo::CurrentPlatform()) {
+        current_platform_(NotificationPromo::CurrentPlatform()),
+        gplus_(false),
+        feature_mask_(0) {
   }
 
   void Init(NotificationPromo* notification_promo,
             const std::string& json,
             double start, double end,
             int build, int time_slice,
             int max_group, int max_views, int platform,
-            const std::string& text, bool closed) {
+            int feature_mask, const std::string& text, bool closed,
+            bool gplus) {
     notification_promo_ = notification_promo;
 
     test_json_.reset(static_cast<DictionaryValue*>(
         base::JSONReader::Read(json, false)));
 
     start_ = start;
     end_ = end;
 
     build_ = build;
     time_slice_ = time_slice;
     max_group_ = max_group;
     max_views_ = max_views;
     platform_ = platform;
 
     text_ = text;
     closed_ = closed;
+    gplus_ = gplus;
+    feature_mask_ = feature_mask;
 
     received_notification_ = false;
   }
 
   // NotificationPromo::Delegate implementation.
-  virtual void OnNewNotification(double start, double end) {
-    EXPECT_EQ(CalcStart(), start);
-    EXPECT_EQ(notification_promo_->StartTimeWithOffset(), start);
-    EXPECT_EQ(notification_promo_->end_, end);
-    received_notification_ = true;
+  virtual void OnNotificationParsed(double start, double end) {
+    if (should_receive_notification_) {
+      EXPECT_EQ(CalcStart(), start);
+      EXPECT_EQ(notification_promo_->StartTimeWithOffset(), start);
+      EXPECT_EQ(notification_promo_->end_, end);
+    }
+    // If notification info needs to be updated, OnNotificationParsed is
+    // called with start and end times, if no update needed, it is called
+    // with default values (0.0, 0.0).
+    received_notification_ = !(start == 0.0 && end == 0.0);
+
+    // Test if notification received
+    EXPECT_TRUE(received_notification_ == should_receive_notification_);
   }
 
   virtual bool IsBuildAllowed(int builds_targeted) const {
     EXPECT_EQ(builds_targeted, build_);
     return build_targeted_;
   }
 
   virtual int CurrentPlatform() const {
     return current_platform_;
   }
 
   const base::DictionaryValue& TestJson() const {
     return *test_json_;
   }
 
   double CalcStart() const {
     return start_ + notification_promo_->group_ * time_slice_ * 60.0 * 60.0;
   }
 
   void InitPromoFromJson(bool should_receive_notification) {
+    should_receive_notification_ = should_receive_notification;
     received_notification_ = false;
-    notification_promo_->InitFromJson(TestJson());
+    notification_promo_->InitFromJson(TestJson(), false);
     EXPECT_TRUE(received_notification_ == should_receive_notification);
 
     // Test the fields.
     TestNotification();
     TestPrefs();
   }
 
+  void ParseAndCallCookies(const DictionaryValue& json,

[achuithb, 2011/11/27 07:48:26]

You don't need this function at all. You're mainly testing CheckForGplusCookie,
so write some tests that pass in different cookie strings, and check the return
value. Eg:
EXPECT_TRUE(NotificationPromo::CheckForGPlusCookie("SID=12346"));
EXPECT_FALSE(NotificationPromo::CheckForGPlusCookie("WARNING=123456");

Also, there's a lot of copy/paste from NotificationPromo::InitFromJson, which
should be avoided. What am I missing?

[Cait (Slow), 2011/11/28 19:06:25]

The purpose of this code is to set the promo values from json w/o triggering an
async cookie fetch or a call directly to CheckForNewNotification (as gplus_ has
to be set first). Your idea of integrating with NotificationPromoTest is much
cleaner and avoids this, though. So I'll do that.

On 2011/11/27 07:48:26, achuith.bhandarkar wrote:

+      const std::string& cookies, bool should_receive_notification,
+      bool should_find_cookie) {
+    should_receive_notification_ = should_receive_notification;
+    received_notification_ = false;
+
+    const char kHeaderProperty[] = "topic";
+    const char kArrayProperty[] = "answers";
+
+    DictionaryValue* dict;
+    if (json.GetDictionary(kHeaderProperty, &dict)) {
+      ListValue* answers;
+      if (dict->GetList(kArrayProperty, &answers)) {
+        for (ListValue::const_iterator it = answers->begin();
+             it != answers->end();
+             ++it) {
+          if ((*it)->IsType(Value::TYPE_DICTIONARY))
+            notification_promo_->Parse(static_cast<DictionaryValue*>(*it));
+        }
+      }
+    }
+
+    bool found_cookie = notification_promo_->CheckForGPlusCookie(cookies);
+    EXPECT_TRUE(found_cookie == should_find_cookie);
+
+    notification_promo_->CheckForNewNotification(found_cookie);
+    EXPECT_TRUE(received_notification_ == should_receive_notification);
+
+    // Test the fields.
+    TestNotification();
+    TestPrefs();
+  }
+
   void TestNotification() {
     // Check values.
     EXPECT_EQ(notification_promo_->start_, start_);
     EXPECT_EQ(notification_promo_->end_, end_);
     EXPECT_EQ(notification_promo_->build_, build_);
     EXPECT_EQ(notification_promo_->time_slice_, time_slice_);
     EXPECT_EQ(notification_promo_->max_group_, max_group_);
     EXPECT_EQ(notification_promo_->max_views_, max_views_);
     EXPECT_EQ(notification_promo_->platform_, platform_);
     EXPECT_EQ(notification_promo_->text_, text_);
     EXPECT_EQ(notification_promo_->closed_, closed_);
+    EXPECT_EQ(notification_promo_->gplus_, gplus_);
+    EXPECT_EQ(notification_promo_->feature_mask_, feature_mask_);
 
     // Check group within bounds.
     EXPECT_GE(notification_promo_->group_, 0);
     EXPECT_LT(notification_promo_->group_, 100);
 
     // Views should be 0 for now.
     EXPECT_EQ(notification_promo_->views_, 0);
 
     // Check calculated time.
     EXPECT_EQ(notification_promo_->StartTimeWithOffset(), CalcStart());
   }
 
   void TestPrefs() {
     EXPECT_EQ(prefs_->GetDouble(prefs::kNTPPromoStart), start_);
     EXPECT_EQ(prefs_->GetDouble(prefs::kNTPPromoEnd), end_);
 
     EXPECT_EQ(prefs_->GetInteger(prefs::kNTPPromoBuild), build_);
     EXPECT_EQ(prefs_->GetInteger(prefs::kNTPPromoGroupTimeSlice), time_slice_);
     EXPECT_EQ(prefs_->GetInteger(prefs::kNTPPromoGroupMax), max_group_);
     EXPECT_EQ(prefs_->GetInteger(prefs::kNTPPromoViewsMax), max_views_);
     EXPECT_EQ(prefs_->GetInteger(prefs::kNTPPromoPlatform), platform_);
 
     EXPECT_EQ(prefs_->GetInteger(prefs::kNTPPromoGroup),
         notification_promo_ ? notification_promo_->group_: 0);
     EXPECT_EQ(prefs_->GetInteger(prefs::kNTPPromoViews), 0);
     EXPECT_EQ(prefs_->GetString(prefs::kNTPPromoLine), text_);
     EXPECT_EQ(prefs_->GetBoolean(prefs::kNTPPromoClosed), closed_);
+    EXPECT_EQ(prefs_->GetBoolean(prefs::kNTPPromoIsLoggedInToPlus), gplus_);
+    EXPECT_EQ(prefs_->GetInteger(prefs::kNTPPromoFeatureMask), feature_mask_);
   }
 
   // Create a new NotificationPromo from prefs and compare to current
   // notification.
   void TestInitFromPrefs() {
-    NotificationPromo prefs_notification_promo(prefs_, this);
-    prefs_notification_promo.InitFromPrefs();
-    const bool is_equal = *notification_promo_ == prefs_notification_promo;
+    scoped_refptr<NotificationPromo> prefs_notification_promo =
+    NotificationPromo::Factory(profile_, this);
+    prefs_notification_promo->InitFromPrefs();
+    const bool is_equal = *notification_promo_ == *prefs_notification_promo;
     EXPECT_TRUE(is_equal);
   }
 
   void TestGroup() {
     // Test out of range groups.
     for (int i = max_group_; i <= NotificationPromo::kMaxGroupSize; ++i) {
       notification_promo_->group_ = i;
       EXPECT_FALSE(notification_promo_->CanShow());
     }
 
@@ skipping 177 matching lines @@
     // End time has past.
     notification_promo_->start_ = now - qhour;
     notification_promo_->end_ = now - qhour;
     EXPECT_FALSE(notification_promo_->CanShow());
 
     notification_promo_->start_ = start_;
     notification_promo_->end_ = end_;
     EXPECT_TRUE(notification_promo_->CanShow());
   }
 
+  void TestFeatureMask() {
+    // If no feature mask, gplus_ value is ignored.
+    notification_promo_->gplus_ = false;
+    notification_promo_->feature_mask_ = 0;
+    EXPECT_TRUE(notification_promo_->CanShow());
+
+    notification_promo_->gplus_ = true;
+    notification_promo_->feature_mask_ = 0;
+    EXPECT_TRUE(notification_promo_->CanShow());
+
+    // No gplus cookie, feature mask in use.
+    notification_promo_->gplus_ = false;
+    notification_promo_->feature_mask_ = 1;

[achuithb, 2011/11/27 07:48:26]

Please use the value NotificationPromo::FEATURE_GPLUS instead. It's clearer and
less fragile.

[Cait (Slow), 2011/11/28 19:06:25]

Done.

+    EXPECT_FALSE(notification_promo_->CanShow());

[achuithb, 2011/11/27 07:48:26]

I would EXPECT_TRUE here, based on the inversion of logic for this feature.

[Cait (Slow), 2011/11/28 19:06:25]

We want to show the promo if the user is already logged in to gplus, so it
should not show if no cookie is found.

On 2011/11/27 07:48:26, achuith.bhandarkar wrote:

+
+    // Gplus cookie, feature mask in use.
+    notification_promo_->gplus_ = true;
+    notification_promo_->feature_mask_ = 1;
+    EXPECT_TRUE(notification_promo_->CanShow());
+  }
+
  private:
+  Profile* profile_;
   PrefService* prefs_;
   NotificationPromo* notification_promo_;
   bool received_notification_;
+  bool should_receive_notification_;
   bool build_targeted_;
   scoped_ptr<DictionaryValue> test_json_;
 
   double start_;
   double end_;
 
   int build_;
   int time_slice_;
   int max_group_;
   int max_views_;
   int platform_;
+  int feature_mask_;
 
   std::string text_;
   bool closed_;
 
+  bool gplus_;
   int current_platform_;
 };
 
 TEST_F(PromoResourceServiceTest, NotificationPromoTest) {
   // Check that prefs are set correctly.
   PrefService* prefs = profile_.GetPrefs();
   ASSERT_TRUE(prefs != NULL);
 
-  NotificationPromoTestDelegate delegate(prefs);
-  NotificationPromo notification_promo(prefs, &delegate);
+  NotificationPromoTestDelegate delegate(&profile_);
+  scoped_refptr<NotificationPromo> notification_promo =
+      NotificationPromo::Factory(&profile_, &delegate);
 
   // Make sure prefs are unset.
   delegate.TestPrefs();
 
   // Set up start and end dates and promo line in a Dictionary as if parsed
   // from the service.
-  delegate.Init(&notification_promo,
+  delegate.Init(notification_promo,
                 "{ "
                 "  \"topic\": {"
                 "    \"answers\": ["
                 "       {"
                 "        \"name\": \"promo_start\","
-                "        \"question\": \"3:2:5:10:15\","
+                "        \"question\": \"3:2:5:10:15:0\","
                 "        \"tooltip\": \"Eat more pie!\","
                 "        \"inproduct\": \"31/01/10 01:00 GMT\""
                 "       },"
                 "       {"
                 "        \"name\": \"promo_end\","
                 "        \"inproduct\": \"31/01/12 01:00 GMT\""

[achuithb, 2011/11/27 07:48:26]

Could you please update this date? This test is going to start failing in 2+
months.

                 "       }"
                 "    ]"
                 "  }"
                 "}",
                 1264899600,  // unix epoch for Jan 31 2010 0100 GMT.
                 1327971600,  // unix epoch for Jan 31 2012 0100 GMT.
-                3, 2, 5, 10, 15,
-                "Eat more pie!", false);
+                3, 2, 5, 10, 15, 0,
+                "Eat more pie!", false, false);
 
   delegate.InitPromoFromJson(true);
 
   // Second time should not trigger a notification.
   delegate.InitPromoFromJson(false);
 
   delegate.TestInitFromPrefs();
 
   // Test various conditions of CanShow.
   // TestGroup Has the side effect of setting us to a passing group.
   delegate.TestGroup();
   delegate.TestViews();
   delegate.TestBuild();
   delegate.TestClosed();
   delegate.TestText();
   delegate.TestTime();
+  delegate.TestFeatureMask();
   delegate.TestPlatforms();
 }
 
 TEST_F(PromoResourceServiceTest, NotificationPromoTestFail) {
   // Check that prefs are set correctly.
   PrefService* prefs = profile_.GetPrefs();
   ASSERT_TRUE(prefs != NULL);
 
-  NotificationPromoTestDelegate delegate(prefs);
-  NotificationPromo notification_promo(prefs, &delegate);
+  NotificationPromoTestDelegate delegate(&profile_);
+  scoped_refptr<NotificationPromo> notification_promo =
+      NotificationPromo::Factory(&profile_, &delegate);
 
   // Set up start and end dates and promo line in a Dictionary as if parsed
   // from the service.
-  delegate.Init(&notification_promo,
+  delegate.Init(notification_promo,
                 "{ "
                 "  \"topic\": {"
                 "    \"answers\": ["
                 "       {"
                 "        \"name\": \"promo_start\","
-                "        \"question\": \"12:8:10:20:15\","
+                "        \"question\": \"12:8:10:20:15:0\","
                 "        \"tooltip\": \"Happy 3rd Birthday!\","
                 "        \"inproduct\": \"09/15/10 05:00 PDT\""
                 "       },"
                 "       {"
                 "        \"name\": \"promo_end\","
                 "        \"inproduct\": \"09/30/10 05:00 PDT\""
                 "       }"
                 "    ]"
                 "  }"
                 "}",
                 1284552000,  // unix epoch for Sep 15 2010 0500 PDT.
                 1285848000,  // unix epoch for Sep 30 2010 0500 PDT.
-                12, 8, 10, 20, 15,
-                "Happy 3rd Birthday!", false);
+                12, 8, 10, 20, 15, 0,
+                "Happy 3rd Birthday!", false, false);
 
   delegate.InitPromoFromJson(true);
 
   // Second time should not trigger a notification.
   delegate.InitPromoFromJson(false);
 
   delegate.TestInitFromPrefs();
 
   // Should fail because out of time bounds.
-  EXPECT_FALSE(notification_promo.CanShow());
+  EXPECT_FALSE(notification_promo->CanShow());
 }
 
+TEST_F(PromoResourceServiceTest, NotificationPromoCookieTest) {

[achuithb, 2011/11/27 07:48:26]

Please delete this test - it has a lot of copy/paste. Replace it with a test
that just tests CheckForGPlusCookie.

If you want to exercise the ability for the cookie to not trigger a promo, try
to integrate it with NotificationPromoTest instead.

+  // Check that prefs are set correctly.
+  PrefService* prefs = profile_.GetPrefs();
+  ASSERT_TRUE(prefs != NULL);
+
+  NotificationPromoTestDelegate delegate(&profile_);
+  scoped_refptr<NotificationPromo> notification_promo =
+      NotificationPromo::Factory(&profile_, &delegate);
+
+  // Make sure prefs are unset.
+  delegate.TestPrefs();
+
+  // Set up start and end dates and promo line in a Dictionary as if parsed
+  // from the service.
+  delegate.Init(notification_promo,
+                "{ "
+                "  \"topic\": {"
+                "    \"answers\": ["
+                "       {"
+                "        \"name\": \"promo_start\","
+                "        \"question\": \"3:2:5:10:15:1\","
+                "        \"tooltip\": \"Eat more pie!\","
+                "        \"inproduct\": \"31/01/10 01:00 GMT\""
+                "       },"
+                "       {"
+                "        \"name\": \"promo_end\","
+                "        \"inproduct\": \"31/01/12 01:00 GMT\""
+                "       }"
+                "    ]"
+                "  }"
+                "}",
+                1264899600,  // unix epoch for Jan 31 2010 0100 GMT.
+                1327971600,  // unix epoch for Jan 31 2012 0100 GMT.
+                3, 2, 5, 10, 15, 1,
+                "Eat more pie!", false, true);
+
+  delegate.ParseAndCallCookies(delegate.TestJson(), "SID=123456;", true, true);
+  delegate.ParseAndCallCookies(delegate.TestJson(), "SID=123456;", false, true);
+
+  delegate.TestInitFromPrefs();
+
+  // Test various conditions of CanShow.
+  // TestGroup Has the side effect of setting us to a passing group.
+  delegate.TestGroup();
+  delegate.TestViews();
+  delegate.TestBuild();
+  delegate.TestClosed();
+  delegate.TestText();
+  delegate.TestTime();
+  delegate.TestFeatureMask();
+  delegate.TestPlatforms();
+}

[achuithb, 2011/11/27 07:48:26]

Delete this test too. It's a bit difficult to maintain all the copy/paste code
here.

+TEST_F(PromoResourceServiceTest, NotificationPromoCookieTestFail) {
+  // Check that prefs are set correctly.
+  PrefService* prefs = profile_.GetPrefs();
+  ASSERT_TRUE(prefs != NULL);
+
+  NotificationPromoTestDelegate delegate(&profile_);
+  scoped_refptr<NotificationPromo> notification_promo =
+      NotificationPromo::Factory(&profile_, &delegate);
+
+  // Make sure prefs are unset.
+  delegate.TestPrefs();
+
+  // Set up start and end dates and promo line in a Dictionary as if parsed
+  // from the service.
+  delegate.Init(notification_promo,
+                "{ "
+                "  \"topic\": {"
+                "    \"answers\": ["
+                "       {"
+                "        \"name\": \"promo_start\","
+                "        \"question\": \"3:2:5:10:15:1\","
+                "        \"tooltip\": \"Eat more pie!\","
+                "        \"inproduct\": \"31/01/10 01:00 GMT\""
+                "       },"
+                "       {"
+                "        \"name\": \"promo_end\","
+                "        \"inproduct\": \"31/01/12 01:00 GMT\""
+                "       }"
+                "    ]"
+                "  }"
+                "}",
+                1264899600,  // unix epoch for Jan 31 2010 0100 GMT.
+                1327971600,  // unix epoch for Jan 31 2012 0100 GMT.
+                3, 2, 5, 10, 15, 1,
+                "Eat more pie!", false, false);
+
+  delegate.ParseAndCallCookies(delegate.TestJson(), "WRONG=123456;",
+                               true, false);
+  // Should not trigger notification on second call.
+  delegate.ParseAndCallCookies(delegate.TestJson(), "WRONG=123456;",
+                               false, false);
+
+  delegate.TestInitFromPrefs();
+
+  // Should fail because cookie is required but was not set.
+  EXPECT_FALSE(notification_promo->CanShow());
+}
 TEST_F(PromoResourceServiceTest, GetNextQuestionValueTest) {
-  const std::string question("0:-100:2048:0:2a");
-  const int question_vec[] = { 0, -100, 2048, 0 };
+  const std::string question("0:-100:2048:0:0:0:2a");
+  const int question_vec[] = { 0, -100, 2048, 0, 0, 0};
   size_t index = 0;
   bool err = false;
 
   for (size_t i = 0; i < arraysize(question_vec); ++i) {
     EXPECT_EQ(question_vec[i],
         NotificationPromo::GetNextQuestionValue(question, &index, &err));
     EXPECT_FALSE(err);
   }
   EXPECT_EQ(NotificationPromo::GetNextQuestionValue(question, &index, &err), 0);
   EXPECT_TRUE(err);
@@ skipping 391 matching lines @@
   EXPECT_TRUE(PromoResourceService::IsBuildTargeted(stable, 7));
   EXPECT_TRUE(PromoResourceService::IsBuildTargeted(stable, 15));
   EXPECT_FALSE(PromoResourceService::IsBuildTargeted(stable, 8));
   EXPECT_FALSE(PromoResourceService::IsBuildTargeted(stable, 11));
   EXPECT_TRUE(PromoResourceService::IsBuildTargeted(stable, 12));
 
   // invalid
   EXPECT_FALSE(PromoResourceService::IsBuildTargeted(stable, -1));
   EXPECT_FALSE(PromoResourceService::IsBuildTargeted(stable, INT_MAX));
 }