Add UserStatsLoader.

editor/editor_tests/user_stats_test.cpp

@@ -2,19 +2,56 @@
 
 #include "editor/user_stats.hpp"
 
+#include "platform/platform_tests_support/writable_dir_changer.hpp"
+
 namespace editor
 {
 namespace
 {
-UNIT_TEST(UserStats_Smoke)
+auto constexpr kEditorTestDir = "editor-tests";
+auto constexpr kUserName = "Vladimir BI";
+
+UNIT_TEST(UserStatsLoader_Smoke)
 {
-  // This user made only two changes and the possibility of further changes is very low.
-  UserStats userStats("Vladimir BI");
-  TEST(userStats.GetUpdateStatus(), ());
-  TEST(userStats.IsChangesCountInitialized(), ());
-  TEST(userStats.IsRankInitialized(), ());
-  TEST_EQUAL(userStats.GetChangesCount(), 2, ());
-  TEST_GREATER_OR_EQUAL(userStats.GetRank(), 5800, ());
+  WritableDirChanger wdc(kEditorTestDir, WritableDirChanger::SettingsDirPolicy::UseWritableDir);
+
+  {
+     UserStatsLoader statsLoader;
+     TEST(!statsLoader.GetStats(kUserName), ());
+  }
+
+  {
+    // This user made only two changes and the possibility of further changes is very low.
+    UserStatsLoader statsLoader;
+
+    statsLoader.Update(kUserName);
+    auto const userStats = statsLoader.GetStats(kUserName);
+
+    TEST(userStats, ());
+    int32_t rank, changesCount;
+    TEST(userStats.GetRank(rank), ());
+    TEST(userStats.GetChangesCount(changesCount), ());
+
+    TEST_GREATER_OR_EQUAL(rank, 5800, ());
+    TEST_EQUAL(changesCount, 2, ());
+  }
+
+  // This test checks if user stats info was stored in setting.
+  // NOTE: there Update function is not called.
+  {
+    UserStatsLoader statsLoader;
+
+    TEST_EQUAL(statsLoader.GetUserName(), kUserName, ());
+    auto const userStats = statsLoader.GetStats(kUserName);
+
+    TEST(userStats, ());
+    int32_t rank, changesCount;
+    TEST(userStats.GetRank(rank), ());
+    TEST(userStats.GetChangesCount(changesCount), ());
+
+    TEST_GREATER_OR_EQUAL(rank, 5800, ());
+    TEST_EQUAL(changesCount, 2, ());
+  }
 }
 }  // namespace
 }  // namespace editor

editor/user_stats.cpp

@@ -1,47 +1,94 @@
 #include "editor/user_stats.hpp"
 
+#include "platform/platform.hpp"
+#include "platform/settings.hpp"
+
 #include "coding/url_encode.hpp"
 
 #include "base/logging.hpp"
+#include "base/timer.hpp"
+
+#include "std/thread.hpp"
 
 #include "3party/Alohalytics/src/http_client.h"
 #include "3party/pugixml/src/pugixml.hpp"
 
 using TRequest = alohalytics::HTTPClientPlatformWrapper;
 
-
 namespace
 {
 string const kUserStatsUrl = "http://py.osmz.ru/mmwatch/user?format=xml";
-auto constexpr kUninitialized = -1;
+int32_t constexpr kUninitialized = -1;
+
+auto constexpr kSettingsUserName = "LastLoggedUser";
+auto constexpr kSettingsRating = "UserEditorRating";
+auto constexpr kSettingsChangesCount = "UserEditorChangesCount";
+auto constexpr kSettingsLastUpdate = "UserEditorLastUpdate";
+
+auto constexpr kSecondsInHour = 60 * 60;
 }  // namespace
 
 namespace editor
 {
-UserStats::UserStats(string const & userName)
-  : m_userName(userName), m_changesCount(kUninitialized), m_rank(kUninitialized)
-{
-  m_updateStatus = Update();
-}
+// UserStat ----------------------------------------------------------------------------------------
 
-UserStats::UserStats(string const & userName, uint32_t rating, uint32_t changesCount)
-  : m_userName(userName), m_changesCount(changesCount), m_rank(rating), m_updateStatus(true)
+UserStats::UserStats()
+  : m_changesCount(kUninitialized), m_rank(kUninitialized)
+  , m_updateTime(my::SecondsSinceEpochToTimeT(0)), m_valid(false)
 {
 }
 
-bool UserStats::IsChangesCountInitialized() const
+UserStats::UserStats(time_t const updateTime, uint32_t const rating,
+                     uint32_t const changesCount, string const & levelUpFeat)
+  : m_changesCount(changesCount), m_rank(rating)
+  , m_updateTime(updateTime), m_levelUpRequiredFeat(levelUpFeat)
+  , m_valid(true)
 {
-  return m_changesCount != kUninitialized;
 }
 
-bool UserStats::IsRankInitialized() const
+bool UserStats::GetChangesCount(int32_t & changesCount) const
 {
-  return m_rank != kUninitialized;
+  if (m_changesCount == kUninitialized)
+    return false;
+  changesCount = m_changesCount;
+  return true;
 }
 
-bool UserStats::Update()
+bool UserStats::GetRank(int32_t & rank) const
 {
-  auto const url = kUserStatsUrl + "&name=" + UrlEncode(m_userName);
+  if (m_rank == kUninitialized)
+    return false;
+  rank = m_rank;
+  return true;
+}
+
+bool UserStats::GetLevelUpRequiredFeat(string & levelUpFeat)
+{
+  if (m_levelUpRequiredFeat.empty())
+    return false;
+  levelUpFeat = m_levelUpRequiredFeat;
+  return true;
+}
+
+// UserStatsLoader ---------------------------------------------------------------------------------
+
+UserStatsLoader::UserStatsLoader()
+  : m_lastUpdate(my::SecondsSinceEpochToTimeT(0))
+{
+  if (!LoadFromSettings())
+    LOG(LINFO, ("There is no cached user stats info in settings"));
+  else
+    LOG(LINFO, ("User stats info was loaded successfully"));
+}
+
+bool UserStatsLoader::Update(string const & userName)
+{
+  {
+    lock_guard<mutex> g(m_mutex);
+    m_userName = userName;
+  }
+
+  auto const url = kUserStatsUrl + "&name=" + UrlEncode(userName);
   TRequest request(url);
 
   if (!request.RunHTTPRequest())
@@ -65,9 +112,103 @@ bool UserStats::Update()
     return false;
   }
 
-  m_changesCount = document.select_node("mmwatch/edits/@value").attribute().as_int(kUninitialized);
-  m_rank = document.select_node("mmwatch/rank/@value").attribute().as_int(kUninitialized);
+  auto changesCount = document.select_node("mmwatch/edits/@value").attribute().as_int(-1);
+  auto rank = document.select_node("mmwatch/rank/@value").attribute().as_int(-1);
+  auto levelUpFeat = document.select_node("mmwatch/levelUpFeat/@value").attribute().as_string();
+
+  lock_guard<mutex> g(m_mutex);
+  if (m_userName != userName)
+    return false;
+
+  m_lastUpdate = time(nullptr);
+  m_userStats = UserStats(m_lastUpdate, rank, changesCount, levelUpFeat);
+  SaveToSettings();
 
   return true;
 }
+
+void UserStatsLoader::Update(string const & userName, TOnUpdateCallback fn)
+{
+  auto nothingToUpdate = false;
+  {
+    lock_guard<mutex> g(m_mutex);
+    nothingToUpdate = m_userStats && m_userName == userName && m_userStats &&
+                      difftime(m_lastUpdate, time(nullptr)) < kSecondsInHour;
+  }
+
+  if (nothingToUpdate)
+  {
+    GetPlatform().RunOnGuiThread(fn);
+    return;
+  }
+
+  thread([this, userName, fn] {
+    if (Update(userName))
+      GetPlatform().RunOnGuiThread(fn);
+  }).detach();
+}
+
+void UserStatsLoader::DropStats(string const & userName)
+{
+  lock_guard<mutex> g(m_mutex);
+  if (m_userName != userName)
+    return;
+  m_userStats = {};
+  DropSettings();
+}
+
+UserStats UserStatsLoader::GetStats(string const & userName) const
+{
+  lock_guard<mutex> g(m_mutex);
+  if (m_userName == userName)
+    return m_userStats;
+  return {};
+}
+
+string UserStatsLoader::GetUserName() const
+{
+  lock_guard<mutex> g(m_mutex);
+  return m_userName;
+}
+
+bool UserStatsLoader::LoadFromSettings()
+{
+  uint32_t rating, changesCount;
+  uint64_t lastUpdate;
+  if (!settings::Get(kSettingsUserName, m_userName) ||
+      !settings::Get(kSettingsChangesCount, changesCount) ||
+      !settings::Get(kSettingsRating, rating) ||
+      !settings::Get(kSettingsLastUpdate, lastUpdate))
+  {
+    return false;
+  }
+
+  m_lastUpdate = my::SecondsSinceEpochToTimeT(lastUpdate);
+  m_userStats = UserStats(m_lastUpdate, rating, changesCount, "");
+  return true;
+}
+
+void UserStatsLoader::SaveToSettings()
+{
+  if (!m_userStats)
+    return;
+
+  settings::Set(kSettingsUserName, m_userName);
+  int32_t rank;
+  if (m_userStats.GetRank(rank))
+    settings::Set(kSettingsRating, rank);
+  int32_t changesCount;
+  if (m_userStats.GetChangesCount(changesCount))
+    settings::Set(kSettingsChangesCount, changesCount);
+  settings::Set(kSettingsLastUpdate, my::TimeTToSecondsSinceEpoch(m_lastUpdate));
+  // Do not save m_requiredLevelUpFeat for it becomes obsolete very fast.
+}
+
+void UserStatsLoader::DropSettings()
+{
+  settings::Delete(kSettingsUserName);
+  settings::Delete(kSettingsRating);
+  settings::Delete(kSettingsChangesCount);
+  settings::Delete(kSettingsLastUpdate);
+}
 }  // namespace editor

editor/user_stats.hpp

@@ -1,6 +1,9 @@
 #pragma once
 
 #include "std/cstdint.hpp"
+#include "std/ctime.hpp"
+#include "std/function.hpp"
+#include "std/mutex.hpp"
 #include "std/string.hpp"
 
 namespace editor
@@ -8,24 +11,64 @@ namespace editor
 class UserStats
 {
 public:
-  explicit UserStats(string const & userName);
-  explicit UserStats(string const & userName, uint32_t rating, uint32_t changesCount);
+  UserStats();
+  UserStats(time_t const updateTime, uint32_t const rating,
+            uint32_t const changesCount, string const & levelUpFeat);
 
-  bool IsChangesCountInitialized() const;
-  bool IsRankInitialized() const;
+  bool IsValid() const { return m_valid; }
 
-  int32_t GetChangesCount() const { return m_changesCount; }
-  int32_t GetRank() const { return m_rank; }
+  operator bool() const { return IsValid(); }
 
-  bool GetUpdateStatus() const { return m_updateStatus; }
-  bool Update();
+  bool GetChangesCount(int32_t & changesCount) const;
+  bool GetRank(int32_t & rank) const;
+  bool GetLevelUpRequiredFeat(string & levelUpFeat);
+
+  time_t GetLastUpdate() const { return m_updateTime; }
 
 private:
-  string m_userName;
   int32_t m_changesCount;
   int32_t m_rank;
+  time_t m_updateTime;
+  /// A very doubtful field representing what a user must commit to have a better rank.
+  string m_levelUpRequiredFeat;
+  bool m_valid;
+};
 
-  /// True if last update was successful.
-  bool m_updateStatus;
+class UserStatsLoader
+{
+public:
+  using TOnUpdateCallback = function<void()>;
+
+  UserStatsLoader();
+
+  /// Synchronously sends request to the server. Updates stats and returns true on success.
+  bool Update(string const & userName);
+
+  /// Launch the update process if stats are too old.
+  /// The process posts fn to a gui thread on success.
+  void Update(string const & userName, TOnUpdateCallback fn);
+
+  /// Resets internal state and removes records from settings.
+  void DropStats(string const & userName);
+
+  /// Atomically returns stats if userName is still actual.
+  UserStats GetStats(string const & userName) const;
+
+  /// Debug only.
+  string GetUserName() const;
+
+private:
+  /// Not thread-safe, but called only in constructor.
+  bool LoadFromSettings();
+  /// Not thread-safe, use synchonization.
+  void SaveToSettings();
+  void DropSettings();
+
+  string m_userName;
+
+  time_t m_lastUpdate;
+  mutable mutex m_mutex;
+
+  UserStats m_userStats;
 };
 }  // namespace editor

map/framework.cpp

@@ -412,11 +412,6 @@ Framework::Framework()
   m_model.GetIndex().AddObserver(editor);
 
   LOG(LINFO, ("Editor initialized"));
-
-  if(!LoadUserStatsFromSettings())
-    LOG(LINFO, ("There is no cached stats in settings"));
-  else
-    LOG(LINFO, ("User stats was loaded successfully"));
 }
 
 Framework::~Framework()
@@ -2901,82 +2896,3 @@ bool Framework::RollBackChanges(FeatureID const & fid)
   }
   return rolledBack;
 }
-
-namespace
-{
-char const kSettingsUserName[] = "LoginedUser";
-char const kSettingsRating[] = "UserEditorRating";
-char const kSettingsChangesCount[] = "UserEditorChangsCount";
-}  // namespace
-
-shared_ptr<editor::UserStats const> Framework::GetUserStats(string const & userName) const
-{
-  lock_guard<mutex> g(m_userStatsMutex);
-  if (m_userName == userName)
-    return m_userStats;
-  return nullptr;
-}
-
-void Framework::UpdateUserStats(string const & userName, TOnStatsUpdated const & fn)
-{
-  {
-    lock_guard<mutex> g(m_userStatsMutex);
-
-    auto nothingToUpdate = m_userStats && m_userName == userName;
-    nothingToUpdate = nothingToUpdate && difftime(time(nullptr), m_latsUpdate) <= 60 * 60 * 60;
-
-    if (nothingToUpdate)
-      return GetPlatform().RunOnGuiThread(fn);
-
-    m_userName = userName;
-  }
-
-  thread([this, &fn, userName]()
-  {
-    if (UpdateUserStats(userName))
-    {
-      lock_guard<mutex> g(m_userStatsMutex);
-      if (m_userName == userName)
-        GetPlatform().RunOnGuiThread(fn);
-    }
-  }).detach();
-}
-
-bool Framework::UpdateUserStats(string const & userName)
-{
-  auto userStats = make_shared<editor::UserStats>(userName);
-  if (!userStats->GetUpdateStatus())
-    return false;
-
-  lock_guard<mutex> g(m_userStatsMutex);
-  if (m_userName != userName)
-    return false;
-  m_latsUpdate = time(nullptr);
-  m_userStats = move(userStats);
-  SaveUserStatsToSettings();
-  return true;
-}
-
-bool Framework::LoadUserStatsFromSettings()
-{
-  uint32_t rating, changesCount;
-  if (!settings::Get(kSettingsUserName, m_userName) ||
-      !settings::Get(kSettingsChangesCount, changesCount) ||
-      !settings::Get(kSettingsRating, rating))
-  {
-    return false;
-  }
-
-  m_userStats = make_shared<editor::UserStats>(m_userName, rating, changesCount);
-  return true;
-}
-
-void Framework::SaveUserStatsToSettings()
-{
-  if (!m_userStats)
-    return;
-
-  settings::Set(kSettingsUserName, m_userName);
-  settings::Set(kSettingsRating, m_userStats->GetRank());
-  settings::Set(kSettingsChangesCount, m_userStats->GetChangesCount());
-}

map/framework.hpp

@@ -43,8 +43,6 @@
 #include "base/thread_checker.hpp"
 
 #include "std/list.hpp"
-#include "std/mutex.hpp"  // TODO(mgsergio): remove when m_UserStatsMutex is gone.
-#include "std/shared_ptr.hpp"
 #include "std/target_os.hpp"
 #include "std/unique_ptr.hpp"
 #include "std/vector.hpp"
@@ -676,30 +674,24 @@ private:
 
 public:
   //@{
-  // TODO(mgsergio): move mutexed logic out from framework.
   // User statistics.
 
-  // A callback type to be passed in UpdateUserStats.
-  using TOnStatsUpdated = function<void()>;
+  editor::UserStats GetUserStats(string const & userName) const
+  {
+    return m_userStatsLoader.GetStats(userName);
+  }
 
-  // TODO(mgsergio): Comment to this function.
-  shared_ptr<editor::UserStats const> GetUserStats(string const & userName) const;
+  // Reads user stats from server or gets it from cache calls |fn| on success.
+  void UpdateUserStats(string const & userName,
+                       editor::UserStatsLoader::TOnUpdateCallback fn)
+  {
+    m_userStatsLoader.Update(userName, fn);
+  }
 
-  // Reads user stats from server or gets it from cache calls reader on success.
-  void UpdateUserStats(string const & userName, TOnStatsUpdated const & fn);
+  void DropUserStats(string const & userName) { m_userStatsLoader.DropStats(userName); }
 
 private:
-  /// Sends a synchronous request to the server and updates user's stats.
-  bool UpdateUserStats(string const & userName);
-
-  bool LoadUserStatsFromSettings();
-  /// Not thread-safe, use synchonization.
-  void SaveUserStatsToSettings();
-
-  shared_ptr<editor::UserStats> m_userStats;
-  string m_userName;
-  time_t m_latsUpdate{};
-  mutable mutex m_userStatsMutex;
+  editor::UserStatsLoader m_userStatsLoader;
   //@}
 
   DECLARE_THREAD_CHECKER(m_threadChecker);

omim.pro

@@ -196,7 +196,7 @@ SUBDIRS = 3party base coding geometry editor indexer routing search
     SUBDIRS *= generator_tests
 
     editor_tests.subdir = editor/editor_tests
-    editor_tests.depends = 3party base coding geometry editor
+    editor_tests.depends = 3party base coding geometry platform editor
     SUBDIRS *= editor_tests
 
     SUBDIRS *= qt_tstfrm

platform/platform.cpp

@@ -87,6 +87,11 @@ bool Platform::RmDirRecursively(string const & dirName)
   return res;
 }
 
+void Platform::SetSettingsDirForTests(string const & path)
+{
+  m_settingsDir = my::AddSlashIfNeeded(path);
+}
+
 string Platform::ReadPathForFile(string const & file, string searchScope) const
 {
   if (searchScope.empty())

platform/platform.hpp

@@ -126,6 +126,8 @@ public:
   /// @return path for directory in the persistent memory, can be the same
   /// as WritableDir, but on some platforms it's different
   string SettingsDir() const { return m_settingsDir; }
+  /// Set settings dir — use for testing.
+  void SetSettingsDirForTests(string const & path);
   /// @return full path to file in the settings directory
   string SettingsPathForFile(string const & file) const { return SettingsDir() + file; }
 

platform/platform_tests_support/writable_dir_changer.cpp

@@ -8,15 +8,18 @@
 #include "coding/file_name_utils.hpp"
 #include "coding/internal/file_data.hpp"
 
-WritableDirChanger::WritableDirChanger(string const & testDir)
+WritableDirChanger::WritableDirChanger(string const & testDir, SettingsDirPolicy settingsDirPolicy)
   : m_writableDirBeforeTest(GetPlatform().WritableDir())
   , m_testDirFullPath(m_writableDirBeforeTest + testDir)
+  , m_settingsDirPolicy(settingsDirPolicy)
 {
   Platform & platform = GetPlatform();
   platform.RmDirRecursively(m_testDirFullPath);
   TEST(!platform.IsFileExistsByFullPath(m_testDirFullPath), ());
   TEST_EQUAL(Platform::ERR_OK, platform.MkDir(m_testDirFullPath), ());
   platform.SetWritableDirForTests(m_testDirFullPath);
+  if (m_settingsDirPolicy == SettingsDirPolicy::UseWritableDir)
+    platform.SetSettingsDirForTests(m_testDirFullPath);
   settings::Clear();
 }
 
@@ -26,5 +29,7 @@ WritableDirChanger::~WritableDirChanger()
   Platform & platform = GetPlatform();
   string const writableDirForTest = platform.WritableDir();
   platform.SetWritableDirForTests(m_writableDirBeforeTest);
+  if (m_settingsDirPolicy == SettingsDirPolicy::UseWritableDir)
+    platform.SetSettingsDirForTests(m_writableDirBeforeTest);
   platform.RmDirRecursively(writableDirForTest);
 }

platform/platform_tests_support/writable_dir_changer.hpp

@@ -3,10 +3,17 @@
 class WritableDirChanger
 {
 public:
-  WritableDirChanger(string const & testDir);
+  enum class SettingsDirPolicy
+  {
+    UseDefault, UseWritableDir
+  };
+
+  WritableDirChanger(string const & testDir,
+                     SettingsDirPolicy settingsDirPolicy = SettingsDirPolicy::UseDefault);
   ~WritableDirChanger();
 
 private:
   string const m_writableDirBeforeTest;
   string const m_testDirFullPath;
+  SettingsDirPolicy m_settingsDirPolicy;
 };