Fix code review notes

Rename the db column, move test data from a specific call to the initializer, fix alembic migration downstream ref, remove CSS changes (pending another commit to fix fully
7 years ago · 3d080e12dd
11 changed files with 32 additions and 38 deletions
--- a/tildes/alembic/versions/347859b0355e_added_account_default_theme_setting.py
+++ b/tildes/alembic/versions/347859b0355e_added_account_default_theme_setting.py
@ -11,19 +11,14 @@ import sqlalchemy as sa
 # revision identifiers, used by Alembic.
 revision = "347859b0355e"
 down_revision = "fab922a8bb04"
 down_revision = "50c251c4a19c"
 branch_labels = None
 depends_on = None
 def upgrade():
    op.add_column(
        "users",
        sa.Column(
            "theme_account_default", sa.Text(), server_default="", nullable=False
        ),
    )
    op.add_column("users", sa.Column("theme_default", sa.Text()))
 def downgrade():
    op.drop_column("users", "theme_account_default")
    op.drop_column("users", "theme_default")
--- a/tildes/scss/modules/_form.scss
+++ b/tildes/scss/modules/_form.scss
@ -38,10 +38,6 @@ select.form-select:not([multiple]) {
  }
 }
 .form-buttons.no-flex-reverse {
  flex-direction: row;
 }
 textarea.form-input {
  height: 8rem;
  line-height: 1.5;
--- a/tildes/scss/modules/_tab.scss
+++ b/tildes/scss/modules/_tab.scss
@ -28,7 +28,3 @@
    }
  }
 }
 .tab-listing-order.tab-flex-2 {
  flex: 2;
 }
--- a/tildes/static/js/behaviors/theme-selector.js
+++ b/tildes/static/js/behaviors/theme-selector.js
@ -3,9 +3,9 @@ $.onmount('[data-js-theme-selector]', function() {
        event.preventDefault();
        var new_theme = $(this).val();
        var selected_text = $(this).find('option:selected').text()
        var $setDefaultLink = $('#button-set-default-theme')
        var $formDefaultValue = $('#input-set-default-theme')
        var selected_text = $(this).find('option:selected').text();
        var $setDefaultLink = $('#button-set-default-theme');
        var $formDefaultValue = $('#input-set-default-theme');
        // persist the new theme for the user in their cookie
        document.cookie = 'theme=' + new_theme + ';' +
@ -27,13 +27,13 @@ $.onmount('[data-js-theme-selector]', function() {
        }
        // set the IC hidden input with the new value
        $formDefaultValue.val(new_theme)
        $formDefaultValue.val(new_theme);
        // set visibility of 'Set as account default' link
        if (selected_text.indexOf('(account default)') !== -1) {
            $setDefaultLink.css('visibility', 'hidden')
            $setDefaultLink.css('visibility', 'hidden');
        } else {
            $setDefaultLink.css('visibility', 'visible')
            $setDefaultLink.css('visibility', 'visible');
        }
    });
 });
--- a/tildes/tests/conftest.py
+++ b/tildes/tests/conftest.py
@ -202,7 +202,11 @@ def webtest(base_app):
        base_app,
        # This "tm.active" is a temporary fix around this fixture failing to rollback
        # data after the tests are complete.
        extra_environ={"wsgi.url_scheme": "https", "tm.active": True},
        extra_environ={
            "wsgi.url_scheme": "https",
            "tm.active": True,
            "REMOTE_ADDR": "0.0.0.0",
        },
        cookiejar=CookieJar(),
    )
@ -212,7 +216,7 @@ def webtest(base_app):
    login_page.form["password"] = "session user password"
    # The login process requires a non-None IP address; this
    # populates `request.client_addr`.
    login_page.form.submit(extra_environ={"HTTP_X_FORWARDED_FOR": "0.0.0.0"})
    login_page.form.submit()
    yield app
--- a/tildes/tildes/models/user/user.py
+++ b/tildes/tildes/models/user/user.py
@ -87,7 +87,7 @@ class User(DatabaseModel):
        Boolean, nullable=False, server_default="false"
    )
    open_new_tab_text: bool = Column(Boolean, nullable=False, server_default="false")
    theme_account_default: str = Column(Text, nullable=False, server_default="")
    theme_default: str = Column(Text)
    is_banned: bool = Column(Boolean, nullable=False, server_default="false")
    permissions: Any = Column(JSONB)
    home_default_order: Optional[TopicSortOption] = Column(ENUM(TopicSortOption))
--- a/tildes/tildes/templates/base.jinja2
+++ b/tildes/tildes/templates/base.jinja2
@ -38,8 +38,8 @@
 {% block body_tag %}
  {% if request.cookies.get('theme', '') %}
  <body class="theme-{{ request.cookies.get('theme', '') }}">
  {% elif request.user and request.user.theme_account_default %}
  <body class="theme-{{ request.user.theme_account_default }}">
  {% elif request.user and request.user.theme_default %}
  <body class="theme-{{ request.user.theme_default }}">
  {% else %}
  <body>
  {% endif %}
--- a/tildes/tildes/templates/base_no_sidebar.jinja2
+++ b/tildes/tildes/templates/base_no_sidebar.jinja2
@ -3,8 +3,8 @@
 {% block body_tag %}
  {% if request.cookies.get('theme', '') %}
  <body class="l-no-sidebar theme-{{ request.cookies.get('theme', '') }}">
  {% elif request.user and request.user.theme_account_default %}
  <body class="l-no-sidebar theme-{{ request.user.theme_account_default }}">
  {% elif request.user and request.user.theme_default %}
  <body class="l-no-sidebar theme-{{ request.user.theme_default }}">
  {% else %}
  <body class="l-no-sidebar">
  {% endif %}
--- a/tildes/tildes/templates/settings.jinja2
+++ b/tildes/tildes/templates/settings.jinja2
@ -8,8 +8,13 @@
  <ul class="settings-list">
    <li>
      <label for="theme">Choose a display theme:</label>
      <div class="listing-options">
        <menu class="tab-listing-order tab-flex-2">
      {#
        2 styling changes needed:
          - the select should be wider, as wide as it is now (roughly) on live
          - the success message should be to the rigt of the label button
      #}
      <div class="">
        <menu class="">
          <select class="form-select" name="theme" id="theme" data-js-theme-selector>
            {% for theme, description in theme_options.items() %}
              <option
@ -31,7 +36,7 @@
          <div class="form-buttons no-flex-reverse">
            <button id="button-set-default-theme"
              class="btn btn-link
              {% if current_theme == request.user.theme_account_default %}d-invisible{% endif %}"
              {% if current_theme == request.user.theme_default %}d-invisible{% endif %}"
            >
              Set as account default
            </button>
--- a/tildes/tildes/views/api/web/user.py
+++ b/tildes/tildes/views/api/web/user.py
@ -202,12 +202,12 @@ def patch_change_track_comment_visits(request: Request) -> Response:
    request_param="ic-trigger-name=account-default-theme",
    permission="change_account_default_theme_setting",
 )
 def change_account_default_theme(request: Request) -> Response:
 def patch_change_account_default_theme(request: Request) -> Response:
    """Change the user's "theme account default" setting."""
    user = request.context
    new_theme = request.params.get("theme")
    user.theme_account_default = new_theme
    user.theme_default = new_theme
    return IC_NOOP
--- a/tildes/tildes/views/settings.py
+++ b/tildes/tildes/views/settings.py
@ -20,10 +20,8 @@ DEFAULT_THEME_NAME = "white"
@view_config(route_name="settings", renderer="settings.jinja2")
 def get_settings(request: Request) -> dict:
    """Generate the user settings page."""
    current_theme = (
        request.cookies.get("theme", "") or request.user.theme_account_default
    )
    user_default = request.user.theme_account_default or "white"
    current_theme = request.cookies.get("theme", "") or request.user.theme_default
    user_default = request.user.theme_default or DEFAULT_THEME_NAME
    theme_options = {
        "white": "White",
        "light": "Solarized Light",