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.

Sign up for GitHub

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

Jump to bottom

WIP #1037 Add Quantity to Customer and Add Query or Create Variant … #1055

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

WIP #1037 Add Quantity to Customer and Add Query or Create Variant … #1055

wants to merge 1 commit into from

Conversation

Regene27
Copy link
Collaborator

image

simplescreenrecorder-2024-01-29_17.15.26.mp4

@Regene27 Regene27 requested a review from channainfo January 29, 2024 10:18
@Regene27 Regene27 self-assigned this Jan 29, 2024
@Regene27 Regene27 linked an issue Jan 29, 2024 that may be closed by this pull request
Add variant creation through subscription and add quantity field to cm_customers #1037
Open
@Regene27 Regene27 force-pushed the 1037-add-variant-through-subscription-and-add-quantity-field-to-cm-customers branch 2 times, most recently from c9025c9 to 66062a8 Compare January 29, 2024 10:24
@Regene27 Regene27 force-pushed the 1037-add-variant-through-subscription-and-add-quantity-field-to-cm-customers branch from 66062a8 to d225183 Compare January 31, 2024 08:13
@Regene27 Regene27 requested a review from channainfo January 31, 2024 08:13
@Regene27 Regene27 force-pushed the 1037-add-variant-through-subscription-and-add-quantity-field-to-cm-customers branch 3 times, most recently from 6d7b1dc to e791bba Compare February 1, 2024 07:10
@Regene27 Regene27 force-pushed the 1037-add-variant-through-subscription-and-add-quantity-field-to-cm-customers branch from e791bba to 4017c3f Compare February 22, 2024 10:28
@Regene27 Regene27 requested a review from channainfo February 22, 2024 10:28
@Regene27 Regene27 force-pushed the 1037-add-variant-through-subscription-and-add-quantity-field-to-cm-customers branch 6 times, most recently from 2a83fdb to 2db9e20 Compare February 28, 2024 09:35
@Regene27 Regene27 force-pushed the 1037-add-variant-through-subscription-and-add-quantity-field-to-cm-customers branch 2 times, most recently from 0a60d0e to ca377d5 Compare February 29, 2024 04:44
@Regene27 Regene27 force-pushed the 1037-add-variant-through-subscription-and-add-quantity-field-to-cm-customers branch 2 times, most recently from 4d16186 to 00231bc Compare March 7, 2024 02:59
@Regene27 Regene27 requested a review from channainfo March 7, 2024 03:00
@channainfo
Copy link
Owner

@Regene27 It seems there's a misunderstanding regarding the use of before_action, load_, and lazy loading (memoization) techniques. Please come to the Bookmebus office so we can clarify and resolve the issues in this PR.

@Regene27 Regene27 force-pushed the 1037-add-variant-through-subscription-and-add-quantity-field-to-cm-customers branch from 00231bc to 2d6222b Compare March 7, 2024 04:52
@Regene27 Regene27 force-pushed the 1037-add-variant-through-subscription-and-add-quantity-field-to-cm-customers branch from 2d6222b to ce0dd33 Compare March 7, 2024 05:51
channainfo
channainfo requested changes Mar 7, 2024
View reviewed changes
app/controllers/spree/billing/variants_controller.rb
if request.xhr?
@variant = @product.variants.build

render :show, layout: false
Copy link
Owner

Choose a reason for hiding this comment

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

render layout: false instead and create a new.html.erb in the view.

app/controllers/spree/billing/variants_controller.rb

render :show, layout: false
else
redirect_to new_billing_product_variant_url(@product)
Copy link
Owner

Choose a reason for hiding this comment

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

this will redirect to itself and then it will end up with a loop redirect

app/views/spree/billing/subscriptions/_form.html.erb
var productSku = $(productSelector).val();

$.ajax({
url: '/en/billing/products/' + productSku + '/variants/new',
Copy link
Owner

Choose a reason for hiding this comment

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

pass the URL from the Rails. Don't construct the URL in the js

spec/services/spree_cm_commissioner/sku_generator_spec.rb
RSpec.describe SpreeCmCommissioner::SkuGenerator do
let(:product) { create(:product, name: "Waste Collection") }
let(:option_types) { [create(:option_type, name: "Month"), create(:option_type, name: "Due Date"), create(:option_type, name: "Payment Method")] }
let(:option_values) { [create(:option_value, name: "1", option_type: option_types[0]), create(:option_value, name: "1", option_type: option_types[1]), create(:option_value, name: "pre-paid", option_type: option_types[2])] }
Copy link
Owner

Choose a reason for hiding this comment

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

align this properly

channainfo
channainfo requested changes Mar 29, 2024
View reviewed changes
app/services/spree_cm_commissioner/sku_generator.rb
def generate_sku
return if @variant_params.nil? || @variant_params.blank?

option_values = @variant_params[:option_value_ids].map { |id| Spree::OptionValue.find(id) }
Copy link
Owner

Choose a reason for hiding this comment

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

Use Spree::OptionValue.find(@variant_params[:option_value_ids) instead.

app/services/spree_cm_commissioner/sku_generator.rb
option_values = @variant_params[:option_value_ids].map { |id| Spree::OptionValue.find(id) }
sku_parts = [product.name]

option_values.each do |option_value|
Copy link
Owner

Choose a reason for hiding this comment

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

use the map method instead.

app/services/spree_cm_commissioner/variant_checker.rb
attr_reader :variant

def initialize(variant_params, current_vendor)
@product = Spree::Product.find_by(id: variant_params[:product_id])
Copy link
Owner

Choose a reason for hiding this comment

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

use the find method instead and create a method called product with

@product ||= Spree::Product.find(variant_params[:product_id])

app/services/spree_cm_commissioner/variant_checker.rb
@current_vendor = current_vendor
end

def find_or_create_variant
Copy link
Owner

Choose a reason for hiding this comment

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

rename find_or_create_variant to variant instead and merge the implementation of the find_variant_by_sku and create_variant. No need to have find_variant_by_sku and create_variant methods.

app/services/spree_cm_commissioner/variant_checker.rb

def variant_sku
sku_generator = SpreeCmCommissioner::SkuGenerator.new(@product, @variant_params)
sku_generator.generate_sku
Copy link
Owner

Choose a reason for hiding this comment

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

memoize this method.

app/services/spree_cm_commissioner/variant_creator.rb
@@ -0,0 +1,37 @@
module SpreeCmCommissioner
class VariantCreator
Copy link
Owner

Choose a reason for hiding this comment

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

apply the same concept here.

app/services/spree_cm_commissioner/variant_creator.rb
end

def create_variant
@variant = @product.variants.build
Copy link
Owner

Choose a reason for hiding this comment

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

inconsistent code.

@Regene27 Regene27 changed the title Close #1037 Add Quantity to Customer and Add Query or Create Variant … WIP #1037 Add Quantity to Customer and Add Query or Create Variant … Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@channainfo channainfo channainfo requested changes

Assignees

@Regene27 Regene27

Labels
backend
Projects
Spree CM Commissioner
Status: 🏗 In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add variant creation through subscription and add quantity field to cm_customers
2 participants
@Regene27 @channainfo