FIXUP: Add cooldown period

dangerzone/gui/updater.py

@@ -2,6 +2,7 @@
 
 import logging
 import platform
+import time
 import typing
 from typing import Any, Dict, Optional
 
@@ -34,6 +35,8 @@
 If you prefer another way of getting notified about new releases, we suggest adding to your RSS reader our Mastodon feed: https://fosstodon.org/@dangerzone.rss. For more information about updates, check our wiki page: https://github.com/freedomofpress/dangerzone/wiki/Updates
 """
 
+UPDATE_CHECK_COOLDOWN_SECS = 60 * 60 * 12  # Check for updates at most every 12 hours.
+
 
 class UpdateReport:
     """A report for an update check."""
@@ -82,14 +85,6 @@ def __init__(self, dangerzone: DangerzoneGui):
     # These helpers make it easy to retrieve specific updater-related settings, as well
     # as save the settings file, only when necessary.
 
-    @property
-    def first_run(self) -> bool:
-        return self.dangerzone.settings.get("updater_first_run")
-
-    @first_run.setter
-    def first_run(self, val: bool) -> None:
-        self.dangerzone.settings.set("updater_first_run", val, autosave=True)
-
     @property
     def check(self) -> Optional[bool]:
         return self.dangerzone.settings.get("updater_check")
@@ -113,7 +108,17 @@ def prompt_for_checks(self) -> bool:
         return bool(check)
 
     def should_check_for_updates(self) -> bool:
-        """Determine if we can check for updates based on settings and user prefs."""
+        """Determine if we can check for updates based on settings and user prefs.
+
+        Note that this method only checks if the user has expressed an interest for
+        learning about new updates, and not whether we should actually make an update
+        check. Those two things are distinct, actually. For example:
+
+        * A user may have expressed that they want to learn about new updates.
+        * A previous update check may have found out that there's a new version out.
+        * Thus we will always show to the user the cached info about the new version,
+          and won't make a new update check.
+        """
         log.debug("Checking platform type")
         # TODO: Disable updates for Homebrew installations.
         # TODO: Add a flag that will force-check for updates, so that we can test it on
@@ -124,9 +129,9 @@ def should_check_for_updates(self) -> bool:
         #     return False
 
         log.debug("Checking if first run of Dangerzone")
-        if self.first_run:
+        if self.dangerzone.settings.get("updater_last_check") is None:
             log.debug("Dangerzone is running for the first time, updates are stalled")
-            self.first_run = False
+            self.dangerzone.settings.set("updater_last_check", 0, autosave=True)
             return False
 
         log.debug("Checking if user has already expressed their preference")
@@ -151,6 +156,22 @@ def can_update(self, cur_version: str, latest_version: str) -> bool:
         else:
             return True
 
+    def _get_now_timestamp(self) -> int:
+        return int(time.time())
+
+    def _should_postpone_update_check(self) -> bool:
+        """Consult and update cooldown timer.
+
+        If the previous check happened before the cooldown period expires, do not check
+        again.
+        """
+        current_time = self._get_now_timestamp()
+        last_check = self.dangerzone.settings.get("updater_last_check")
+        if current_time < last_check + UPDATE_CHECK_COOLDOWN_SECS:
+            return True
+        else:
+            return False
+
     def get_latest_info(self) -> UpdateReport:
         """Get the latest release info from GitHub.
 
@@ -188,7 +209,17 @@ def _check_for_updates(self) -> UpdateReport:
                 changelog=self.dangerzone.settings.get("updater_latest_changelog"),
             )
 
-        # TODO: We must have a back-off period (e.g., 12 hours) between each check.
+        # If the previous check happened before the cooldown period expires, do not
+        # check again. Else, bump the last check timestamp, before making the actual
+        # check. This is to ensure that even failed update checks respect the cooldown
+        # period.
+        if self._should_postpone_update_check():
+            return UpdateReport()
+        else:
+            self.dangerzone.settings.set(
+                "updater_last_check", self._get_now_timestamp(), autosave=True
+            )
+
         log.debug(f"Checking the latest GitHub release")
         report = self.get_latest_info()
         log.debug(f"Latest version in GitHub is {report.version}")

dangerzone/settings.py

@@ -34,7 +34,7 @@ def generate_default_settings(cls) -> Dict[str, Any]:
             "open_app": None,
             "safe_extension": SAFE_EXTENSION,
             "updater_check": None,
-            "updater_first_run": True,
+            "updater_last_check": None,  # last check in UNIX epoch (secs since 1970)
             # FIXME: How to invalidate those if they change upstream?
             "updater_latest_version": get_version(),
             "updater_latest_changelog": "",

tests/gui/test_main_window.py

@@ -19,13 +19,15 @@ def test_qt(
 ) -> None:
     updater = qt_updater
     updater.dangerzone.settings.set("updater_check", True)
-    updater.dangerzone.settings.set("updater_first_run", False)
+    updater.dangerzone.settings.set("updater_last_check", 0)
     expected_settings = default_updater_settings()
-    expected_settings["updater_first_run"] = False
     expected_settings["updater_check"] = True
 
     mock_upstream_info = {"tag_name": f"v{get_version()}", "body": "changelog"}
 
+    # Always assume that we can perform multiple update checks in a row.
+    monkeypatch.setattr(updater, "_should_postpone_update_check", lambda: False)
+
     # Make requests.get().json() return the above dictionary.
     requests_mock = mocker.MagicMock()
     requests_mock().json.return_value = mock_upstream_info
@@ -37,6 +39,9 @@ def test_qt(
         updater.start()
 
     assert len(window.hamburger_button.menu().actions()) == 3
+    expected_settings["updater_last_check"] = updater.dangerzone.settings.get(
+        "updater_last_check"
+    )
     assert updater.dangerzone.settings.get_updater_settings() == expected_settings
 
     mock_upstream_info["tag_name"] = "v99.9.9"

tests/gui/test_updater.py

@@ -1,6 +1,7 @@
 import json
 import os
 import platform
+import time
 from pathlib import Path
 
 import pytest
@@ -118,17 +119,6 @@ def test_post_0_4_2_settings(tmp_path: Path, monkeypatch: MonkeyPatch) -> None:
     assert updater.dangerzone.settings.get_updater_settings() == expected_settings
 
 
-# FIXME: This test should not fail
-@pytest.mark.xfail(reason="We haven't enabled this behavior in Linux yet")
-def test_linux_first_run(updater: UpdaterThread) -> None:
-    """Check that Linux users are not prompted for update checks."""
-    expected_settings = default_updater_settings()
-    expected_settings["updater_first_run"] = False
-    expected_settings["updater_check"] = False
-    assert updater.should_check_for_updates() == False
-    assert updater.dangerzone.settings.get_updater_settings() == expected_settings
-
-
 # FIXME: Make this check run only on Windows/MacOS platforms.
 def test_user_prompts(
     updater: UpdaterThread, monkeypatch: MonkeyPatch, mocker: MockerFixture
@@ -139,8 +129,8 @@ def test_user_prompts(
     # When Dangerzone runs for the first time, users should not be asked to enable
     # updates.
     expected_settings = default_updater_settings()
-    expected_settings["updater_first_run"] = False
     expected_settings["updater_check"] = None
+    expected_settings["updater_last_check"] = 0
     assert updater.should_check_for_updates() == False
     assert updater.dangerzone.settings.get_updater_settings() == expected_settings
 
@@ -186,6 +176,9 @@ def test_update_checks(
     requests_mock().json.return_value = mock_upstream_info
     monkeypatch.setattr(updater_module.requests, "get", requests_mock)
 
+    # Always assume that we can perform multiple update checks in a row.
+    monkeypatch.setattr(updater, "_should_postpone_update_check", lambda: False)
+
     # Test 1 - Check that the current version triggers no updates.
     report = updater.check_for_updates()
     assert_report_equal(report, UpdateReport())
@@ -211,3 +204,71 @@ def test_update_checks(
     assert_report_equal(
         report, UpdateReport(version="99.9.9", changelog="<p>changelog</p>")
     )
+
+
+# FIXME: Make this check run only on Windows/MacOS platforms.
+def test_update_checks_cooldown(updater: UpdaterThread, mocker: MockerFixture) -> None:
+    """Make sure Dangerzone only checks for updates every X hours"""
+    updater.dangerzone.settings.set("updater_check", True)
+    updater.dangerzone.settings.set("updater_last_check", 0)
+
+    # Mock some functions before the tests start
+    cooldown_spy = mocker.spy(updater, "_should_postpone_update_check")
+    timestamp_mock = mocker.patch.object(updater, "_get_now_timestamp")
+    mocker.patch("dangerzone.gui.updater.requests.get")
+    requests_mock = updater_module.requests.get
+
+    # # Make requests.get().json() return the version info that we want.
+    mock_upstream_info = {"tag_name": "99.9.9", "body": "changelog"}
+    requests_mock().json.return_value = mock_upstream_info  # type: ignore [attr-defined, call-arg]
+
+    # Test 1: The first time Dangerzone checks for updates, the cooldown period should
+    # not stop it. Once we learn about an update, the last check setting should be
+    # bumped.
+    curtime = int(time.time())
+    timestamp_mock.return_value = curtime
+
+    report = updater.check_for_updates()
+    assert cooldown_spy.spy_return == False
+    assert updater.dangerzone.settings.get("updater_last_check") == curtime
+    assert_report_equal(report, UpdateReport("99.9.9", "<p>changelog</p>"))
+
+    # Test 2: Advance the current time by 1 second, and ensure that no update will take
+    # place, due to the cooldown period. The last check timestamp should remain the
+    # previous one.
+    curtime += 1
+    timestamp_mock.return_value = curtime
+    requests_mock.side_effect = Exception("failed")  # type: ignore [attr-defined]
+    updater.dangerzone.settings.set("updater_latest_version", get_version())
+    updater.dangerzone.settings.set("updater_latest_changelog", None)
+
+    report = updater.check_for_updates()
+    assert cooldown_spy.spy_return == True
+    assert updater.dangerzone.settings.get("updater_last_check") == curtime - 1
+    assert_report_equal(report, UpdateReport())
+
+    # Test 3: Advance the current time by <cooldown period> seconds. Ensure that
+    # Dangerzone checks for updates again, and the last check timestamp gets bumped.
+    curtime += updater_module.UPDATE_CHECK_COOLDOWN_SECS
+    timestamp_mock.return_value = curtime
+    requests_mock.side_effect = None  # type: ignore [attr-defined]
+
+    report = updater.check_for_updates()
+    assert cooldown_spy.spy_return == False
+    assert updater.dangerzone.settings.get("updater_last_check") == curtime
+    assert_report_equal(report, UpdateReport("99.9.9", "<p>changelog</p>"))
+
+    # Test 4: Make Dangerzone check for updates again, but this time, it should
+    # encounter an error while doing so. In that case, the last check timestamp
+    # should be bumped, so that subsequent checks don't take place.
+    updater.dangerzone.settings.set("updater_latest_version", get_version())
+    updater.dangerzone.settings.set("updater_latest_changelog", None)
+
+    curtime += updater_module.UPDATE_CHECK_COOLDOWN_SECS
+    timestamp_mock.return_value = curtime
+    requests_mock.side_effect = Exception("failed")  # type: ignore [attr-defined]
+
+    report = updater.check_for_updates()
+    assert cooldown_spy.spy_return == False
+    assert updater.dangerzone.settings.get("updater_last_check") == curtime
+    assert_report_equal(report, UpdateReport(error="failed"))