Merge pull request #363 from christianlouis/copilot/fix-settings-rollback-issue

fix(settings): rollback uses old_value, Remove from DB button always visible

app/api/settings.py

@@ -434,12 +434,13 @@ async def rollback_setting_to_history(
     admin: AdminUser,
 ):
     """
-    Revert a setting to the value it held at a specific point in the audit log.
+    Revert a setting to the value it had *before* a specific audit log change.
 
     The ``history_id`` is the ID of the :class:`~app.models.SettingsAuditLog`
-    entry whose ``new_value`` should be reinstated.  If that entry recorded a
-    deletion (``new_value`` is ``None``), the setting is removed from the
-    database and reverts to its ENV/default value.
+    entry whose ``old_value`` should be reinstated, effectively undoing that
+    change.  If ``old_value`` is ``None`` (the setting did not exist before
+    that change), the setting is removed from the database and reverts to its
+    ENV/default value.
 
     A new audit log entry is written to record the rollback.
     Admin only.

app/utils/settings_service.py

@@ -1201,19 +1201,20 @@ def get_setting_history(db: Session, key: str) -> List[Dict[str, Any]]:
 
 def rollback_setting(db: Session, key: str, history_id: int, changed_by: str = "system") -> bool:
     """
-    Revert a setting to the value recorded in a specific audit log entry.
+    Revert a setting to the value it had *before* a specific audit log entry.
 
-    The value stored in the chosen history entry's ``new_value`` field is
-    re-applied as the current database value.  If that value is ``None``
-    (i.e. the entry recorded a deletion) the setting is removed from the
-    database entirely, reverting to ENV/defaults.
+    The value stored in the chosen history entry's ``old_value`` field is
+    re-applied as the current database value, effectively undoing that change.
+    If ``old_value`` is ``None`` (i.e. the setting did not exist before that
+    change) the setting is removed from the database entirely, reverting to
+    ENV/defaults.
 
     A new audit log entry is written to record the rollback operation.
 
     Args:
         db: Database session
         key: Setting key to roll back
-        history_id: ID of the SettingsAuditLog entry whose ``new_value``
+        history_id: ID of the SettingsAuditLog entry whose ``old_value``
                     should become the restored value
         changed_by: Username performing the rollback (for audit log)
 
@@ -1229,10 +1230,10 @@ def rollback_setting(db: Session, key: str, history_id: int, changed_by: str = "
             logger.warning(f"Rollback failed: audit log entry {history_id} not found for key '{key}'")
             return False
 
-        target_value = history_entry.new_value
+        target_value = history_entry.old_value
 
         if target_value is None:
-            # The history entry recorded a deletion – reinstate that by deleting the current db value
+            # The old value was empty – remove the current db value to revert to ENV/default
             return delete_setting_from_db(db, key, changed_by=changed_by)
         else:
             return save_setting_to_db(db, key, target_value, changed_by=changed_by)

frontend/templates/audit_log.html

@@ -73,7 +73,7 @@
           <td class="px-4 py-3 text-sm">
             <button
               type="button"
-              @click="rollback('{{ entry.key }}', {{ entry.id }}, '{{ entry.new_value or '' }}')"
+              @click="rollback('{{ entry.key }}', {{ entry.id }}, '{{ entry.old_value or '' }}')"
               :disabled="rollingBack === {{ entry.id }}"
               class="inline-flex items-center px-2 py-1 text-xs font-medium rounded border border-gray-300 text-gray-700 bg-white hover:bg-yellow-50 hover:border-yellow-400 focus:outline-none focus:ring-2 focus:ring-offset-1 focus:ring-yellow-400 disabled:opacity-50 disabled:cursor-not-allowed"
               title="Revert '{{ entry.key }}' to the value in this log entry"

frontend/templates/settings.html

@@ -201,14 +201,14 @@
               </button>
               <button
                 type="button"
-                x-show="isDbOverride['{{ setting.key }}'] && formData['{{ setting.key }}'] === originalData['{{ setting.key }}']"
+                x-show="isDbOverride['{{ setting.key }}']"
                 x-transition
                 @click="revertSetting('{{ setting.key }}')"
                 :disabled="revertingKey === '{{ setting.key }}'"
                 class="px-3 py-1 text-sm bg-orange-500 text-white rounded-md hover:bg-orange-600 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-orange-400 disabled:opacity-50 disabled:cursor-not-allowed whitespace-nowrap"
                 title="Remove DB override and revert to environment variable or default"
               >
-                <span x-show="revertingKey !== '{{ setting.key }}'"><i class="fas fa-undo mr-1"></i>Back to ENV</span>
+                <span x-show="revertingKey !== '{{ setting.key }}'"><i class="fas fa-trash-alt mr-1"></i>Remove from DB</span>
                 <span x-show="revertingKey === '{{ setting.key }}'">Reverting…</span>
               </button>
             </div>
@@ -330,6 +330,10 @@ function settingsApp() {
     },
 
     async revertSetting(key) {
+      if (!confirm(`Remove '${key}' from the database?\n\nThe setting will revert to its environment variable or default value.`)) {
+        return;
+      }
+
       this.revertingKey = key;
       this.hideAlert();
 

tests/test_settings_audit_log.py

@@ -203,19 +203,36 @@ class TestRollbackSetting:
     """rollback_setting reinstates the value from a given audit log entry."""
 
     def test_rollback_to_previous_value(self, db_session):
+        """Rolling back an entry restores the old_value (the value *before* that change)."""
         from app.utils.settings_service import get_setting_from_db, rollback_setting, save_setting_to_db
 
-        save_setting_to_db(db_session, "workdir", "/v1", changed_by="admin")  # entry id 1
-        save_setting_to_db(db_session, "workdir", "/v2", changed_by="admin")  # entry id 2
+        save_setting_to_db(db_session, "workdir", "/v1", changed_by="admin")  # entry 1: old=None, new=/v1
+        save_setting_to_db(db_session, "workdir", "/v2", changed_by="admin")  # entry 2: old=/v1, new=/v2
 
-        first_entry = db_session.query(SettingsAuditLog).filter_by(key="workdir").first()
-        # first entry has new_value="/v1"
-        success = rollback_setting(db_session, "workdir", first_entry.id, changed_by="rollbacker")
+        # Rolling back entry 2 should undo the /v1→/v2 change and restore /v1
+        second_entry = (
+            db_session.query(SettingsAuditLog).filter_by(key="workdir").order_by(SettingsAuditLog.id.desc()).first()
+        )
+        success = rollback_setting(db_session, "workdir", second_entry.id, changed_by="rollbacker")
 
         assert success is True
         current = get_setting_from_db(db_session, "workdir")
         assert current == "/v1"
 
+    def test_rollback_deletes_setting_when_old_value_is_none(self, db_session):
+        """Rolling back the first-ever entry (old_value=None) deletes the setting from DB."""
+        from app.utils.settings_service import get_setting_from_db, rollback_setting, save_setting_to_db
+
+        save_setting_to_db(db_session, "workdir", "/v1", changed_by="admin")  # old=None, new=/v1
+
+        first_entry = db_session.query(SettingsAuditLog).filter_by(key="workdir").first()
+        success = rollback_setting(db_session, "workdir", first_entry.id, changed_by="rollbacker")
+
+        assert success is True
+        # Setting should be removed from DB (fallback to ENV/default)
+        current = get_setting_from_db(db_session, "workdir")
+        assert current is None
+
     def test_rollback_creates_new_audit_entry(self, db_session):
         from app.utils.settings_service import rollback_setting, save_setting_to_db