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

Add change invoices ability #241

Merged
merged 1 commit into from Jul 6, 2017
Merged

Add change invoices ability #241

merged 1 commit into from Jul 6, 2017

Conversation

TakteS
Copy link

@TakteS TakteS commented Jun 15, 2017

Hello!

I just added functions for change invoices. My new tests are passes, but some old tests failed - I think it's because I changed setup block in the test file. Should I rewrite old VCR files to make it work?

@coveralls
Copy link

coveralls commented Jun 15, 2017

Coverage Status

Coverage decreased (-0.8%) to 77.551% when pulling 0d94671 on TakteS:master into 1b6a71a on code-corps:master.

@coveralls
Copy link

coveralls commented Jun 15, 2017

Coverage Status

Coverage decreased (-0.8%) to 77.551% when pulling a88ac47 on TakteS:master into 1b6a71a on code-corps:master.

@begedin
Copy link
Contributor

begedin commented Jul 4, 2017

@TakteS Unfortunately, our VCRs are quite flimsy and unstable, as I'm sure you've noticed. If you managed to get this working, we'll be happy to merge.

Otherwise, one of us will tackle it when we get the chance. However, I'm not sure when that will be.

While we're on it, I'd like to point you in the direction of our 2.0 branch. You might be able to get your feature working more easily there.

@lessless
Copy link

lessless commented Jul 4, 2017

@begedin can it be due to different API versions?

@begedin
Copy link
Contributor

begedin commented Jul 5, 2017

@lessless Somehow, the VCR tapes have "sequential" dependencies, meaning they need to be re-recorded in exact order to work. It was before my time here, but I do have an idea how it may have happened. Unfortunately, that means that in order to fix them and avoid the same problem in the future, we need to rethink the general approach used and invest a moderate amount of time to do it.

@lessless
Copy link

lessless commented Jul 5, 2017

@begedin I see. What would be an immediate solution? We do need this feature in our app :)

@coveralls
Copy link

coveralls commented Jul 5, 2017

Coverage Status

Coverage increased (+1.4%) to 79.747% when pulling 6b7f215 on TakteS:master into 1b6a71a on code-corps:master.

@TakteS
Copy link
Author

TakteS commented Jul 5, 2017

@begedin I rewrote some cassettes and now all tests are passed

@begedin
Copy link
Contributor

begedin commented Jul 5, 2017

@TakteS Excellent work. This is good to go, as far as I'm concerned. I just need you to squash the commits into a single commit and I can merge and release.

@lessless
Copy link

lessless commented Jul 5, 2017

@begedin can you flip the switch then, please?

@begedin
Copy link
Contributor

begedin commented Jul 5, 2017

@lessless As I said, I need @TakteS To merge the 3 commits into one before I can merge. I do not have access to his fork, so @TakteS is the only one who can do this.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 79.747% when pulling d89e865 on TakteS:master into 802694c on code-corps:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 79.747% when pulling d89e865 on TakteS:master into 802694c on code-corps:master.

@TakteS
Copy link
Author

TakteS commented Jul 5, 2017

@begedin Done

@begedin begedin merged commit 26f2d6a into beam-community:master Jul 6, 2017
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.

None yet

4 participants