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

Loosen our dependency on FriendlyId #8072

Merged
merged 6 commits into from Jun 19, 2017

Conversation

Projects
None yet
3 participants
@damianlegawiec
Member

damianlegawiec commented Jun 17, 2017

FriendlyId should be used only for SEO purposes (product URLs, taxon permalinks) not for models like Order/Shipment/Payment which uses NumberGenerator. Switching back to standard rails finders.

Basically, reverts 259c6b3

@damianlegawiec damianlegawiec requested a review from krzysiek1507 Jun 17, 2017

@@ -10,7 +10,7 @@ class OrdersController < Spree::StoreController
skip_before_action :verify_authenticity_token, only: [:populate]
def show
@order = Order.includes(line_items: [variant: [:option_values, :images, :product]], bill_address: :state, ship_address: :state).find_by_number!(params[:id])
@order = Order.includes(line_items: [variant: [:option_values, :images, :product]], bill_address: :state, ship_address: :state).find_by!(number: params[:id])

This comment has been minimized.

@houndci-bot

houndci-bot Jun 17, 2017

Use %i or %I for an array of symbols.
Line is too long. [163/120]

@houndci-bot

houndci-bot Jun 17, 2017

Use %i or %I for an array of symbols.
Line is too long. [163/120]

@variant = Spree::Variant.find(params[:variant_id])
@quantity = params[:quantity].to_i
authorize! :read, @original_shipment
authorize! :create, Shipment
end
def find_and_update_shipment
@shipment = Spree::Shipment.accessible_by(current_ability, :update).readonly(false).friendly.find(params[:id])
@shipment = Spree::Shipment.accessible_by(current_ability, :update).readonly(false).find_by!(number: params[:id])

This comment has been minimized.

@houndci-bot

houndci-bot Jun 17, 2017

Line is too long. [123/120]

@houndci-bot

houndci-bot Jun 17, 2017

Line is too long. [123/120]

Show outdated Hide outdated api/app/controllers/spree/api/v1/shipments_controller.rb
@@ -33,7 +33,7 @@ def create
end
def update
@shipment = Spree::Shipment.accessible_by(current_ability, :update).readonly(false).friendly.find(params[:id])
@shipment = Spree::Shipment.accessible_by(current_ability, :update).readonly(false).find_by!(number: params[:id])

This comment has been minimized.

@houndci-bot

houndci-bot Jun 17, 2017

Line is too long. [123/120]

@houndci-bot

houndci-bot Jun 17, 2017

Line is too long. [123/120]

constructor: (id) ->
@url = Spree.url("#{Spree.routes.payments_api(order_id)}/#{id}.json" + '?token=' + Spree.api_key)
constructor: (number) ->
@url = Spree.url("#{Spree.routes.payments_api(order_id)}/#{number}.json" + '?token=' + Spree.api_key)

This comment has been minimized.

@houndci-bot

houndci-bot Jun 17, 2017

Line exceeds maximum allowed length. Length is 108, max is 80.

@houndci-bot

houndci-bot Jun 17, 2017

Line exceeds maximum allowed length. Length is 108, max is 80.

@krzysiek1507

I'm worried about friendly.find(params[:id]) => find_by!(number: params[:id]) change. It's breaking change in some cases

Show outdated Hide outdated core/app/models/spree/order.rb
Show outdated Hide outdated api/app/controllers/spree/api/v1/payments_controller.rb
@@ -121,7 +121,7 @@ def normalize_params
end
def find_order(lock = false)
@order = Spree::Order.lock(lock).friendly.find(params[:id])
@order = Spree::Order.lock(lock).find_by!(number: params[:id])

This comment has been minimized.

@krzysiek1507

krzysiek1507 Jun 17, 2017

Contributor

I hope it was only number using here

@krzysiek1507

krzysiek1507 Jun 17, 2017

Contributor

I hope it was only number using here

This comment has been minimized.

@damianlegawiec

damianlegawiec Jun 18, 2017

Member

@krzysiek1507 all API calls are number based, that's what official guides enforced years ago

@damianlegawiec

damianlegawiec Jun 18, 2017

Member

@krzysiek1507 all API calls are number based, that's what official guides enforced years ago

include Spree::Order::Checkout
include Spree::Order::CurrencyUpdater
include Spree::Order::Payments
include Spree::Order::StoreCredit
include Spree::Core::NumberGenerator.new(prefix: 'R')
include Spree::Core::TokenGenerator

This comment has been minimized.

@houndci-bot

houndci-bot Jun 18, 2017

Trailing whitespace detected.

@houndci-bot

houndci-bot Jun 18, 2017

Trailing whitespace detected.

@@ -0,0 +1,9 @@
module Spree

This comment has been minimized.

@houndci-bot

houndci-bot Jun 18, 2017

Missing magic comment # frozen_string_literal: true.

@houndci-bot

houndci-bot Jun 18, 2017

Missing magic comment # frozen_string_literal: true.

include Spree::Order::Checkout
include Spree::Order::CurrencyUpdater
include Spree::Order::Payments
include Spree::Order::StoreCredit
include Spree::Core::NumberGenerator.new(prefix: 'R')
include Spree::Core::TokenGenerator

This comment has been minimized.

@houndci-bot

houndci-bot Jun 18, 2017

Trailing whitespace detected.

@houndci-bot

houndci-bot Jun 18, 2017

Trailing whitespace detected.

@@ -0,0 +1,9 @@
module Spree

This comment has been minimized.

@houndci-bot

houndci-bot Jun 18, 2017

Missing magic comment # frozen_string_literal: true.

@houndci-bot

houndci-bot Jun 18, 2017

Missing magic comment # frozen_string_literal: true.

@damianlegawiec damianlegawiec merged commit f7d3e26 into spree:master Jun 19, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
hound 6 violations found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment