shipping fees on admin: avoid backend error when updating calculator type

[jibees]

What? Why?

Ensure that form element are well disabled to not submit them, as they're not handled by the backend

This solution is not perfect (I did not change the UX behavior which can be improve a lot, I didn't remove any angular code, ...) but this seems to be straightforward and should probably improve the UX experience.

Closes #6300

What should we test?

Well described in the original issue:

On both /admin/shipping_methods/9/edit and /admin/enterprise_fees

• Set a weight-based fee, on an existing shipping method, choosing kg or lb and attributing a value
• Press Update
• Select another Calculator, for example Flat Percent
• Press Update: this should be ok!

Release notes

Changelog Category: User facing changes

The title of the pull request will be included in the release notes.

[mkllnk]

Nice.

[mkllnk]

A test would have been nice. 😜

I think that every bug deserves a spec. It helps while developing and makes sure that the same bug doesn't happen again. I find this often more important than trying to cover new features with all possible specs. You can't predict bugs but you can make sure that you don't do the same mistake twice.

[jibees]

A test would have been nice. 😜

I thought about it. My concern was about creating a test for a very specific bug, pretty old actually, that will increase the time consumed by each continuous integration. For sure creating test will avoid to us to do the same mistake. But in this particular case, I thought it was unnecessary.

Will do anyway.

[mkllnk]

Thank you!

[jibees]

This one is 1 month old, has been reviewed by @mkllnk --> moving to Test Ready column.
If you feel that this should not happened, feel free to move it back to Code Review column.

[filipefurtad0]

I think that every bug deserves a spec

Ideally, we would not have manual testing stage at all - right?
Proposal: PRs which are well covered by tests should not be manually tested. This could potentially speed up delivery.

But I guess slowing down the build is a side effect, yeah. But still desired trade-off, right? What do you think? Maybe something for delivery circle :-)

[filipefurtad0]

Hey @jibees ,

Somehow this one is not quite working just yet, I still get the error 500 when switching from a weight-based calculator to any other calculator:

curl 'https://staging.openfoodnetwork.org.uk/admin/shipping_methods/887' -X POST -H 'User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:99.0) Gecko/20100101 Firefox/99.0' -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8' -H 'Accept-Language: en-US,en;q=0.5' -H 'Accept-Encoding: gzip, deflate, br' -H 'Referer: https://staging.openfoodnetwork.org.uk/admin/shipping_methods/887/edit' -H 'Content-Type: application/x-www-form-urlencoded' -H 'Origin: https://staging.openfoodnetwork.org.uk' -H 'DNT: 1' -H 'Connection: keep-alive' -H 'Cookie: XSRF-TOKEN=3H18UvCFo8LpYXWSIaYKbv9H4zbSZOCxfJ-6mzjzCWDCv4Y9q-wC_JISHNImwYx0rs0HiAWFwadKH4FKrSrHhg; _ofn_session_id=21e9d8699ba439acb315d13f1ac4e657; _pk_id.5.cabc=b45342d0d8661acf.1650189521.; __stripe_mid=b4843a16-7233-4e9d-8e88-8fa88e8e0e0d6a0af0; cookies_consent=cookies_consent; _pk_id.2.dae1=ff104d0b58364fce.1650189529.; __stripe_mid=b4843a16-7233-4e9d-8e88-8fa88e8e0e0d6a0af0; _pk_ses.5.cabc=1; __stripe_sid=a204435f-198c-4ae8-b953-34b91ee802e73d826e' -H 'Upgrade-Insecure-Requests: 1' -H 'Sec-Fetch-Dest: document' -H 'Sec-Fetch-Mode: navigate' -H 'Sec-Fetch-Site: same-origin' -H 'Sec-Fetch-User: ?1' -H 'TE: trailers' --data-raw '_method=patch&authenticity_token=X5Evh7Y15HrfSYtVa2wKhW3nXZTf6YZ2rFWw_tw-0JtBU9Xo7VxFRKQ64hVsC4yfPG25KggIp2Ca1YsvSecefQ&shipping_method%5Bname%5D=back+and...+forth+with+the+bike&shipping_method%5Bdescription%5D=back+and...forth+with+the+bike&shipping_method%5Bdisplay_on%5D=&shipping_method%5Brequire_ship_address%5D=true&shipping_method%5Btag_list%5D=&shipping_method%5Bcalculator_type%5D=Calculator%3A%3AFlatRate&shipping_method%5Bcalculator_attributes%5D%5Bpreferred_unit_from_list%5D=kg&shipping_method%5Btax_category_id%5D=1&shipping_method%5Bshipping_categories%5D%5B%5D=2&shipping_method%5Bshipping_categories%5D%5B%5D=3&shipping_method%5Bzones%5D%5B%5D=15&shipping_method%5Bzones%5D%5B%5D=5&shipping_method%5Bzones%5D%5B%5D=6&shipping_method%5Bzones%5D%5B%5D=7&shipping_method%5Bzones%5D%5B%5D=8&shipping_method%5Bzones%5D%5B%5D=9&shipping_method%5Bzones%5D%5B%5D=10&shipping_method%5Bzones%5D%5B%5D=11&shipping_method%5Bzones%5D%5B%5D=13&shipping_method%5Bzones%5D%5B%5D=14&shipping_method%5Bzones%5D%5B%5D=2&shipping_method%5Bzones%5D%5B%5D=18&shipping_method%5Bzones%5D%5B%5D=12&shipping_method%5Bzones%5D%5B%5D=19&shipping_method%5Bzones%5D%5B%5D=3&shipping_method%5Bzones%5D%5B%5D=4&shipping_method%5Bdistributor_ids%5D%5B%5D=&shipping_method%5Bdistributor_ids%5D%5B%5D=1799&button='

Also, I've looked at the server logs, the error might be triggered by:

ActiveModel::UnknownAttributeError (unknown attribute 'preferred_unit_from_list' for Spree::Calculator.):

Here is the full log:

openfoodnetwork@staging:~/apps/openfoodnetwork/current/log$ tail -f staging.log
I, [2022-04-18 11:21:05#29812]  INFO -- : Processing by Spree::Admin::ShippingMethodsController#update as HTML
I, [2022-04-18 11:21:05#29812]  INFO -- :   Parameters: {"authenticity_token"=>"KfDEPqD8NJXrgNbgQ3zvMQEfYYlzaBtdbZKSjiyCBMh3HMODPIAg0XG2AJhaIwZjlacxUV0znrHXwtf_MFyYZw", "shipping_method"=>{"name"=>"back and... forth with the bike", "description"=>"back and...forth with the bike", "display_on"=>"", "require_ship_address"=>"true", "tag_list"=>"", "calculator_type"=>"Calculator::FlatPercentItemTotal", "calculator_attributes"=>{"preferred_unit_from_list"=>"kg"}, "tax_category_id"=>"1", "shipping_categories"=>["2", "3"], "zones"=>["15", "5", "6", "7", "8", "9", "10", "11", "13", "14", "2", "18", "12", "19", "3", "4"], "distributor_ids"=>["", "1799"]}, "button"=>"", "id"=>"887"}
I, [2022-04-18 11:21:05#29812]  INFO -- : Completed 500 Internal Server Error in 86ms (ActiveRecord: 37.5ms | Allocations: 19337)
I, [2022-04-18 11:21:05#29812]  INFO -- [Bugsnag]: Notifying https://notify.bugsnag.com of ActiveModel::UnknownAttributeError
F, [2022-04-18 11:21:05#29812] FATAL -- :   
ActiveModel::UnknownAttributeError (unknown attribute 'preferred_unit_from_list' for Spree::Calculator.):
  
app/controllers/admin/resource_controller.rb:28:in `update'
lib/session_cookie_upgrader.rb:19:in `call'
I, [2022-04-18 11:21:05#29812]  INFO -- : source=rack-timeout id=6b34f3ca-eb0e-4d1c-bc76-9978116fcec9 wait=3ms timeout=120000ms service=140ms state=completed
I, [2022-04-18 11:23:15#29812]  INFO -- : source=rack-timeout id=4ca105bd-dbd1-4c3d-8c30-426adc9960d6 wait=7ms timeout=120000ms state=ready
I, [2022-04-18 11:23:15#29812]  INFO -- : Started PATCH "/admin/shipping_methods/887" for 2.82.201.180 at 2022-04-18 11:23:15 +0000
I, [2022-04-18 11:23:15#29812]  INFO -- : Processing by Spree::Admin::ShippingMethodsController#update as HTML
I, [2022-04-18 11:23:15#29812]  INFO -- :   Parameters: {"authenticity_token"=>"KFadMbPXQalnC4xc554s-trwpeecTOj3CBbukrP2yRE2lGde6L7glxx45Rzg-argi3pBWUutyeE-ltVDJi8H9w", "shipping_method"=>{"name"=>"back and... forth with the bike", "description"=>"back and...forth with the bike", "display_on"=>"", "require_ship_address"=>"true", "tag_list"=>"", "calculator_type"=>"Calculator::FlatPercentItemTotal", "calculator_attributes"=>{"preferred_unit_from_list"=>"kg"}, "tax_category_id"=>"1", "shipping_categories"=>["2", "3"], "zones"=>["15", "5", "6", "7", "8", "9", "10", "11", "13", "14", "2", "18", "12", "19", "3", "4"], "distributor_ids"=>["", "1799"]}, "button"=>"", "id"=>"887"}
I, [2022-04-18 11:23:15#29812]  INFO -- : Completed 500 Internal Server Error in 89ms (ActiveRecord: 38.0ms | Allocations: 19326)
I, [2022-04-18 11:23:15#29812]  INFO -- [Bugsnag]: Notifying https://notify.bugsnag.com of ActiveModel::UnknownAttributeError
F, [2022-04-18 11:23:15#29812] FATAL -- :   
ActiveModel::UnknownAttributeError (unknown attribute 'preferred_unit_from_list' for Spree::Calculator.):
  
app/controllers/admin/resource_controller.rb:28:in `update'
lib/session_cookie_upgrader.rb:19:in `call'
I, [2022-04-18 11:23:15#29812]  INFO -- : source=rack-timeout id=4ca105bd-dbd1-4c3d-8c30-426adc9960d6 wait=7ms timeout=120000ms service=153ms state=completed

Hope this is helpful. Moving back to in dev.

[mkllnk]

Ideally, we would not have manual testing stage at all - right?

I'm not sure, actually. Ideally, we would have manual release testing. But the author of a PR is always biased and it's very difficult to think outside the box. That's why we have reviews for the code and we need testing as a review of functionality. The best PR testing is actually not just following testing steps because that's something the author can do and we can automate. We need a hacker mind thinking: "How can I break this?" How could this go wrong? Which other parts of the application may be affected?

But I guess slowing down the build is a side effect, yeah. But still desired trade-off, right?

Right. There's a limit, of course, but a spec that runs a few seconds is always better than manual testing.

[jibees]

@filipefurtad0
I cannot reproduce it on my local env.
I rebased this PR, and I think we would 👍

• test this new rebased PR on staging-uk with the same shipping method (887 )
• test master branch on staging-uk with the same shipping method

EDIT Right, I finally reproduced it (need more ☕️ ?)

[jibees]

Ok, found it.
I've corrected in this interface:

But I should correct under the /admin/shipping_methods/[SHIPPING_METHOD_ID]/edit too.

[jibees]

Not that issue should be now tested in both /admin/shipping_methods/9/edit and /admin/enterprise_fees page.

[jibees]

Back in Code Review. Just small modifications (similar of the previous one + test in the /admin/shipping_methods/ID/edit context)

[filipefurtad0]

Hey @jibees ,

Awesome, all good now:

• /admin/shipping_methods/<id>/edit
  

• /admin/enterprise_fees
  

Merging 🎉

PS: not related to this PR, we have this console output - when editing shipping methods:

12:09:40.437 TypeError: $scope.Enterprise[$scope.address_type] is null
    addressStateMatchesCountry all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:704
    <anonymous> all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:704
    $digest all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:570
    $apply all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:570
    timeoutId all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:571
    completeTask all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:571
    timeoutId all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    __trace__ inline-script-content.js:146
    setTimeout handler*We/load/r/< inline-script-content.js:159
    defer all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    timeout all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:571
    <anonymous> all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:704
    invoke all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    get all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:568
    nodeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:568
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    nodeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:568
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    nodeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:568
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    compositeLinkFn all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    compile all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    doBootstrap all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:566
    $eval all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:570
    $apply all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:570
    doBootstrap all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:566
    invoke all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:567
    doBootstrap all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:566
    bootstrap all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:566
    angularInit all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:566
    <anonymous> all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:575
    fire all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:26
    fireWith all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:26
    ready all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:26
    completed all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:14
    __trace__ inline-script-content.js:146
    r inline-script-content.js:159
    promise all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:26
    <anonymous> all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:26
    <anonymous> all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:14
    <anonymous> all-6de01f2b58a47f7a89eee84dfd9203c8d8a869fc48286efa49bcdfec6c371522.js:14
console-breadcrumbs.js:33:15