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

Do not display non-eligible adjustments in the admin cart overview #3585

Merged

Conversation

coorasse
Copy link
Contributor

@coorasse coorasse commented Apr 13, 2020

See #2860 and #2484 .
This PR tackles the points discussed in the previous PR from 2018.
In particular it adds a regression test for the issue.
It also improves another test, since it was testing nothing

.map(function(item){ return item.get("adjustments") || [] })
.map(function(item) {
return (item.get("adjustments") || [])
.filter(function(adjustment) {

Choose a reason for hiding this comment

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

Expected to return a value at the end of function array-callback-return

Copy link
Member

Choose a reason for hiding this comment

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

@coorasse I think @houndci-bot is right here, we should return something anyway.

@coorasse coorasse changed the title **Description** <!-- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. Do not display non-eligible adjustments in the admin cart overview Apr 13, 2020
@@ -40,8 +41,11 @@
end
end

it "only shows eligible adjustments" do
expect(page).not_to have_content("ineligible")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This page shows both eligible and non-eligible adjustments, and that's correct, because it highlights if they are not eligiblew by adding the class adjustment-ineligible. This test now checks that.

expect(page).to have_content("Rebate")
expect(page).not_to have_content("Non-Eligible")
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the regression test.

@kennyadsl kennyadsl added the changelog:solidus_backend Changes to the solidus_backend gem label Apr 17, 2020
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.

Thanks @coorasse, this makes sense, and thanks for the regression test!

.map(function(item){ return item.get("adjustments") || [] })
.map(function(item) {
return (item.get("adjustments") || [])
.filter(function(adjustment) {
Copy link
Member

Choose a reason for hiding this comment

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

@coorasse I think @houndci-bot is right here, we should return something anyway.

@kennyadsl
Copy link
Member

P.S. I've re-run failed Netlify checks, should be all green now but please take a look at the HoundCI complain.

@aldesantis
Copy link
Member

@kennyadsl Netlify is still failing here. 😢

@coorasse coorasse requested a review from kennyadsl April 20, 2020 06:58
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.

Thanks, @coorasse! 🎉

@aldesantis aldesantis merged commit 98d40b2 into solidusio:master Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_backend Changes to the solidus_backend gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants