Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete dead code in OrdersController#update_distribution #7638

Conversation

Matt-Yorkley
Copy link
Contributor

What? Why?

I can't see any evidence that these params ever get sent to this action containing these strings.

Correct me if I'm wrong though!

🔥🔥

What should we test?

This theoretically relates to places in the frontoffice where the user can select either the distributor or the order cycle for their current order. There's not many places where we do that. I guess just a quick check that the user can choose a shop and select an order cycle? Are there any other places or scenarios where that could theoretically happen?

Release notes

Removed some unused code from OrdersController.

Changelog Category: Technical changes

I can't see any evidence that these params ever get sent to this action...
@Matt-Yorkley Matt-Yorkley self-assigned this May 13, 2021
@codecov
Copy link

codecov bot commented May 13, 2021

Codecov Report

Merging #7638 (eafd74f) into master (4932e1d) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head eafd74f differs from pull request most recent head 82a6bef. Consider uploading reports for the commit 82a6bef to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7638      +/-   ##
==========================================
+ Coverage   93.21%   93.26%   +0.05%     
==========================================
  Files         635      635              
  Lines       18136    18129       -7     
==========================================
+ Hits        16905    16908       +3     
+ Misses       1231     1221      -10     
Impacted Files Coverage Δ
app/controllers/api/v0/shipments_controller.rb 100.00% <100.00%> (+2.85%) ⬆️
app/controllers/spree/orders_controller.rb 92.59% <100.00%> (+6.03%) ⬆️
...ent/app/services/order_management/order/updater.rb 98.73% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4932e1d...82a6bef. Read the comment docs.

Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! I'd say this is a pr-no-test.

@@ -14,7 +14,7 @@ class OrdersController < ::BaseController
respond_to :html
respond_to :json

before_action :update_distribution, only: :update
before_action :set_current_order, only: :update
before_action :filter_order_params, only: :update
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you move these before_actions, only: update to beginning of the update method?

@@ -74,6 +71,10 @@ def edit
end

def update
set_current_order
filter_order_params
check_at_least_one_line_item
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@sauloperez
Copy link
Contributor

I guess just a quick check that the user can choose a shop and select an order cycle? Are there any other places or scenarios where that could theoretically happen?

this is definitely covered by the test suite. Are you happy to merge this @Matt-Yorkley I'm with @luisramos0 on the pr-no-test.

@Matt-Yorkley Matt-Yorkley force-pushed the dead-code-order-distribution branch from 3702105 to 82a6bef Compare May 14, 2021 17:39
@Matt-Yorkley
Copy link
Contributor Author

Moving those before_action methods into the action broke various specs 😞 I've dropped that commit for now.

I think this controller needs more cleanup at some point, but maybe in another PR. For example, I'm not sure that params-mangling that's happening in #filter_order_params actually works in the way it was originally intended to (since Rails 4.2)!

@luisramos0
Copy link
Contributor

👍 🙈

@andrewpbrett andrewpbrett merged commit c7f80d8 into openfoodfoundation:master May 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants