Do some refinements on the new my/security page: Collapse 2fa options in one menu, change button colors, use Primer components for inlineMessage

app/components/components/on_off_status_component.html.erb

@@ -29,20 +29,12 @@ See COPYRIGHT and LICENSE files for more details.
 
 <%# The javscript for this cell is included in `settings.js.erb`. %>
 
-<div class="on-off-status-cell">
-  <% if enabled? %>
-  <span class="on-off-status -enabled">
-    <%= helpers.op_icon "icon-yes" %>
-    <%= model[:on_text] %>
-  </span>
-      <p>
-        <%= model[:on_description] %>
-      </p>
-  <% else %>
-  <span class="on-off-status -disabled">
-    <%= helpers.op_icon "icon-not-supported" %>
-    <%= model[:off_text] %>
-  </span>
-      <p><%= model[:off_description] %></p>
+<%= render Primer::OpenProject::InlineMessage.new(scheme:, classes:) do %>
+  <%= render(Primer::Beta::Text.new(tag: :span)) do %>
+    <%= enabled? ? model[:on_text] : model[:off_text] %>
   <% end %>
-</div>
+<% end %>
+
+<span>
+  <%= enabled? ? model[:on_description] : model[:off_description] %>
+</span>

app/components/components/on_off_status_component.rb

@@ -34,13 +34,31 @@ module Components
   class OnOffStatusComponent < ::ApplicationComponent
     options :on_text
     options :on_description
+    options :on_scheme
     options :off_text
     options :off_description
+    options :off_scheme
 
     options :is_on
 
     def enabled?
       !!model[:is_on]
     end
+
+    def scheme
+      if enabled?
+        model[:on_scheme] || :success
+      else
+        model[:off_scheme] || :critical
+      end
+    end
+
+    def classes
+      [].tap do |classes|
+        classes << "on-off-status"
+        classes << "-enabled" if enabled?
+        classes << "-disabled" unless enabled?
+      end
+    end
   end
 end

app/forms/my/password_form.rb

@@ -29,10 +29,11 @@
 #++
 
 class My::PasswordForm < ApplicationForm
-  def initialize(user:, back_url: nil)
+  def initialize(user:, back_url: nil, submit_button_scheme: :primary)
     super()
     @user = user
     @back_url = back_url
+    @submit_button_scheme = submit_button_scheme
   end
 
   form do |f|
@@ -69,7 +70,7 @@ class My::PasswordForm < ApplicationForm
         autocomplete: "new-password"
       )
 
-      fg.submit(name: :submit, label: helpers.t(:button_save), scheme: :primary)
+      fg.submit(name: :submit, label: helpers.t(:button_change_password), scheme: @submit_button_scheme)
     end
   end
 end

app/views/my/_password.html.erb

@@ -1,7 +1,7 @@
 <% if @user.change_password_allowed? %>
   <%= error_messages_for "user" %>
   <%=
-    settings_primer_form_with(
+    primer_form_with(
       url: { action: :change_password },
       method: :post,
       autocomplete: "off",
@@ -10,7 +10,7 @@
         controller: "password-requirements"
       }
     ) do |form|
-      render(My::PasswordForm.new(form, user: @user, back_url: params[:back_url]))
+      render(My::PasswordForm.new(form, user: @user, back_url: params[:back_url], submit_button_scheme: local_assigns.fetch(:submit_button_scheme, :primary)))
     end
   %>
 <% end %>

app/views/my/security.html.erb

@@ -38,6 +38,6 @@ See COPYRIGHT and LICENSE files for more details.
   end
 %>
 
-<%= render "my/password" %>
+<%= render partial: "my/password", locals: { submit_button_scheme: :secondary } %>
 
 <%= call_hook(:view_my_security_2fa_section, user: @user, cookies:) %>

frontend/src/global_styles/content/_index.sass

@@ -58,7 +58,6 @@
 @import datepicker
 @import focus_within
 @import help_texts
-@import on_off_status
 @import custom_actions
 @import user_mention
 @import hide_section

frontend/src/global_styles/content/_on_off_status.sass

@@ -1,40 +0,0 @@
-//-- copyright
-// OpenProject is an open source project management software.
-// Copyright (C) the OpenProject GmbH
-//
-// This program is free software; you can redistribute it and/or
-// modify it under the terms of the GNU General Public License version 3.
-//
-// OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
-// Copyright (C) 2006-2013 Jean-Philippe Lang
-// Copyright (C) 2010-2013 the ChiliProject Team
-//
-// This program is free software; you can redistribute it and/or
-// modify it under the terms of the GNU General Public License
-// as published by the Free Software Foundation; either version 2
-// of the License, or (at your option) any later version.
-//
-// This program is distributed in the hope that it will be useful,
-// but WITHOUT ANY WARRANTY; without even the implied warranty of
-// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-// GNU General Public License for more details.
-//
-// You should have received a copy of the GNU General Public License
-// along with this program; if not, write to the Free Software
-// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
-//
-// See COPYRIGHT and LICENSE files for more details.
-//++
-
-.on-off-status
-  display: inline-block
-  font-weight: var(--base-text-weight-bold)
-
-  i
-    padding-right: 5px
-
-  &.-enabled
-    color: #41B478
-
-  &.-disabled
-    color: #CA3F3F

modules/two_factor_authentication/app/components/two_factor_authentication/devices/row_component.rb

@@ -8,9 +8,7 @@ module ::TwoFactorAuthentication
       end
 
       def row_css_class
-        is_default = "blocked" if device.default
-
-        ["mobile-otp--two-factor-device-row", is_default].compact.join(" ")
+        "mobile-otp--two-factor-device-row"
       end
 
       def device_type
@@ -25,12 +23,11 @@ module ::TwoFactorAuthentication
         end
       end
 
-      def confirmed
+      def active
         if device.active
           render(Primer::Beta::Octicon.new(icon: :check))
         elsif table.self_table?
-          link_to t("two_factor_authentication.devices.confirm_now"),
-                  { controller: table.target_controller, action: :confirm, device_id: device.id }
+          "-"
         else
           render(Primer::Beta::Octicon.new(icon: :x))
         end
@@ -39,57 +36,70 @@ module ::TwoFactorAuthentication
       ###
 
       def button_links
-        links = []
-        links << make_default_button unless device.default
-        links << delete_button
-
-        links
+        [menu_button]
       end
 
-      def make_default_button
-        helpers.form_tag(
-          { controller: table.target_controller, action: :make_default, device_id: device.id },
-          method: :post,
-          id: "two_factor_make_default_form",
-          data: helpers.password_confirmation_data_attribute({})
-        ) do
-          render(
-            Primer::Beta::IconButton.new(
-              icon: :star,
-              tag: :button,
-              size: :small,
-              type: :submit,
-              test_selector: "two-factor--make-default-button",
-              "aria-label": I18n.t(:button_make_default)
-            )
+      def menu_button
+        render(Primer::Alpha::ActionMenu.new) do |menu|
+          menu.with_show_button(
+            icon: "kebab-horizontal",
+            scheme: :invisible,
+            "aria-label": t(:label_actions),
+            test_selector: "two-factor--actions-button"
           )
+
+          make_default_action(menu)
+          make_active_action(menu) if table.self_table?
+          delete_action(menu)
         end
       end
 
-      def delete_button
-        helpers.form_tag(
-          { controller: table.target_controller, action: :destroy, device_id: device.id },
-          method: :delete,
-          id: "two_factor_delete_form",
-          data: helpers.password_confirmation_data_attribute({})
-        ) do
-          render(
-            Primer::Beta::IconButton.new(
-              scheme: :danger,
-              icon: :trash,
-              tag: :button,
-              size: :small,
-              type: :submit,
-              disabled: deletion_blocked?,
-              test_selector: "two-factor--delete-button",
-              "aria-label": if deletion_blocked?
-                              I18n.t("two_factor_authentication.devices.is_default_cannot_delete")
-                            else
-                              I18n.t(:button_delete)
-                            end
-            )
-          )
-        end
+      def make_default_action(menu)
+        menu.with_item(
+          label: t(:button_make_default),
+          tag: :button,
+          disabled: device.default,
+          href: helpers.url_for(controller: table.target_controller, action: :make_default, device_id: device.id),
+          form_arguments: {
+            method: :post,
+            id: "two_factor_make_default_form",
+            data: helpers.password_confirmation_data_attribute({})
+          },
+          test_selector: "two-factor--make-default-button",
+          "aria-label": t(:button_make_default)
+        )
+      end
+
+      def make_active_action(menu)
+        menu.with_item(
+          label: I18n.t(:button_make_active),
+          tag: :a,
+          disabled: device.active,
+          href: helpers.url_for(controller: table.target_controller, action: :confirm, device_id: device.id),
+          test_selector: "two-factor--make-active-button",
+          "aria-label": t("two_factor_authentication.devices.confirm_now")
+        )
+      end
+
+      def delete_action(menu)
+        menu.with_item(
+          label: t(:button_remove),
+          scheme: :danger,
+          tag: :button,
+          disabled: deletion_blocked?,
+          href: helpers.url_for(controller: table.target_controller, action: :destroy, device_id: device.id),
+          form_arguments: {
+            method: :delete,
+            id: "two_factor_delete_form",
+            data: helpers.password_confirmation_data_attribute({})
+          },
+          test_selector: "two-factor--delete-button",
+          "aria-label": if deletion_blocked?
+                          t("two_factor_authentication.devices.is_default_cannot_delete")
+                        else
+                          t(:button_delete)
+                        end
+        )
       end
 
       def deletion_blocked?

modules/two_factor_authentication/app/components/two_factor_authentication/devices/table_component.rb

@@ -4,8 +4,8 @@ module ::TwoFactorAuthentication
   module Devices
     class TableComponent < ::OpPrimer::BorderBoxTableComponent
       options :admin_table
-      columns :device_type, :default, :confirmed
-      mobile_labels :default, :confirmed
+      columns :device_type, :default, :active
+      mobile_labels :default, :active
 
       def mobile_title
         I18n.t("two_factor_authentication.label_devices")
@@ -49,7 +49,7 @@ module ::TwoFactorAuthentication
         [
           [:device_type, { caption: I18n.t("two_factor_authentication.label_device_type") }],
           [:default, { caption: I18n.t(:label_default) }],
-          [:confirmed, { caption: I18n.t(:label_confirmed) }]
+          [:active, { caption: I18n.t(:label_active) }]
         ]
       end
     end

modules/two_factor_authentication/app/components/two_factor_authentication/my/security_section_component.html.erb

@@ -2,58 +2,62 @@
       subhead.with_heading(tag: :h3, size: :medium) { t "two_factor_authentication.label_devices" }
     end %>
 
-<% if @has_remember_token_for_user %>
-  <%= render(
-        Primer::Alpha::Banner.new(
-          scheme: :default, dismiss_scheme: :none, mb: 3,
-          id: "two_factor_authentication_remember_cookie"
-        )
-      ) do |banner|
-        banner.with_action_button(
-          tag: :a,
-          href: helpers.my_2fa_remember_cookie_path,
-          test_selector: "two-factor-authentication--remove-remember-cookie-link",
-          data: { turbo_method: :delete }
-        ) do
-          t("two_factor_authentication.remember.clear_cookie")
+<section class="admin--edit-section">
+  <% if @has_remember_token_for_user %>
+    <%= render(
+          Primer::Alpha::Banner.new(
+            scheme: :default, dismiss_scheme: :none, mb: 3,
+            id: "two_factor_authentication_remember_cookie"
+          )
+        ) do |banner|
+          banner.with_action_button(
+            tag: :a,
+            href: helpers.my_2fa_remember_cookie_path,
+            test_selector: "two-factor-authentication--remove-remember-cookie-link",
+            data: { turbo_method: :delete }
+          ) do
+            t("two_factor_authentication.remember.clear_cookie")
+          end
+
+          if @remember_token
+            t(
+              "two_factor_authentication.remember.active_session_notice",
+              expires_on: helpers.format_date(@remember_token.expires_on)
+            )
+          else
+            t("two_factor_authentication.remember.other_active_session_notice")
+          end
+        end %>
+  <% end %>
+
+  <%= render partial: "two_factor_authentication/my/two_factor_devices/two_factor_status", locals: { device: @default_device } %>
+</section>
+
+<section class="admin--edit-section">
+  <%= render ::TwoFactorAuthentication::Devices::TableComponent.new(rows: @two_factor_devices, admin_table: false) %>
+
+  <%= render(Primer::Alpha::ActionMenu.new(anchor_align: :end)) do |menu|
+        menu.with_show_button(
+          test_selector: "two_factor_authentication_devices_button",
+          aria: { label: t("two_factor_authentication.label_device") },
+          mt: 3
+        ) do |button|
+          button.with_leading_visual_icon(icon: :plus)
+          button.with_trailing_visual_icon(icon: :"triangle-down")
+          t("two_factor_authentication.label_device")
         end
 
-        if @remember_token
-          t(
-            "two_factor_authentication.remember.active_session_notice",
-            expires_on: helpers.format_date(@remember_token.expires_on)
-          )
-        else
-          t("two_factor_authentication.remember.other_active_session_notice")
+        @available_devices.each_key do |key|
+          menu.with_item(
+            label: t("two_factor_authentication.devices.#{key}.title"),
+            href: helpers.new_my_2fa_device_path(type: key),
+            test_selector: "two_factor_authentication_devices_#{key}"
+          ) do |item|
+            item.with_description.with_content(t("two_factor_authentication.devices.#{key}.description"))
+          end
         end
       end %>
-<% end %>
-
-<%= render partial: "two_factor_authentication/my/two_factor_devices/two_factor_status", locals: { device: @default_device } %>
-
-<%= render ::TwoFactorAuthentication::Devices::TableComponent.new(rows: @two_factor_devices, admin_table: false) %>
-
-<%= render(Primer::Alpha::ActionMenu.new(anchor_align: :end)) do |menu|
-      menu.with_show_button(
-        test_selector: "two_factor_authentication_devices_button",
-        aria: { label: t("two_factor_authentication.label_device") },
-        mt: 3
-      ) do |button|
-        button.with_leading_visual_icon(icon: :plus)
-        button.with_trailing_visual_icon(icon: :"triangle-down")
-        t("two_factor_authentication.label_device")
-      end
-
-      @available_devices.each_key do |key|
-        menu.with_item(
-          label: t("two_factor_authentication.devices.#{key}.title"),
-          href: helpers.new_my_2fa_device_path(type: key),
-          test_selector: "two_factor_authentication_devices_#{key}"
-        ) do |item|
-          item.with_description.with_content(t("two_factor_authentication.devices.#{key}.description"))
-        end
-      end
-    end %>
+</section>
 
 <%= render(Primer::Beta::Subhead.new(mt: 5)) do |subhead|
       subhead.with_heading(tag: :h3, size: :medium) { t "two_factor_authentication.backup_codes.plural" }
@@ -74,7 +78,8 @@
       id: "two_factor_backup_codes_form",
       data: helpers.password_confirmation_data_attribute({})
     ) do %>
-  <%= render(Primer::Beta::Button.new(type: :submit)) do %>
+  <%= render(Primer::Beta::Button.new(type: :submit)) do |button| %>
+    <% button.with_leading_visual_icon(icon: :"op-server-key") %>
     <%= t("two_factor_authentication.backup_codes.generate.title") %>
   <% end %>
 <% end %>

modules/two_factor_authentication/app/views/two_factor_authentication/my/two_factor_devices/_two_factor_status.html.erb

@@ -4,6 +4,7 @@
         on_text: t("two_factor_authentication.label_2fa_enabled"),
         on_description: t("two_factor_authentication.text_2fa_enabled"),
         off_text: t("two_factor_authentication.label_2fa_disabled"),
-        off_description: t("two_factor_authentication.text_2fa_disabled")
+        off_description: t("two_factor_authentication.text_2fa_disabled"),
+        off_scheme: :warning
       }
     ) %>

modules/two_factor_authentication/app/views/users/_two_factor_authentication_admin.html.erb

@@ -8,7 +8,8 @@
           on_text: t("two_factor_authentication.label_2fa_enabled"),
           on_description: t("two_factor_authentication.admin.text_2fa_enabled"),
           off_text: t("two_factor_authentication.label_2fa_disabled"),
-          off_description: t("two_factor_authentication.admin.text_2fa_disabled")
+          off_description: t("two_factor_authentication.admin.text_2fa_disabled"),
+          off_scheme: :warning
         }
       ) %>
 </section>

modules/two_factor_authentication/config/locales/en.yml

@@ -176,6 +176,7 @@ en:
   notice_account_has_no_phone: "No cell phone number is associated with your account."
   label_confirmed: "Confirmed"
   button_continue: "Continue"
+  button_make_active: "Activate"
   button_make_default: "Mark as default"
   notice_phone_number_format: "Please enter the number in the following format: +XX XXXXXXXX."
   text_otp_not_receive: "Other verification methods"

modules/two_factor_authentication/spec/features/admin_delete_two_factor_device_spec.rb

@@ -28,6 +28,7 @@ RSpec.describe "Admin deleting another user's 2FA device", :js,
 
     expect(page).to have_css(".mobile-otp--two-factor-device-row", count: 1)
 
+    find_test_selector("two-factor--actions-button").click
     find_test_selector("two-factor--delete-button").click
     dialog.confirm_flow_with(admin_password)
 

modules/two_factor_authentication/spec/features/admin_edit_two_factor_devices_spec.rb

@@ -59,6 +59,7 @@ RSpec.describe "Admin 2FA management", :js, :selenium, with_settings: {
 
     SeleniumHubWaiter.wait
     # Delete the one
+    find_test_selector("two-factor--actions-button").click
     find_test_selector("two-factor--delete-button").click
     dialog.confirm_flow_with user_password, should_fail: false
 

modules/two_factor_authentication/spec/features/my_two_factor_devices_spec.rb

@@ -114,10 +114,17 @@ RSpec.describe "My Account 2FA configuration",
 
     # Make the second one the default
     # Confirm the password wrongly
+    within rows[1] do
+      page.find_test_selector("two-factor--actions-button").click
+    end
     page.find_test_selector("two-factor--make-default-button").click
     dialog.confirm_flow_with "wrong_password", should_fail: true
 
     # Confirm again
+    rows = page.all(".mobile-otp--two-factor-device-row")
+    within rows[1] do
+      page.find_test_selector("two-factor--actions-button").click
+    end
     page.find_test_selector("two-factor--make-default-button").click
     dialog.confirm_flow_with user_password, should_fail: false
 
@@ -132,6 +139,9 @@ RSpec.describe "My Account 2FA configuration",
     expect(device.default).to be_truthy
 
     # Delete the sms device
+    within rows[0] do
+      page.find_test_selector("two-factor--actions-button").click
+    end
     rows[0].find("[data-test-selector='two-factor--delete-button']").click
     dialog.confirm_flow_with user_password, should_fail: false
 
@@ -140,6 +150,10 @@ RSpec.describe "My Account 2FA configuration",
     expect(user.otp_devices.count).to eq 1
 
     # Delete the totp device
+    rows = page.all(".mobile-otp--two-factor-device-row")
+    within rows[0] do
+      page.find_test_selector("two-factor--actions-button").click
+    end
     page.find_test_selector("two-factor--delete-button").click
     dialog.confirm_flow_with user_password, should_fail: false
 

modules/two_factor_authentication/spec/features/password_change_spec.rb

@@ -39,7 +39,7 @@ RSpec.describe "Password change with OTP", :js, with_settings: {
       fill_in("password", with: user_password)
       fill_in("new_password", with: new_user_password)
       fill_in("new_password_confirmation", with: new_user_password)
-      click_link_or_button I18n.t(:button_save)
+      click_button I18n.t(:button_change_password)
     end
 
     if requires_otp

spec/controllers/my_controller_spec.rb

@@ -83,8 +83,7 @@ RSpec.describe MyController do
       end
 
       it "shows an error message" do
-        expect(response).to have_http_status :unprocessable_entity
-        assert_template "password"
+        expect(response).to redirect_to action: "security"
         expect(user.errors.attribute_names).to eq([:password_confirmation])
         expect(user.errors.map(&:message).flatten)
           .to contain_exactly("Password confirmation does not match password.")
@@ -92,7 +91,6 @@ RSpec.describe MyController do
     end
 
     describe "with wrong password" do
-      render_views
       before do
         @current_password = user.current_password.id
         post :change_password,
@@ -104,8 +102,7 @@ RSpec.describe MyController do
       end
 
       it "shows an error message" do
-        expect(response).to have_http_status :unprocessable_entity
-        assert_template "password"
+        expect(response).to redirect_to action: "security"
         expect(flash[:error]).to eq("Wrong password")
       end
 
@@ -151,7 +148,7 @@ RSpec.describe MyController do
         end
 
         it "blocks the attempt even with correct password" do
-          expect(response).to have_http_status :unprocessable_entity
+          expect(response).to redirect_to action: "security"
         end
 
         it "does not change the password" do

spec/features/auth/login_spec.rb

@@ -94,7 +94,7 @@ RSpec.describe "Login" do
             fill_in("password", with: user_password)
             fill_in("new_password", with: new_user_password)
             fill_in("new_password_confirmation", with: "#{new_user_password}typo")
-            click_link_or_button I18n.t(:button_save)
+            click_button I18n.t(:button_change_password)
           end
           expect(page).to have_current_path account_change_password_path
 
@@ -103,7 +103,7 @@ RSpec.describe "Login" do
             fill_in("password", with: user_password)
             fill_in("new_password", with: new_user_password)
             fill_in("new_password_confirmation", with: new_user_password)
-            click_link_or_button I18n.t(:button_save)
+            click_button I18n.t(:button_change_password)
           end
 
           # on the my page
@@ -128,7 +128,7 @@ RSpec.describe "Login" do
             fill_in("password", with: user_password)
             fill_in("new_password", with: new_user_password)
             fill_in("new_password_confirmation", with: new_user_password)
-            click_link_or_button I18n.t(:button_save)
+            click_button I18n.t(:button_change_password)
           end
 
           # on the my page
@@ -208,7 +208,7 @@ RSpec.describe "Login" do
         fill_in "New password", with: new_user_password
         fill_in "Confirmation", with: new_user_password
 
-        click_button "Save"
+        click_button "Change password"
 
         expect_being_logged_in(user)
 

spec/features/users/password_change_spec.rb

@@ -76,7 +76,7 @@ RSpec.describe "random password generation", :js do
       fill_in "password", with: old_password
       fill_in "new_password", with: new_password
       fill_in "new_password_confirmation", with: new_password
-      click_on "Save"
+      click_button "Change password"
 
       expect(page).to have_content "Invalid user or password"
 
@@ -91,7 +91,7 @@ RSpec.describe "random password generation", :js do
 
       expect(Sessions::UserSession.for_user(user.id).count).to be >= 1
 
-      click_on "Save"
+      click_button "Change password"
       wait_for_network_idle
       expect_flash(type: :info, message: I18n.t(:notice_account_password_updated))
 

spec/support/pages/my/password.rb

@@ -43,7 +43,7 @@ module Pages
         page.fill_in("new_password", with: new_password)
         page.fill_in("new_password_confirmation", with: confirmation)
 
-        page.click_link_or_button "Save"
+        page.click_button "Change password"
       end
 
       def expect_password_reuse_error_message(count)