Repository: RetroAchievements/RAIntegration
Pull request #729 (2021-05-15 20:04:59)

ensure clone.category is local at the server checkpoint level
fixes an issue where resetting a cloned achievement changes its category to Core


Patch:
diff --git a/src/Exports.cpp b/src/Exports.cpp
index 38086531..59e9f2a0 100644
--- a/src/Exports.cpp
+++ b/src/Exports.cpp
@@ -403,8 +403,12 @@ static void ProcessAchievements()
                 const auto* vmAchievement = pGameContext.Assets().FindAchievement(pChange.nId);
                 if (vmAchievement)
                 {
-                    auto& pOverlayManager = ra::services::ServiceLocator::GetMutable<ra::ui::viewmodels::OverlayManager>();
-                    pOverlayManager.AddChallengeIndicator(vmAchievement->GetID(), ra::ui::ImageType::Badge, ra::Narrow(vmAchievement->GetBadge()));
+                    const auto& pConfiguration = ra::services::ServiceLocator::Get<ra::services::IConfiguration>();
+                    if (pConfiguration.GetPopupLocation(ra::ui::viewmodels::Popup::Challenge) != ra::ui::viewmodels::PopupLocation::None)
+                    {
+                        auto& pOverlayManager = ra::services::ServiceLocator::GetMutable<ra::ui::viewmodels::OverlayManager>();
+                        pOverlayManager.AddChallengeIndicator(vmAchievement->GetID(), ra::ui::ImageType::Badge, ra::Narrow(vmAchievement->GetBadge()));
+                    }
                 }
 
                 break;
@@ -434,10 +438,13 @@ static void ProcessAchievements()
                             L"Leaderboard attempt started", ra::Widen(pLeaderboard->Title()), ra::Widen(pLeaderboard->Description()));
                     }
 
-                    auto& pOverlayManager = ra::services::ServiceLocator::GetMutable<ra::ui::viewmodels::OverlayManager>();
-                    auto& pScoreTracker = pOverlayManager.AddScoreTracker(pLeaderboard->ID());
-                    const auto sDisplayText = pLeaderboard->FormatScore(pChange.nValue);
-                    pScoreTracker.SetDisplayText(ra::Widen(sDisplayText));
+                    if (pConfiguration.GetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardTracker) != ra::ui::viewmodels::PopupLocation::None)
+                    {
+                        auto& pOverlayManager = ra::services::ServiceLocator::GetMutable<ra::ui::viewmodels::OverlayManager>();
+                        auto& pScoreTracker = pOverlayManager.AddScoreTracker(pLeaderboard->ID());
+                        const auto sDisplayText = pLeaderboard->FormatScore(pChange.nValue);
+                        pScoreTracker.SetDisplayText(ra::Widen(sDisplayText));
+                    }
                 }
 
                 break;
diff --git a/src/ui/viewmodels/AssetListViewModel.cpp b/src/ui/viewmodels/AssetListViewModel.cpp
index 852f1c3b..56cbe015 100644
--- a/src/ui/viewmodels/AssetListViewModel.cpp
+++ b/src/ui/viewmodels/AssetListViewModel.cpp
@@ -1396,17 +1396,22 @@ void AssetListViewModel::CloneSelected()
     std::vector<int> vNewIDs;
     for (const auto* pAsset : vSelectedAssets)
     {
-        const auto& pSourceAchievement = dynamic_cast<const ra::data::models::AchievementModel*>(pAsset);
-        auto& vmAchievement = pGameContext.Assets().NewAchievement();
-        vmAchievement.SetCategory(ra::data::models::AssetCategory::Local);
-        vmAchievement.SetName(pSourceAchievement->GetName() + L" (copy)");
-        vmAchievement.SetDescription(pSourceAchievement->GetDescription());
-        vmAchievement.SetBadge(pSourceAchievement->GetBadge());
-        vmAchievement.SetPoints(pSourceAchievement->GetPoints());
-        vmAchievement.SetTrigger(pSourceAchievement->GetTrigger());
-        vmAchievement.SetNew();
-
-        vNewIDs.push_back(vmAchievement.GetID());
+        const auto* pSourceAchievement = dynamic_cast<const ra::data::models::AchievementModel*>(pAsset);
+        if (pSourceAchievement != nullptr)
+        {
+            auto& vmAchievement = pGameContext.Assets().NewAchievement();
+            vmAchievement.SetCategory(ra::data::models::AssetCategory::Local);
+            vmAchievement.UpdateServerCheckpoint();
+
+            vmAchievement.SetName(pSourceAchievement->GetName() + L" (copy)");
+            vmAchievement.SetDescription(pSourceAchievement->GetDescription());
+            vmAchievement.SetBadge(pSourceAchievement->GetBadge());
+            vmAchievement.SetPoints(pSourceAchievement->GetPoints());
+            vmAchievement.SetTrigger(pSourceAchievement->GetTrigger());
+            vmAchievement.SetNew();
+
+            vNewIDs.push_back(vmAchievement.GetID());
+        }
     }
 
     // select the new items and deselect everything else
diff --git a/src/ui/viewmodels/AssetUploadViewModel.cpp b/src/ui/viewmodels/AssetUploadViewModel.cpp
index 0e5df104..47e11d10 100644
--- a/src/ui/viewmodels/AssetUploadViewModel.cpp
+++ b/src/ui/viewmodels/AssetUploadViewModel.cpp
@@ -79,34 +79,16 @@ void AssetUploadViewModel::UploadBadge(const std::wstring& sBadge)
 
     ra::api::UploadBadge::Request request;
     request.ImageFilePath = sFilename;
-    const auto response = request.Call();
 
-    // if the upload failed, set the error message on all the associated achievements (including this one)
-    if (!response.Succeeded())
-    {
-        std::lock_guard<std::mutex> pLock(m_pMutex);
-        for (auto& pItem : m_vUploadQueue)
-        {
-            if (pItem.nState == UploadState::WaitingForImage)
-            {
-                const auto* pQueuedAchievement = dynamic_cast<ra::data::models::AchievementModel*>(pItem.pAsset);
-                if (pQueuedAchievement && pQueuedAchievement->GetBadge() == sBadge)
-                {
-                    pItem.sErrorMessage = response.ErrorMessage;
-                    pItem.nState = UploadState::Failed;
-                }
-            }
-        }
-
-        return;
-    }
-
-    // upload succeeded. update the badge property for each associated achievement and identify any achievements that
-    // need to be reprocessed because the image wasn't ready when they were previously given a chance to process.
     std::vector<ra::data::models::AchievementModel*> vAffectedAchievements;
-
     {
+        // only allow one badge upload at a time to prevent a race condition on server that could
+        // result in non-unique image IDs being returned.
         std::lock_guard<std::mutex> pLock(m_pMutex);
+        const auto response = request.Call();
+
+        // if the upload succeeded, update the badge property for each associated achievement and queue the
+        // achievement update. if the upload failed, set the error message and don't queue the achievement update.
         for (auto& pItem : m_vUploadQueue)
         {
             if (pItem.nState == UploadState::WaitingForImage)
@@ -114,14 +96,23 @@ void AssetUploadViewModel::UploadBadge(const std::wstring& sBadge)
                 auto* pQueuedAchievement = dynamic_cast<ra::data::models::AchievementModel*>(pItem.pAsset);
                 if (pQueuedAchievement && pQueuedAchievement->GetBadge() == sBadge)
                 {
-                    pQueuedAchievement->SetBadge(ra::Widen(response.BadgeId));
-                    vAffectedAchievements.push_back(pQueuedAchievement);
+                    if (response.Succeeded())
+                    {
+                        RA_LOG_INFO("Changed badge on achievement %u to %s", pQueuedAchievement->GetID(), response.BadgeId);
+                        pQueuedAchievement->SetBadge(ra::Widen(response.BadgeId));
+                        vAffectedAchievements.push_back(pQueuedAchievement);
+                    }
+                    else
+                    {
+                        pItem.sErrorMessage = response.ErrorMessage;
+                        pItem.nState = UploadState::Failed;
+                    }
                 }
             }
         }
     }
 
-    // the affected achievements can be uploaded now
+    // the affected achievements can be uploaded now - will be empty on failure
     for (auto* pAchievement : vAffectedAchievements)
     {
         QueueTask([this, pAchievement]()
@@ -151,18 +142,9 @@ void AssetUploadViewModel::UploadAchievement(ra::data::models::AchievementModel&
         request.AchievementId = pAchievement.GetID();
     }
 
-    UploadItem* pItem = nullptr;
-    for (auto& pScan : m_vUploadQueue)
-    {
-        if (pScan.pAsset == &pAchievement)
-        {
-            pItem = &pScan;
-            break;
-        }
-    }
-    Expects(pItem != nullptr);
-
     const auto& response = request.Call();
+
+    // update the achievement
     if (response.Succeeded())
     {
         if (pAchievement.GetCategory() == ra::data::models::AssetCategory::Local)
@@ -173,13 +155,18 @@ void AssetUploadViewModel::UploadAchievement(ra::data::models::AchievementModel&
 
         pAchievement.UpdateLocalCheckpoint();
         pAchievement.UpdateServerCheckpoint();
-
-        pItem->nState = UploadState::Success;
     }
-    else
+
+    // update the queue
+    std::lock_guard<std::mutex> pLock(m_pMutex);
+    for (auto& pScan : m_vUploadQueue)
     {
-        pItem->sErrorMessage = response.ErrorMessage;
-        pItem->nState = UploadState::Failed;
+        if (pScan.pAsset == &pAchievement)
+        {
+            pScan.sErrorMessage = response.ErrorMessage;
+            pScan.nState = response.Succeeded() ? UploadState::Success : UploadState::Failed;
+            break;
+        }
     }
 }
 


Test patch:
diff --git a/tests/Exports_Tests.cpp b/tests/Exports_Tests.cpp
index a99e3887..828fc246 100644
--- a/tests/Exports_Tests.cpp
+++ b/tests/Exports_Tests.cpp
@@ -478,6 +478,7 @@ public:
     {
         DoAchievementsFrameHarness harness;
         harness.MockAchievement(1U).SetBadge(L"55223");
+        harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::Challenge, ra::ui::viewmodels::PopupLocation::BottomRight);
         harness.mockRuntime.QueueChange(ra::services::AchievementRuntime::ChangeType::AchievementPrimed, 1U);
 
         _RA_DoAchievementsFrame();
@@ -490,10 +491,24 @@ public:
         Assert::AreEqual(std::string("55223"), pIndicator->GetImage().Name());
     }
 
+    TEST_METHOD(TestDoAchievementsFrameAchievementPrimedNoTracker)
+    {
+        DoAchievementsFrameHarness harness;
+        harness.MockAchievement(1U).SetBadge(L"55223");
+        harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::Challenge, ra::ui::viewmodels::PopupLocation::None);
+        harness.mockRuntime.QueueChange(ra::services::AchievementRuntime::ChangeType::AchievementPrimed, 1U);
+
+        _RA_DoAchievementsFrame();
+
+        const auto* pIndicator = harness.mockOverlayManager.GetChallengeIndicator(1U);
+        Assert::IsNull(pIndicator);
+    }
+
     TEST_METHOD(TestDoAchievementsFrameAchievementUnprimed)
     {
         DoAchievementsFrameHarness harness;
         harness.MockAchievement(1U).SetBadge(L"55223");
+        harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::Challenge, ra::ui::viewmodels::PopupLocation::BottomRight);
         harness.mockOverlayManager.AddChallengeIndicator(1U, ra::ui::ImageType::Badge, "55223");
 
         harness.mockRuntime.QueueChange(ra::services::AchievementRuntime::ChangeType::AchievementActivated, 1U);
@@ -511,6 +526,7 @@ public:
     {
         DoAchievementsFrameHarness harness;
         harness.MockAchievement(1U).SetBadge(L"55223");
+        harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::Challenge, ra::ui::viewmodels::PopupLocation::BottomRight);
         harness.mockRuntime.QueueChange(ra::services::AchievementRuntime::ChangeType::AchievementPrimed, 1U);
 
         _RA_DoAchievementsFrame();
@@ -602,6 +618,7 @@ public:
     {
         DoAchievementsFrameHarness harness;
         harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardStarted, ra::ui::viewmodels::PopupLocation::BottomLeft);
+        harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardTracker, ra::ui::viewmodels::PopupLocation::BottomRight);
         auto& pLeaderboard = harness.mockGameContext.NewLeaderboard(1U);
         pLeaderboard.SetTitle("Title");
         pLeaderboard.SetDescription("Description");
@@ -623,10 +640,30 @@ public:
         Assert::AreEqual(std::wstring(L"1234"), pScore->GetDisplayText());
     }
 
+    TEST_METHOD(TestDoAchievementsFrameLeaderboardStartNoPopups)
+    {
+        DoAchievementsFrameHarness harness;
+        harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardStarted, ra::ui::viewmodels::PopupLocation::None);
+        harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardTracker, ra::ui::viewmodels::PopupLocation::None);
+        auto& pLeaderboard = harness.mockGameContext.NewLeaderboard(1U);
+        pLeaderboard.SetTitle("Title");
+        pLeaderboard.SetDescription("Description");
+
+        harness.mockRuntime.QueueChange(ra::services::AchievementRuntime::ChangeType::LeaderboardStarted, 1U, 1234U);
+        _RA_DoAchievementsFrame();
+
+        const auto* pPopup = harness.mockOverlayManager.GetMessage(1);
+        Assert::IsNull(pPopup);
+
+        const auto* pScore = harness.mockOverlayManager.GetScoreTracker(1U);
+        Assert::IsNull(pScore);
+    }
+
     TEST_METHOD(TestDoAchievementsFrameLeaderboardStartPopupDisabled)
     {
         DoAchievementsFrameHarness harness;
         harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardStarted, ra::ui::viewmodels::PopupLocation::None);
+        harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardTracker, ra::ui::viewmodels::PopupLocation::BottomRight);
         harness.mockGameContext.NewLeaderboard(1U);
 
         harness.mockRuntime.QueueChange(ra::services::AchievementRuntime::ChangeType::LeaderboardStarted, 1U, 1234U);
@@ -645,6 +682,7 @@ public:
     {
         DoAchievementsFrameHarness harness;
         harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardCanceled, ra::ui::viewmodels::PopupLocation::BottomLeft);
+        harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardTracker, ra::ui::viewmodels::PopupLocation::BottomRight);
         auto& pLeaderboard = harness.mockGameContext.NewLeaderboard(1U);
         pLeaderboard.SetTitle("Title");
         pLeaderboard.SetDescription("Description");
@@ -667,6 +705,7 @@ public:
     {
         DoAchievementsFrameHarness harness;
         harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardCanceled, ra::ui::viewmodels::PopupLocation::None);
+        harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardTracker, ra::ui::viewmodels::PopupLocation::BottomRight);
         auto& pLeaderboard = harness.mockGameContext.NewLeaderboard(1U);
         pLeaderboard.SetTitle("Title");
         pLeaderboard.SetDescription("Description");
@@ -685,6 +724,7 @@ public:
     {
         DoAchievementsFrameHarness harness;
         harness.mockGameContext.NewLeaderboard(1U);
+        harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardTracker, ra::ui::viewmodels::PopupLocation::BottomRight);
         harness.mockOverlayManager.AddScoreTracker(1U);
 
         harness.mockRuntime.QueueChange(ra::services::AchievementRuntime::ChangeType::LeaderboardUpdated, 1U, 1235U);
@@ -700,6 +740,7 @@ public:
     {
         DoAchievementsFrameHarness harness;
         harness.mockConfiguration.SetFeatureEnabled(ra::services::Feature::Hardcore, true);
+        harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardTracker, ra::ui::viewmodels::PopupLocation::BottomRight);
         harness.mockGameContext.NewLeaderboard(1U);
 
         harness.mockRuntime.QueueChange(ra::services::AchievementRuntime::ChangeType::LeaderboardTriggered, 1U, 1236U);
@@ -718,6 +759,7 @@ public:
         harness.MockAchievement(2U);
         harness.mockGameContext.NewLeaderboard(3U);
         harness.mockGameContext.NewLeaderboard(4U);
+        harness.mockConfiguration.SetPopupLocation(ra::ui::viewmodels::Popup::LeaderboardTracker, ra::ui::viewmodels::PopupLocation::BottomRight);
 
         harness.mockRuntime.QueueChange(ra::services::AchievementRuntime::ChangeType::LeaderboardStarted, 3U, 1234U);
         harness.mockRuntime.QueueChange(ra::services::AchievementRuntime::ChangeType::AchievementTriggered, 1U);
diff --git a/tests/ui/viewmodels/AssetListViewModel_Tests.cpp b/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
index 0b889337..db85e728 100644
--- a/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
+++ b/tests/ui/viewmodels/AssetListViewModel_Tests.cpp
@@ -2652,6 +2652,63 @@ public:
         Assert::IsTrue(bMessageSeen);
     }
 
+    TEST_METHOD(TestCloneReset)
+    {
+        AssetListViewModelHarness vmAssetList;
+        vmAssetList.SetGameId(22U);
+        vmAssetList.AddAchievement(AssetCategory::Core, 5, L"Test1", L"Desc1", L"12345", "0xH1234=1");
+        vmAssetList.AddAchievement(AssetCategory::Core, 7, L"Test2", L"Desc2", L"11111", "0xH1111=1");
+
+        Assert::AreEqual({ 2U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 2U }, vmAssetList.FilteredAssets().Count());
+        Assert::AreEqual(AssetListViewModel::FilterCategory::Core, vmAssetList.GetFilterCategory());
+
+        vmAssetList.FilteredAssets().GetItemAt(1)->SetSelected(true);
+        vmAssetList.ForceUpdateButtons();
+
+        bool bEditorShown = false;
+        vmAssetList.mockDesktop.ExpectWindow<AssetEditorViewModel>([&bEditorShown](AssetEditorViewModel&)
+        {
+            bEditorShown = true;
+            return DialogResult::None;
+        });
+
+        vmAssetList.CloneSelected();
+
+        // new Local achievement should be created and focused
+        Assert::AreEqual({ 3U }, vmAssetList.mockGameContext.Assets().Count());
+        Assert::AreEqual({ 1U }, vmAssetList.FilteredAssets().Count());
+        Assert::AreEqual(AssetListViewModel::FilterCategory::Local, vmAssetList.GetFilterCategory());
+
+        auto* pAsset = vmAssetList.FilteredAssets().GetItemAt(0);
+        Expects(pAsset != nullptr);
+        Assert::IsTrue(pAsset->IsSelected());
+        Assert::AreEqual(std::wstring(L"Test2 (copy)"), pAsset->GetLabel());
+        Assert::AreEqual(AssetCategory::Local, pAsset->GetCategory());
+        Assert::AreEqual(AssetState::Inactive, pAsset->GetState());
+        Assert::AreEqual(AssetChanges::New, pAsset->GetChanges());
+        Assert::AreEqual({ 111000001U }, pAsset->GetId());
+        Assert::AreEqual(7, pAsset->GetPoints());
+
+        // save the asset - if we reset without saving, it'll just get deleted
+        vmAssetList.ForceUpdateButtons();
+        vmAssetList.SaveSelected();
+        Assert::AreEqual(AssetChanges::Unpublished, pAsset->GetChanges());
+
+        // resetting the achievement should not change anything about it
+        vmAssetList.ForceUpdateButtons();
+        vmAssetList.ResetSelected();
+
+        pAsset = vmAssetList.FilteredAssets().GetItemAt(0);
+        Expects(pAsset != nullptr);
+        Assert::AreEqual(std::wstring(L"Test2 (copy)"), pAsset->GetLabel());
+        Assert::AreEqual(AssetCategory::Local, pAsset->GetCategory());
+        Assert::AreEqual(AssetState::Inactive, pAsset->GetState());
+        Assert::AreEqual(AssetChanges::Unpublished, pAsset->GetChanges());
+        Assert::AreEqual({ 111000001U }, pAsset->GetId());
+        Assert::AreEqual(7, pAsset->GetPoints());
+    }
+
     TEST_METHOD(TestResetSelectedAllUnmodified)
     {
         AssetListViewModelHarness vmAssetList;