Only show G+ promo for users logged into G+

chrome/browser/web_resource/promo_resource_service_unittest.cc

Index: chrome/browser/web_resource/promo_resource_service_unittest.cc
===================================================================
--- chrome/browser/web_resource/promo_resource_service_unittest.cc	(revision 111558)
+++ chrome/browser/web_resource/promo_resource_service_unittest.cc	(working copy)
@@ -103,9 +103,9 @@
       "  }"
       "}";
 
-   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:
@@ -194,8 +194,9 @@
 
 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),
@@ -208,7 +209,9 @@
         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,
@@ -216,7 +219,8 @@
             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*>(
@@ -233,16 +237,26 @@
 
     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 {
@@ -263,8 +277,9 @@
   }
 
   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.
@@ -272,6 +287,39 @@
     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_);
@@ -283,6 +331,8 @@
     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);
@@ -310,14 +360,17 @@
     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);
   }
 
@@ -515,10 +568,33 @@
     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_;
 
@@ -530,10 +606,12 @@
   int max_group_;
   int max_views_;
   int platform_;
+  int feature_mask_;
 
   std::string text_;
   bool closed_;
 
+  bool gplus_;
   int current_platform_;
 };
 
@@ -542,21 +620,22 @@
   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\""
                 "       },"
@@ -569,8 +648,8 @@
                 "}",
                 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);
 
@@ -587,6 +666,7 @@
   delegate.TestClosed();
   delegate.TestText();
   delegate.TestTime();
+  delegate.TestFeatureMask();
   delegate.TestPlatforms();
 }
 
@@ -595,18 +675,19 @@
   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\""
                 "       },"
@@ -619,8 +700,8 @@
                 "}",
                 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);
 
@@ -630,12 +711,111 @@
   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;