From 53f4b28aae40e2fdd22ba5eea4ab3ce42fba67ba Mon Sep 17 00:00:00 2001 From: Madeline Collier Date: Mon, 7 Oct 2024 14:51:59 +0200 Subject: [PATCH] Add new users admin addresses page This migrates the `users/:id/addresses` page to the new admin. Instead of squashing everything into one controller action with an `if request.put?` wrapper (like the legacy backend controller did), I chose to break this new admin into two actions separated by their methods and responsibilities. The old admin page did not have anything in the way of validations or error messages, it would simply fail to update the address and leave it as is if the user provided invalid values. (While also showing a message saying that the update was successful?) This page improves upon that flow, adding validations to the address forms when their values are invalid and preventing the update, but since we are submitting an update against the user and not the address directly, we still need to do some extra work to run the validations and generate errors for the form (rather than silently failing to update like the legacy back-end controller used to do). --- .../users/addresses/component.html.erb | 56 +++++++++++++++ .../users/addresses/component.rb | 72 +++++++++++++++++++ .../users/addresses/component.yml | 18 +++++ .../solidus_admin/users/edit/component.rb | 15 ++-- .../solidus_admin/users_controller.rb | 49 ++++++++++++- admin/config/locales/users.en.yml | 6 ++ admin/config/routes.rb | 8 ++- admin/spec/features/users_spec.rb | 52 ++++++++++++++ .../spec/requests/solidus_admin/users_spec.rb | 61 +++++++++++++++- 9 files changed, 324 insertions(+), 13 deletions(-) create mode 100644 admin/app/components/solidus_admin/users/addresses/component.html.erb create mode 100644 admin/app/components/solidus_admin/users/addresses/component.rb create mode 100644 admin/app/components/solidus_admin/users/addresses/component.yml diff --git a/admin/app/components/solidus_admin/users/addresses/component.html.erb b/admin/app/components/solidus_admin/users/addresses/component.html.erb new file mode 100644 index 00000000000..b2d41f37e3c --- /dev/null +++ b/admin/app/components/solidus_admin/users/addresses/component.html.erb @@ -0,0 +1,56 @@ +<%= page do %> + <%= page_header do %> + <%= page_header_back(solidus_admin.users_path) %> + <%= page_header_title(t(".title", email: @user.email)) %> + + <%= page_header_actions do %> + <%= render component("ui/button").new(tag: :a, text: t(".create_order_for_user"), href: spree.new_admin_order_path(user_id: @user.id)) %> + <% end %> + <% end %> + + <%= page_header do %> + <% tabs.each do |tab| %> + <%= render(component("ui/button").new(tag: :a, scheme: :ghost, text: tab[:text], 'aria-current': tab[:current], href: tab[:href])) %> + <% end %> + <% end %> + + <%= page_with_sidebar do %> + <%= page_with_sidebar_main do %> + <%= render component('ui/panel').new(title: t(".billing_address")) do %> + <%= form_for @user, url: solidus_admin.update_addresses_user_path(@user), method: :put, html: { id: "#{form_id}_billing", autocomplete: "off", class: "bill_address" } do |f| %> + + <%= render component('ui/forms/address').new(address: @bill_address, name: "user[bill_address_attributes]") %> +
+ <%= render component("ui/button").new(tag: :button, text: t(".update"), form: "#{form_id}_billing") %> + <%= render component("ui/button").new(tag: :a, text: t(".cancel"), href: solidus_admin.addresses_user_path(@user), scheme: :secondary) %> +
+ <% end %> + <% end %> + + <%= render component('ui/panel').new(title: t(".shipping_address")) do %> + <%= form_for @user, url: solidus_admin.update_addresses_user_path(@user), method: :put, html: { id: "#{form_id}_shipping", autocomplete: "off", class: "ship_address" } do |f| %> + + <%= render component('ui/forms/address').new(address: @ship_address, name: "user[ship_address_attributes]") %> +
+ <%= render component("ui/button").new(tag: :button, text: t(".update"), form: "#{form_id}_shipping") %> + <%= render component("ui/button").new(tag: :a, text: t(".cancel"), href: solidus_admin.addresses_user_path(@user), scheme: :secondary) %> +
+ <% end %> + <% end %> + <% end %> + + <%= page_with_sidebar_aside do %> + <%= render component("ui/panel").new(title: t("spree.lifetime_stats")) do %> + <%= render component("ui/details_list").new( + items: [ + { label: t("spree.total_sales"), value: @user.display_lifetime_value.to_html }, + { label: t("spree.order_count"), value: @user.order_count.to_i }, + { label: t("spree.average_order_value"), value: @user.display_average_order_value.to_html }, + { label: t("spree.member_since"), value: @user.created_at.to_date }, + { label: t(".last_active"), value: last_login(@user) }, + ] + ) %> + <% end %> + <% end %> + <% end %> +<% end %> diff --git a/admin/app/components/solidus_admin/users/addresses/component.rb b/admin/app/components/solidus_admin/users/addresses/component.rb new file mode 100644 index 00000000000..bc7665b3cc6 --- /dev/null +++ b/admin/app/components/solidus_admin/users/addresses/component.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +class SolidusAdmin::Users::Addresses::Component < SolidusAdmin::BaseComponent + include SolidusAdmin::Layout::PageHelpers + + def initialize(user:, address: nil, type: nil) + @user = user + @bill_address = bill_address(address, type) + @ship_address = ship_address(address, type) + end + + def form_id + @form_id ||= "#{stimulus_id}--form-#{@user.id}" + end + + def tabs + [ + { + text: t('.account'), + href: solidus_admin.user_path(@user), + current: false, + }, + { + text: t('.addresses'), + href: solidus_admin.addresses_user_path(@user), + current: true, + }, + { + text: t('.order_history'), + href: spree.orders_admin_user_path(@user), + current: false, + }, + { + text: t('.items'), + href: spree.items_admin_user_path(@user), + current: false, + }, + { + text: t('.store_credit'), + href: spree.admin_user_store_credits_path(@user), + current: false, + }, + ] + end + + def last_login(user) + return t('.last_login.never') if user.try(:last_sign_in_at).blank? + + t( + '.last_login.login_time_ago', + # @note The second `.try` is here for the specs and for setups that use a + # custom User class which may not have this attribute. + last_login_time: time_ago_in_words(user.try(:last_sign_in_at)) + ).capitalize + end + + def bill_address(address, type) + if address.present? && type == "bill" + address + else + @user.bill_address || Spree::Address.build_default + end + end + + def ship_address(address, type) + if address.present? && type == "ship" + address + else + @user.ship_address || Spree::Address.build_default + end + end +end diff --git a/admin/app/components/solidus_admin/users/addresses/component.yml b/admin/app/components/solidus_admin/users/addresses/component.yml new file mode 100644 index 00000000000..6872c1d5cd5 --- /dev/null +++ b/admin/app/components/solidus_admin/users/addresses/component.yml @@ -0,0 +1,18 @@ +en: + title: "Users / %{email} / Addresses" + account: Account + addresses: Addresses + order_history: Order History + items: Items + store_credit: Store Credit + last_active: Last Active + last_login: + login_time_ago: "%{last_login_time} ago" + never: Never + invitation_sent: Invitation sent + create_order_for_user: Create order for this user + update: Update + cancel: Cancel + back: Back + billing_address: Billing Address + shipping_address: Shipping Address diff --git a/admin/app/components/solidus_admin/users/edit/component.rb b/admin/app/components/solidus_admin/users/edit/component.rb index 78f1b8a82eb..1263af87cfd 100644 --- a/admin/app/components/solidus_admin/users/edit/component.rb +++ b/admin/app/components/solidus_admin/users/edit/component.rb @@ -15,32 +15,31 @@ def tabs [ { text: t('.account'), - href: solidus_admin.users_path, - current: action_name == "show", + href: solidus_admin.user_path(@user), + current: action_name == "edit", }, { text: t('.addresses'), - href: spree.addresses_admin_user_path(@user), - # @todo: update this "current" logic once folded into new admin - current: action_name != "show", + href: solidus_admin.addresses_user_path(@user), + current: action_name == "addresses", }, { text: t('.order_history'), href: spree.orders_admin_user_path(@user), # @todo: update this "current" logic once folded into new admin - current: action_name != "show", + current: action_name != "edit", }, { text: t('.items'), href: spree.items_admin_user_path(@user), # @todo: update this "current" logic once folded into new admin - current: action_name != "show", + current: action_name != "edit", }, { text: t('.store_credit'), href: spree.admin_user_store_credits_path(@user), # @todo: update this "current" logic once folded into new admin - current: action_name != "show", + current: action_name != "edit", }, ] end diff --git a/admin/app/controllers/solidus_admin/users_controller.rb b/admin/app/controllers/solidus_admin/users_controller.rb index 5a679a4d2e1..59ac12d44c2 100644 --- a/admin/app/controllers/solidus_admin/users_controller.rb +++ b/admin/app/controllers/solidus_admin/users_controller.rb @@ -3,6 +3,9 @@ module SolidusAdmin class UsersController < SolidusAdmin::BaseController include SolidusAdmin::ControllerHelpers::Search + include Spree::Core::ControllerHelpers::StrongParameters + + before_action :set_user, only: [:edit, :addresses, :update_addresses] search_scope(:all, default: true) search_scope(:customers) { _1.left_outer_joins(:role_users).where(role_users: { id: nil }) } @@ -23,9 +26,31 @@ def index end end - def edit - set_user + def addresses + respond_to do |format| + format.turbo_stream { render turbo_stream: '' } + format.html { render component('users/addresses').new(user: @user) } + end + end + + def update_addresses + set_address_from_params + if @address.valid? && @user.update(user_params) + flash[:success] = t(".#{@type}.success") + + respond_to do |format| + format.turbo_stream { render turbo_stream: '' } + format.html { render component('users/addresses').new(user: @user) } + end + else + respond_to do |format| + format.html { render component('users/addresses').new(user: @user, address: @address, type: @type), status: :unprocessable_entity } + end + end + end + + def edit respond_to do |format| format.html { render component('users/edit').new(user: @user) } end @@ -47,7 +72,25 @@ def set_user end def user_params - params.require(:user).permit(:user_id, permitted_user_attributes) + params.require(:user).permit( + :user_id, + permitted_user_attributes, + bill_address_attributes: permitted_address_attributes, + ship_address_attributes: permitted_address_attributes + ) + end + + # @note This method is used to generate validation errors on the address. + # Since the update is being performed via the @user, and not directly on + # the @address, we sadly don't seem to get these errors automatically. + def set_address_from_params + if user_params.key?(:bill_address_attributes) + @address = Spree::Address.new(user_params[:bill_address_attributes]) + @type = "bill" + elsif user_params.key?(:ship_address_attributes) + @address = Spree::Address.new(user_params[:ship_address_attributes]) + @type = "ship" + end end def authorization_subject diff --git a/admin/config/locales/users.en.yml b/admin/config/locales/users.en.yml index c15e9fb0faa..5fda686929c 100644 --- a/admin/config/locales/users.en.yml +++ b/admin/config/locales/users.en.yml @@ -4,3 +4,9 @@ en: title: "Users" destroy: success: "Users were successfully removed." + update_addresses: + bill: + success: "Billing Address has been successfully updated." + ship: + success: "Shipping Address has been successfully updated." + error: "Address could not be updated." diff --git a/admin/config/routes.rb b/admin/config/routes.rb index afbb4be37c5..ac4693d32d5 100644 --- a/admin/config/routes.rb +++ b/admin/config/routes.rb @@ -45,7 +45,13 @@ end end - admin_resources :users, only: [:index, :edit, :destroy] + admin_resources :users, only: [:index, :edit, :destroy] do + member do + get :addresses + put :update_addresses + end + end + admin_resources :promotions, only: [:index, :destroy] admin_resources :properties, only: [:index, :destroy] admin_resources :option_types, only: [:index, :destroy], sortable: true diff --git a/admin/spec/features/users_spec.rb b/admin/spec/features/users_spec.rb index 36cd9eba915..5884e3bcc25 100644 --- a/admin/spec/features/users_spec.rb +++ b/admin/spec/features/users_spec.rb @@ -109,4 +109,56 @@ expect(page).not_to have_content("newemail@example.com") end end + + context "when editing a user's addresses" do + before do + create(:user_with_addresses, email: "customer@example.com") + visit "/admin/users" + find_row("customer@example.com").click + click_on "Addresses" + end + + it "shows the address page" do + expect(page).to have_content("Users / customer@example.com / Addresses") + expect(page).to have_content("Lifetime Stats") + expect(page).to have_content("Billing Address") + expect(page).to be_axe_clean + end + + it "allows editing of the existing address" do + # Invalid submission + within("form.ship_address") do + fill_in "Name", with: "" + fill_in "Street Address", with: "" + click_on "Update" + end + expect(page).to have_content("can't be blank").twice + + # Valid submission + within("form.bill_address") do + fill_in "Name", with: "Galadriel" + click_on "Update" + end + expect(page).to have_content("Billing Address has been successfully updated.") + + # Valid submission + within("form.ship_address") do + fill_in "Name", with: "Elrond" + click_on "Update" + end + expect(page).to have_content("Shipping Address has been successfully updated.") + + # Cancel submission + within("form.bill_address") do + fill_in "Name", with: "Smeagol" + click_on "Cancel" + end + expect(page).to have_content("Users / customer@example.com / Addresses") + expect(page).not_to have_content("Smeagol") + + # The address forms weirdly only have values rather than actual text on the page. + expect(page).to have_field("user[bill_address_attributes][name]", with: "Galadriel") + expect(page).to have_field("user[ship_address_attributes][name]", with: "Elrond") + end + end end diff --git a/admin/spec/requests/solidus_admin/users_spec.rb b/admin/spec/requests/solidus_admin/users_spec.rb index 59df8ac3c6c..36847fd639f 100644 --- a/admin/spec/requests/solidus_admin/users_spec.rb +++ b/admin/spec/requests/solidus_admin/users_spec.rb @@ -4,7 +4,42 @@ RSpec.describe "SolidusAdmin::UsersController", type: :request do let(:admin_user) { create(:admin_user) } - let(:user) { create(:user) } + let(:user) { create(:user_with_addresses) } + let(:address) { create(:address) } + + let(:valid_address_params) do + { + user: { + bill_address_attributes: { + name: address.name, + address1: address.address1, + address2: address.address2, + city: address.city, + zipcode: address.zipcode, + state_id: address.state_id, + country_id: address.country_id, + phone: address.phone + } + } + } + end + + # Invalid due to missing "name" field. + let(:invalid_address_params) do + { + user: { + bill_address_attributes: { + address1: address.address1, + address2: address.address2, + city: address.city, + zipcode: address.zipcode, + state_id: address.state_id, + country_id: address.country_id, + phone: address.phone + } + } + } + end before do allow_any_instance_of(SolidusAdmin::BaseController).to receive(:spree_current_user).and_return(admin_user) @@ -44,6 +79,30 @@ end end + describe "GET /addresses" do + it "renders the addresses template with a 200 OK status" do + get solidus_admin.addresses_user_path(user) + expect(response).to have_http_status(:ok) + end + end + + describe "PUT /update_addresses" do + context "with valid address parameters" do + it "updates the user's address and redirects with a success message" do + put solidus_admin.update_addresses_user_path(user), params: valid_address_params + expect(response).to have_http_status(:ok) + expect(response.body).to include("Address has been successfully updated.") + end + end + + context "with invalid address parameters" do + it "does not update the user's address and renders the addresses component with errors" do + put solidus_admin.update_addresses_user_path(user), params: invalid_address_params + expect(response).to have_http_status(:unprocessable_entity) + end + end + end + describe "search functionality" do before do create(:user, email: "test@example.com")