#4368 Admin panel - users - edit exist user - update address field always return 200 #4431

dborovsky · 2022-06-20T05:58:10Z

Issue: #4368

jarednorman · 2022-06-20T15:44:44Z

backend/app/controllers/spree/admin/users_controller.rb

+        if(Address.new(bill_address_params).valid? &&
+          Address.new(ship_address_params).valid?)


I don't think that this approach makes sense given that the attributes are immutably merged with the existing address. I'm pretty sure that's how that works, anyway.

With this approach we do not have error messages on the addresses that we could display.

I am puzzling with

@user.update(user_params) && @user.bill_address.valid? && @user.ship_address.valid?

but it does not seem to work either.

We use nested_attributes for users ship_address and bill_address, but since Spree::Address is read-only it silently (thanks rails) rejects. The address is always valid, because it never updates. We need a way to use Spree::Address.immutable_merge on an existing user.x_address instances somehow.

I also believe we should add a Admin::Users::AddressesController to directly work on the address records in stead of proxying through the user.

Admin::Users::AddressesController

Yes, I think better update address by callling new AddressesController.

waiting-for-dev · 2022-06-21T03:46:09Z

Thanks for your collaboration, @dborovsky!

I think it'd be good to start with a failing test to ensure that the work done is fixing it. Have you been able to isolate what caused the response to be a success even when the attributes weren't saved?

Please, let me know if you need anything.

tvdeyen · 2022-06-23T07:37:19Z

@dborovsky Thanks for the contribution, but I think the issue is in how addresses work in Solidus (they are immutable). The solution is a bit more involved. Do you still want to try working on it? I left some comments about what I currently think could solve the issue.

…ontroler

dborovsky · 2022-07-06T13:21:58Z

Hi @tvdeyen @jarednorman @waiting-for-dev. Review please my last changes.

backend/app/controllers/spree/admin/addresses_controller.rb

waiting-for-dev · 2022-08-30T10:02:09Z

backend/app/controllers/spree/admin/addresses_controller.rb

+        end
+      end
+
+      def model_class


Sorry, why do we need it? I don't see it used anywhere else, right?

waiting-for-dev · 2022-08-30T10:03:32Z

backend/app/controllers/spree/admin/addresses_controller.rb

+          @user.bill_address = new_bill_address
+          @user.ship_address = new_shipping_address


Did you check that it's actually updating the user and not only assigning it in memory?

yes, new values will be saved for user. I tested. I reloaded the page and addresses have already been displayed with new values.

Can we make the same check for user_id here as well and only update both user addresses if it is present? Then update the one record (passed as id), to be inline with the rest of the resource controllers and be RESTful in those cases?

@tvdeyen sorry, I don't fully understand what you mean?

Then update the one record (passed as id), to be inline with the rest of the resource controllers and be RESTful in those cases?

waiting-for-dev · 2022-08-30T10:22:32Z

backend/app/controllers/spree/admin/users_controller.rb

@@ -53,16 +53,6 @@ def update
        end
      end

-      def addresses


I'm afraid we need to deprecate the endpoint before removing it straight away, as some users might have overridden it. You take a look at how we did something similar here: https://github.com/solidusio/solidus/pull/4388/files.

What do you think, @kennyadsl? Or can we remove it now?

Yes, people may still have this path in their customized views. We should deprecate this instead (and not remove the route),

@waiting-for-dev @kennyadsl I added deprecation warning only to put endpoint addresses.

New get endpoint path addresses is the same as the old one

waiting-for-dev · 2022-08-30T10:22:48Z

backend/app/views/spree/admin/addresses/_addresses_form.html.erb

@@ -4,7 +4,7 @@
    <fieldset class="no-border-bottom">
      <legend align="center"><%= t('spree.billing_address') %></legend>
      <%= f.fields_for :bill_address, @user.bill_address || Spree::Address.build_default do |ba_form| %>
-        <%= render partial: 'spree/admin/shared/address_form', locals: { f: ba_form, type: "billing" } %>
+      <%= render partial: 'spree/admin/shared/address_form', locals: { f: ba_form, type: "billing" } %>


Suggested change

<%= render partial: 'spree/admin/shared/address_form', locals: { f: ba_form, type: "billing" } %>

<%= render partial: 'spree/admin/shared/address_form', locals: { f: ba_form, type: "billing" } %>

waiting-for-dev · 2022-08-30T10:23:00Z

backend/app/views/spree/admin/addresses/_addresses_form.html.erb

@@ -13,7 +13,7 @@
    <fieldset class="no-border-bottom">
      <legend align="center"><%= t('spree.shipping_address') %></legend>
      <%= f.fields_for :ship_address, @user.ship_address || Spree::Address.build_default do |sa_form| %>
-        <%= render partial: 'spree/admin/shared/address_form', locals: { f: sa_form, type: "shipping" } %>
+      <%= render partial: 'spree/admin/shared/address_form', locals: { f: sa_form, type: "shipping" } %>


Suggested change

<%= render partial: 'spree/admin/shared/address_form', locals: { f: sa_form, type: "shipping" } %>

<%= render partial: 'spree/admin/shared/address_form', locals: { f: sa_form, type: "shipping" } %>

waiting-for-dev · 2022-08-30T10:23:55Z

backend/app/views/spree/admin/addresses/addresses.html.erb

-          <%= render partial: 'spree/admin/shared/edit_resource_links', locals: { collection_url: admin_users_url } %>
-        </div>
-      <% end %>
+    <% if can?(:update, @user) %>


We also need to restore the indentation here.

waiting-for-dev

Sorry for taking so long before looking into it, @dborovsky 🙏 It's looking good. I left some preliminary comments. Let me know what you think.

kennyadsl

Thanks @dborovsky! I left some other comments related to backward compatibility and routes shape, but overall this is a good job. I think we are close to merging, thanks again!

kennyadsl · 2022-09-05T06:28:23Z

backend/config/routes.rb

@@ -168,9 +168,12 @@
      member do
        get :orders
        get :items
-        get :addresses


I still have a concern related to backward compatibility related to these routes changes: am I right that the route helper method addresses_admin_user_path is no more available (it's called admin_user_addresses_path now)? If yes, I think we should keep it somehow.

kennyadsl · 2022-09-05T06:30:52Z

backend/config/routes.rb

        put :addresses
      end
+
+      get 'addresses', action: :show, controller: 'addresses', as: :addresses
+      put 'addresses/update', action: :update, controller: 'addresses', as: :addresses_update


As mentioned here, it's probably fine to skip the _update part in the as: argument. The PUT verb is already telling routes which action to take. Can we try without it please?

kennyadsl · 2022-09-05T06:31:25Z

backend/spec/controllers/spree/admin/addresses_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'pry'


Can you please remove this require?

tvdeyen

This is a great step in the right direction. Thanks for addressing this (pun indented)

I would like this to be more RESTful and check for the existence of a @user for the edit and update actions. In all other cases it should handle single addresses.

BTW. this is a great step into finally having a Admin -> User -> Addressbook feature 👏🏻

tvdeyen · 2022-09-05T19:54:18Z

backend/config/routes.rb

+      get 'addresses', action: :show, controller: 'addresses', as: :addresses
+      put 'addresses/update', action: :update, controller: 'addresses', as: :addresses_update


The /update part in the url is also not necessary. Rails will be able to resolve the route from the HTTP verb and presence of an id. Also we can use the to keyword to combine controller and action into

Suggested change

get 'addresses', action: :show, controller: 'addresses', as: :addresses

put 'addresses/update', action: :update, controller: 'addresses', as: :addresses_update

get :addresses, to: 'addresses#show', as: :addresses

put :addresses, to: 'addresses#update, as: :addresses

Actually

Suggested change

get 'addresses', action: :show, controller: 'addresses', as: :addresses

put 'addresses/update', action: :update, controller: 'addresses', as: :addresses_update

get :addresses, to: 'addresses#edit', as: :addresses

put :addresses, to: 'addresses#update, as: :addresses

backend/app/controllers/spree/admin/addresses_controller.rb

tvdeyen · 2022-09-05T19:58:40Z

backend/app/controllers/spree/admin/addresses_controller.rb

+module Spree
+  module Admin
+    class AddressesController < Spree::Admin::BaseController
+      before_action :initial_user


Can we rename this to load_user please?

tvdeyen · 2022-09-05T20:00:17Z

backend/app/controllers/spree/admin/addresses_controller.rb

+          @user.bill_address = new_bill_address
+          @user.ship_address = new_shipping_address


Can we make the same check for user_id here as well and only update both user addresses if it is present? Then update the one record (passed as id), to be inline with the rest of the resource controllers and be RESTful in those cases?

tvdeyen · 2022-09-05T20:00:27Z

backend/app/controllers/spree/admin/addresses_controller.rb

+
+      private
+
+      def initial_user


Suggested change

def initial_user

def load_user

fthobe · 2025-02-08T16:55:05Z

@jarednorman old admin closed won't fix

solidusio#4368 implemented logic of checking if address is valid

39987b5

jarednorman requested changes Jun 20, 2022

View reviewed changes

dborovsky added 7 commits June 29, 2022 17:21

solidusio#4368 rewrited logic imutable_merge, implemented addresses c…

e19d2d8

…ontroler

solidusio#4368 rewrited logic imutable_merge

b45c224

solidusio#4368 separated action update addresses

dffe203

solidusio#4368 separated action update addresses

e4ff962

solidusio#4368 formated error message

6701f20

solidusio#4368 acceptance test - edit addresses

75ba9b2

solidusio#4368 callback initial user

1bd8b34

dborovsky requested review from tvdeyen and jarednorman July 4, 2022 08:15

dborovsky added 2 commits July 4, 2022 10:29

solidusio#4368 route put addresses/update

de248db

solidusio#4368 using redirect to if addresses were update

66b8967

dborovsky added 3 commits July 7, 2022 09:49

solidusio#4368 test get show addresses, refactored test put update

09e7c46

solidusio#4368 fixed test

a4fdaeb

solidusio#4368 fixed test, removed checking of content type

fd1fc50

kennyadsl added type:bug Error, flaw or fault changelog:solidus_backend Changes to the solidus_backend gem labels Aug 22, 2022

waiting-for-dev reviewed Aug 30, 2022

View reviewed changes

backend/app/controllers/spree/admin/addresses_controller.rb Outdated Show resolved Hide resolved

waiting-for-dev reviewed Aug 30, 2022

View reviewed changes

waiting-for-dev suggested changes Aug 30, 2022

View reviewed changes

github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Sep 4, 2022

solidusio#4368 refactoring, deprecation warn old endpoint

f4b7e8b

kennyadsl requested changes Sep 5, 2022

View reviewed changes

tvdeyen reviewed Sep 5, 2022

View reviewed changes

solidusio#4368 refactoring tests, routing

2542fab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4368 Admin panel - users - edit exist user - update address field always return 200 #4431

#4368 Admin panel - users - edit exist user - update address field always return 200 #4431

dborovsky commented Jun 20, 2022

jarednorman Jun 20, 2022

tvdeyen Jun 23, 2022

tvdeyen Jun 23, 2022 •

edited

Loading

tvdeyen Jun 23, 2022

dborovsky Jun 23, 2022

waiting-for-dev commented Jun 21, 2022

tvdeyen commented Jun 23, 2022

dborovsky commented Jul 6, 2022

waiting-for-dev Aug 30, 2022

waiting-for-dev Aug 30, 2022

dborovsky Aug 31, 2022

tvdeyen Sep 5, 2022

dborovsky Oct 4, 2022

waiting-for-dev Aug 30, 2022 •

edited

Loading

kennyadsl Aug 30, 2022

dborovsky Sep 4, 2022

waiting-for-dev Aug 30, 2022

waiting-for-dev Aug 30, 2022

waiting-for-dev Aug 30, 2022

waiting-for-dev left a comment

kennyadsl left a comment

kennyadsl Sep 5, 2022

kennyadsl Sep 5, 2022

kennyadsl Sep 5, 2022

tvdeyen left a comment

tvdeyen Sep 5, 2022

tvdeyen Sep 5, 2022

tvdeyen Sep 5, 2022

tvdeyen Sep 5, 2022

tvdeyen Sep 5, 2022

fthobe commented Feb 8, 2025

		if(Address.new(bill_address_params).valid? &&
		Address.new(ship_address_params).valid?)

		@user.bill_address = new_bill_address
		@user.ship_address = new_shipping_address

	<%= render partial: 'spree/admin/shared/address_form', locals: { f: ba_form, type: "billing" } %>
	<%= render partial: 'spree/admin/shared/address_form', locals: { f: ba_form, type: "billing" } %>

		get 'addresses', action: :show, controller: 'addresses', as: :addresses
		put 'addresses/update', action: :update, controller: 'addresses', as: :addresses_update

#4368 Admin panel - users - edit exist user - update address field always return 200 #4431

Are you sure you want to change the base?

#4368 Admin panel - users - edit exist user - update address field always return 200 #4431

Conversation

dborovsky commented Jun 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waiting-for-dev commented Jun 21, 2022

tvdeyen commented Jun 23, 2022

dborovsky commented Jul 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waiting-for-dev Aug 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waiting-for-dev left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fthobe commented Feb 8, 2025

tvdeyen Jun 23, 2022 •

edited

Loading

waiting-for-dev Aug 30, 2022 •

edited

Loading