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

#211, currentTotal variable is used till 4.6, therefore retrieved data… #212

Open
wants to merge 1 commit into
base: 4.7-dev
Choose a base branch
from

Conversation

pradpnayak
Copy link

… stored in priceValue

… stored in priceValue

211, currentTotal variable is used till 4.6, therefore retrieved data stored in priceValue

--fixed typeof

--fixed code
// This should not happen on Confirm Contribution, but seems to on 4.6 for some reason.
//return true;
}
currentTotal = $('#pricevalue').data('raw-total');
Copy link
Author

Choose a reason for hiding this comment

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

currentTotal is used till CiviCRM version 4.6.x. In 4.7 currentTotal is never set and hence the code tries to validate payment details which should not be in case total amount is 0. An alternate method which will work for all version of civicrm is to retrieve data stored in pricevalue.

// This is also hit when "Going back", but we already have stripe_token.
debugging('ozlkf');
// This should not happen on Confirm Contribution, but seems to on 4.6 for some reason.
//return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If currentTotaal == 0 and there are no additional participants, shouldn't we return true? I'm not sure why that is commented out in the original or in your fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The comments tell the story.....it was breaking 4.6 transactions.

//This should not happen on Confirm Contribution, but seems to on 4.6 for some reason.

Do you mind testing this PR with that line uncommented on 4.6?

Choose a reason for hiding this comment

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

@h-c-c maybe it's better to have separate branches for 4.6 and 4.7? This issue still exists and there doesn't seem to be any progress on testing it on 4.6.

btw, this commit progressivetech@b255663 is probably the better one to create as a PR.

@jmcclelland
Copy link
Contributor

I'm not running any 4.6 instances at the moment - but since there is a separate 4.6-dev and 4.7-dev branches, perhaps this could be committed to the 4.7-dev branch?

@h-c-c
Copy link
Collaborator

h-c-c commented Jun 20, 2017

That was abandoned a while ago. I've been working to support both in this branch. If you're interested in contributing upstream, you should check out buildkit. You can spin up a civi instance in a one liner!

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.

4 participants