Revert "Add shortcuts preferences to User model (#10038)" (#10054)

This reverts commit 765f6eb.

Author: santicomp2014

h/migrations/versions/7c1a3e2b9f20_add_user_shortcuts_preferences_column.py

@@ -4,7 +4,6 @@
 from typing import TYPE_CHECKING
 
 import sqlalchemy as sa
-from sqlalchemy.dialects.postgresql import JSONB
 from sqlalchemy.ext.declarative import declared_attr
 from sqlalchemy.ext.hybrid import Comparator, hybrid_property
 from sqlalchemy.orm import Mapped, mapped_column
@@ -232,10 +231,6 @@ def __table_args__(cls):  # noqa: N805
     # Has the user opted-in for news etc.
     comms_opt_in = sa.Column(sa.Boolean, nullable=True)
 
-    # A JSON blob with user shortcuts preferences.
-    # Use SQL NULL (not JSON null) when clearing the value.
-    shortcuts_preferences = sa.Column(JSONB(none_as_null=True), nullable=True)
-
     identities = sa.orm.relationship(
         "UserIdentity", backref="user", cascade="all, delete-orphan"
     )

h/models/user.py

@@ -1,26 +1,10 @@
-from collections.abc import Mapping
-
 import sqlalchemy as sa
 
 from h.models import User, UserIdentity
 from h.util.db import on_transaction_end
 from h.util.user import split_user
 
-UPDATE_PREFS_ALLOWED_KEYS = {
-    "show_sidebar_tutorial",
-    "shortcuts_preferences",
-}
-REPEATABLE_SHORTCUT_GROUPS = []
-ALLOWED_SHORTCUT_ACTIONS = {
-    "applyUpdates",
-    "openKeyboardShortcuts",
-    "openSearch",
-    "annotateSelection",
-    "highlightSelection",
-    "toggleHighlights",
-    "showSelection",
-    "hideAdder",
-}
+UPDATE_PREFS_ALLOWED_KEYS = {"show_sidebar_tutorial"}
 
 
 class UserNotActivated(Exception):  # noqa: N818
@@ -187,61 +171,7 @@ def update_preferences(user, **kwargs):
         if "show_sidebar_tutorial" in kwargs:  # pragma: no cover
             user.sidebar_tutorial_dismissed = not kwargs["show_sidebar_tutorial"]
 
-        if "shortcuts_preferences" in kwargs:
-            updated = kwargs["shortcuts_preferences"]
-            validated = _validate_shortcuts_preferences(updated)
-            user.shortcuts_preferences = validated
-
 
 def user_service_factory(_context, request):
     """Return a UserService instance for the passed context and request."""
     return UserService(default_authority=request.default_authority, session=request.db)
-
-
-def _validate_shortcuts_preferences(preferences):
-    if preferences is None:
-        return None
-    if not isinstance(preferences, Mapping):
-        message = "shortcuts_preferences must be a mapping"
-        raise TypeError(message)
-
-    # Check for invalid keys
-    invalid_keys = set(preferences.keys()) - ALLOWED_SHORTCUT_ACTIONS
-    if invalid_keys:
-        keys = ", ".join(sorted(invalid_keys))
-        message = f"shortcuts_preferences with keys {keys} are not allowed"
-        raise TypeError(message)
-
-    # Check for invalid shortcut values
-    has_invalid_value = any(
-        not isinstance(value, str) and value is not None
-        for value in preferences.values()
-    )
-    if has_invalid_value:
-        message = "shortcuts_preferences values must be strings or None"
-        raise TypeError(message)
-
-    # Check for duplicate shortcut values
-    actions_by_value = {}
-    for action, value in preferences.items():
-        actions_by_value.setdefault(value, set()).add(action)
-
-    for value, actions in actions_by_value.items():
-        if value is None:
-            continue
-        if len(actions) <= 1:
-            continue
-        if _allow_duplicate_shortcuts(actions):
-            continue
-        actions_list = ", ".join(sorted(actions))
-        message = (
-            "shortcuts_preferences has duplicate shortcut values for actions "
-            f"{actions_list}"
-        )
-        raise TypeError(message)
-
-    return preferences
-
-
-def _allow_duplicate_shortcuts(actions):
-    return any(actions <= group for group in REPEATABLE_SHORTCUT_GROUPS)

h/services/user.py

@@ -95,8 +95,6 @@ def _user_preferences(user):
     preferences = {}
     if user and not user.sidebar_tutorial_dismissed:
         preferences["show_sidebar_tutorial"] = True
-    if user and user.shortcuts_preferences is not None:
-        preferences["shortcuts_preferences"] = user.shortcuts_preferences
     return preferences
 
 

h/session.py

@@ -1,7 +1,6 @@
 from datetime import datetime, timedelta
 
 import pytest
-import sqlalchemy as sa
 from sqlalchemy import exc
 from sqlalchemy.sql.elements import BinaryExpression
 
@@ -204,21 +203,6 @@ def test_privacy_accepted_defaults_to_None(self):
         # nullable
         assert User().privacy_accepted is None
 
-    def test_shortcuts_preferences_defaults_to_None(self):
-        assert User().shortcuts_preferences is None
-
-    def test_shortcuts_preferences_persists_none_as_sql_null(self, db_session, user):
-        user.shortcuts_preferences = {"openSearch": "k"}
-        db_session.flush()
-
-        user.shortcuts_preferences = None
-        db_session.flush()
-
-        is_null = db_session.execute(
-            sa.select(User.shortcuts_preferences.is_(None)).where(User.id == user.id)
-        ).scalar_one()
-        assert is_null is True
-
     def test_repr(self, user):
         assert repr(user) == f"User(id={user.id})"
 

tests/unit/h/models/user_test.py

@@ -109,116 +109,6 @@ def test_update_preferences_raises_for_unsupported_keys(self, svc, factories):
 
         assert "keys baz, foo are not allowed" in str(exc.value)
 
-    def test_update_preferences_rejects_shortcuts_preferences_non_mapping(
-        self, svc, factories
-    ):
-        user = factories.User.build()
-
-        with pytest.raises(TypeError) as exc:
-            svc.update_preferences(user, shortcuts_preferences="qux")
-
-        assert "shortcuts_preferences must be a mapping" in str(exc.value)
-
-    def test_update_preferences_rejects_shortcuts_preferences_invalid_keys(
-        self, svc, factories
-    ):
-        user = factories.User.build()
-
-        with pytest.raises(TypeError) as exc:
-            svc.update_preferences(user, shortcuts_preferences={"notAllowed": "k"})
-
-        assert "keys notAllowed are not allowed" in str(exc.value)
-
-    def test_update_preferences_rejects_shortcuts_preferences_invalid_values(
-        self, svc, factories
-    ):
-        user = factories.User.build()
-
-        with pytest.raises(TypeError) as exc:
-            svc.update_preferences(user, shortcuts_preferences={"openSearch": 1})
-
-        assert "values must be strings or None" in str(exc.value)
-
-    def test_update_preferences_rejects_shortcut_value_duplicates(self, svc, factories):
-        user = factories.User.build()
-
-        with pytest.raises(TypeError) as exc:
-            svc.update_preferences(
-                user,
-                shortcuts_preferences={
-                    "applyUpdates": "l",
-                    "openKeyboardShortcuts": "l",
-                },
-            )
-
-        assert "duplicate shortcut values" in str(exc.value)
-
-    def test_update_preferences_allows_repeatable_shortcut_duplicates(
-        self, svc, factories, monkeypatch
-    ):
-        user = factories.User.build()
-
-        monkeypatch.setattr(
-            "h.services.user.REPEATABLE_SHORTCUT_GROUPS",
-            [{"applyUpdates", "openKeyboardShortcuts"}],
-        )
-
-        svc.update_preferences(
-            user,
-            shortcuts_preferences={
-                "applyUpdates": "l",
-                "openKeyboardShortcuts": "l",
-            },
-        )
-
-        assert user.shortcuts_preferences == {
-            "applyUpdates": "l",
-            "openKeyboardShortcuts": "l",
-        }
-
-    def test_update_preferences_allows_none_shortcut_values(self, svc, factories):
-        user = factories.User.build()
-
-        svc.update_preferences(
-            user,
-            shortcuts_preferences={
-                "applyUpdates": None,
-                "openKeyboardShortcuts": None,
-            },
-        )
-
-        assert user.shortcuts_preferences == {
-            "applyUpdates": None,
-            "openKeyboardShortcuts": None,
-        }
-
-    def test_update_preferences_accepts_valid_shortcuts_preferences(
-        self, svc, factories
-    ):
-        user = factories.User.build()
-
-        svc.update_preferences(
-            user,
-            shortcuts_preferences={
-                "openSearch": "k",
-                "applyUpdates": "l",
-                "openKeyboardShortcuts": "?",
-            },
-        )
-
-        assert user.shortcuts_preferences == {
-            "openSearch": "k",
-            "applyUpdates": "l",
-            "openKeyboardShortcuts": "?",
-        }
-
-    def test_update_preferences_allows_none_shortcuts_preferences(self, svc, factories):
-        user = factories.User.build(shortcuts_preferences={"openSearch": "k"})
-
-        svc.update_preferences(user, shortcuts_preferences=None)
-
-        assert user.shortcuts_preferences is None
-
     def test_sets_up_cache_clearing_on_transaction_end(self, patch, db_session):
         decorator = patch("h.services.user.on_transaction_end")
 

tests/unit/h/services/user_test.py

@@ -82,23 +82,6 @@ def test_authenticated_sidebar_tutorial(self, authenticated_request, dismissed):
         else:
             assert preferences["show_sidebar_tutorial"] is True
 
-    def test_authenticated_includes_shortcuts_preferences(self, authenticated_request):
-        shortcuts_preferences = {"applyUpdates": "l"}
-        authenticated_request.user.shortcuts_preferences = shortcuts_preferences
-
-        preferences = session.model(authenticated_request)["preferences"]
-
-        assert preferences["shortcuts_preferences"] == shortcuts_preferences
-
-    def test_authenticated_omits_shortcuts_preferences_when_none(
-        self, authenticated_request
-    ):
-        authenticated_request.user.shortcuts_preferences = None
-
-        preferences = session.model(authenticated_request)["preferences"]
-
-        assert "shortcuts_preferences" not in preferences
-
 
 class TestProfile:
     def test_userid_unauthenticated(self, unauthenticated_request):
@@ -183,23 +166,6 @@ def test_authenticated_sidebar_tutorial(self, authenticated_request, dismissed):
         else:
             assert preferences["show_sidebar_tutorial"] is True
 
-    def test_authenticated_includes_shortcuts_preferences(self, authenticated_request):
-        shortcuts_preferences = {"applyUpdates": "l"}
-        authenticated_request.user.shortcuts_preferences = shortcuts_preferences
-
-        preferences = session.profile(authenticated_request)["preferences"]
-
-        assert preferences["shortcuts_preferences"] == shortcuts_preferences
-
-    def test_authenticated_omits_shortcuts_preferences_when_none(
-        self, authenticated_request
-    ):
-        authenticated_request.user.shortcuts_preferences = None
-
-        preferences = session.profile(authenticated_request)["preferences"]
-
-        assert "shortcuts_preferences" not in preferences
-
     def test_anonymous_authority(self, unauthenticated_request, authority):
         assert session.profile(unauthenticated_request)["authority"] == authority