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),
+        should_receive_notification_(false),
         received_notification_(false),
         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,
+                                    bool new_notification) {
+    if (should_receive_notification_) {
+      EXPECT_EQ(CalcStart(), start);
+      EXPECT_EQ(notification_promo_->StartTimeWithOffset(), start);
+      EXPECT_EQ(notification_promo_->end_, end);
+    }
+
+    received_notification_ = new_notification;
+
+    // Test if notification received

[achuithb, 2011/11/28 21:21:22]

Period at the end of the comment (not sure if this comment is necessary).

[Cait (Slow), 2011/11/29 00:12:27]

Done.

+    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 SetFeatureMaskToGPlus() {

[achuithb, 2011/11/28 21:21:22]

I don't think this function is necessary if you create a separate TestGPlus
function as I suggest below.

[Cait (Slow), 2011/11/29 00:12:27]

Done.

+    feature_mask_ = NotificationPromo::FEATURE_GPLUS;
+    notification_promo_->feature_mask_ = NotificationPromo::FEATURE_GPLUS;
+  }
+
+  void RunCookieTests(const DictionaryValue& json,

[achuithb, 2011/11/28 21:21:22]

Could you rename this to TestCookie or TestGPlusCookie for consistency? Also,
you're not using the first argument json at all, so please just remove it.

[Cait (Slow), 2011/11/29 00:12:27]

Done.

+      const std::string& cookies, bool should_receive_notification,
+      bool should_find_cookie) {
+    gplus_ = should_find_cookie;
+    should_receive_notification_ = should_receive_notification;
+    received_notification_ = false;
+
+    bool found_cookie = NotificationPromo::CheckForGPlusCookie(cookies);
+    EXPECT_TRUE(found_cookie == should_find_cookie);
+
+    notification_promo_->CheckForNewNotification(found_cookie);
+    EXPECT_TRUE(received_notification_ == should_receive_notification);
+
+    // Test the fields.
+    EXPECT_EQ(notification_promo_->gplus_, gplus_);
+    EXPECT_EQ(notification_promo_->feature_mask_, feature_mask_);
+    // Test the prefs.
+    EXPECT_EQ(prefs_->GetBoolean(prefs::kNTPPromoIsLoggedInToPlus), gplus_);
+    EXPECT_EQ(prefs_->GetInteger(prefs::kNTPPromoFeatureMask), feature_mask_);

[achuithb, 2011/11/28 21:21:22]

Is there a way to also test CanShow() here? shouldn't it be equal to
should_find_cookie?

[Cait (Slow), 2011/11/29 00:12:27]

Done.

+  }
   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::Create(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.

[achuithb, 2011/11/28 21:21:22]

Could you move this sub-test (no feature mask, gplus_ false)  to be the final
case in TestFeatureMask? That way you leave notification_promo_ the way you
found it. In this current implementation, you have a side effect of setting
gplus_ to true and feature_mask_ to FEATURE_GPLUS. If this test was commented
out, some succeeding tests could fail.

[Cait (Slow), 2011/11/29 00:12:27]

Done.

+    notification_promo_->gplus_ = false;
+    notification_promo_->feature_mask_ = NotificationPromo::NO_FEATURE;
+    EXPECT_TRUE(notification_promo_->CanShow());
+
+    notification_promo_->gplus_ = true;
+    notification_promo_->feature_mask_ = NotificationPromo::NO_FEATURE;
+    EXPECT_TRUE(notification_promo_->CanShow());
+
+    // No gplus cookie, feature mask in use.
+    notification_promo_->gplus_ = false;
+    notification_promo_->feature_mask_ = NotificationPromo::FEATURE_GPLUS;
+    EXPECT_FALSE(notification_promo_->CanShow());
+
+    // Gplus cookie, feature mask in use.
+    notification_promo_->gplus_ = true;
+    notification_promo_->feature_mask_ = NotificationPromo::FEATURE_GPLUS;
+    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::Create(&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\""
+                "        \"inproduct\": \"31/01/14 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,
-                "Eat more pie!", false);
+                1391130000,  // unix epoch for Jan 31 2012 0100 GMT.
+                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();
+
+  delegate.SetFeatureMaskToGPlus();

[achuithb, 2011/11/28 21:21:22]

could you move lines 664-675 into a separate function? Call it TestGplus
perhaps? At the end of that function, please reset the feature_mask_ to 0 and
set gplus_ to false to reset notification_promo_ to the same state as you found
it.

[Cait (Slow), 2011/11/29 00:12:27]

Done.

+
+  delegate.RunCookieTests(delegate.TestJson(), "SID=123456;", true, true);
+  // Should not trigger notification on second call.
+  delegate.RunCookieTests(delegate.TestJson(), "SID=123456;", false, true);
+
+  delegate.RunCookieTests(delegate.TestJson(), "WRONG=123456;", true, false);
+  // Should not trigger notification on second call.
+  delegate.RunCookieTests(delegate.TestJson(), "WRONG=123456;", false, false);
+
+  // Should fail because cookie is required but was not set.
+  EXPECT_FALSE(notification_promo->CanShow());

[achuithb, 2011/11/28 21:21:22]

Could you reverse the order so you test failing cookies, then passing cookies?
That way, when somebody adds more tests, notification_promo->CanShow() will
continue to be true (as it is at the end of all the previous tests).

[Cait (Slow), 2011/11/29 00:12:27]

Done.

 }
 
 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::Create(&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, 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));
 }