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

Fix a bug causing non-eligible adjustments to appear in the the admin… #2860

Conversation

jacobherrington
Copy link
Contributor

… cart

Fixes #2484.

ES6 would make this significantly cleaner, but it seems we favor ES5 for now (happy to make that change though!)

I don't have a ton of experience with Backbone.js, so if there is a more performant built-in solution for this I'd love to hear about it.

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

@jacobherrington thanks! I'm ok with the change but can you please explain better in the PR and/or in the commit message description why filtering adjustments by eligibility should solve the issue?

Also, maybe we should also add a regression spec for this, right?

@jacobherrington
Copy link
Contributor Author

@kennyadsl essentially the frontend is displaying all adjustments including those which are ineligible on the order. They are not actually being applied to the order, just displayed on the page this JS effects.

Filtering them by eligibility causes this function to return an empty array which is what should happen when there are not adjustments affecting the order.

@kennyadsl
Copy link
Member

Can you also please take a look at the specs for this? I'm seeing that we should already be covered but it looks like that test is a false positive.

@jacobherrington
Copy link
Contributor Author

@kennyadsl that test is actually running on another page. That is running admin/orders/:id/adjustment (and should pass), but this change affects admin/orders/:id/cart. I will look into writing a regression spec for this change.

@ericsaupe
Copy link
Contributor

@jacobherrington any update on this?

@jacobherrington
Copy link
Contributor Author

@ericsaupe I haven't had a chance to revisit it unfortunately!

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.

Non Eligible Order Adjustments Showing in Admin Order Cart Page
3 participants