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

Calculator change in shipping methods broken again #1331

Closed
wants to merge 2 commits into from

Conversation

ademidov
Copy link
Contributor

Just small selector fix.

@radar
Copy link
Contributor

radar commented Mar 29, 2012

Please provide some steps to reproduce the issue that you're encountering so we can be sure that this patch fixes something.

@ademidov
Copy link
Contributor Author

Of course.

  1. Edit Shipping Method
  2. Change Calculator
  3. Click 'Update'
  4. Get message 'Shipping method is not found'

@radar
Copy link
Contributor

radar commented Mar 29, 2012

Thanks, will take a look into this.

@radar
Copy link
Contributor

radar commented Mar 29, 2012

I can confirm this is a problem. Will write a test to reproduce it.

@@ -1,8 +1,9 @@
$(function() {
var original_calc_type = $('#calc-type').attr('value');
var calculator_select = $('select#shipping_method_calculator_type')
Copy link
Contributor

Choose a reason for hiding this comment

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

This calculator js is used on the tax rates form too, and so by changing it here you're breaking it on the tax rates form. imo it should be select#calc-type still so it remains compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, i see. So problem in partial /admin/shared/_calculator_fields.html.erb

@radar radar closed this in 8265438 Mar 29, 2012
@radar
Copy link
Contributor

radar commented Mar 29, 2012

First commit contains your changes, second commit contains my changes to your changes and a spec to test it.

Thanks!

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.

2 participants