Fix pre-auth behavior #2
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bpcreech
force-pushed
the
feat/preauth-is-succeeded
branch
from
cfa1e13
to
6c99bee
Compare
bpcreech
pushed a commit
that referenced
this pull request
Oct 12, 2024
adrienverge#232 is failing with a "This environment is externally managed" error from pip because it's trying to install to the system Python. For some reason this doesn't happen on a fork, e.g., #2. I assume this is due to a Github probably runner change; I'm not sure whether intentional. Anyway we can just be explicit; pip should just use a user install during the test run.
bpcreech
pushed a commit
that referenced
this pull request
Oct 12, 2024
adrienverge#232 is failing with a "This environment is externally managed" error from pip because it's trying to install to the system Python. For some reason this doesn't happen on a fork, e.g., #2. I assume this is due to a Github probably runner change; I'm not sure whether intentional. Anyway we can fix this by using a venv, which is best practice anyway.
bpcreech
force-pushed
the
feat/preauth-is-succeeded
branch
from
40bc732
to
61e8730
Compare
bpcreech
pushed a commit
that referenced
this pull request
Oct 12, 2024
adrienverge#232 is failing with a "This environment is externally managed" error from pip because it's trying to install to the system Python. For some reason this doesn't happen on a fork, e.g., #2. I assume this is due to a Github probably runner change; I'm not sure whether intentional. Anyway we can fix this by using a venv, which is best practice anyway.
bpcreech
force-pushed
the
feat/preauth-is-succeeded
branch
2 times, most recently
from
327e6c8
to
e6a4305
Compare
bpcreech
pushed a commit
that referenced
this pull request
Oct 12, 2024
adrienverge#232 is failing with a "This environment is externally managed" error from pip because it's trying to install to the system Python. For some reason this doesn't happen on a fork, e.g., #2. I assume this is due to a Github probably runner change; I'm not sure whether intentional. Anyway we can fix this by using a venv, which is best practice anyway.
GitHub is rolling out `ubuntu-24.04` as the new `ubuntu-latest` on GitHub
Actions[^1]. Previously it was `ubuntu-22.04`.
There are many small breaking changes that people are complaining about in
this update[^2].
Localstripe is impacted by the fact that `pip` now fails on installing a paquet
system-wide with the following error:
```
error: externally-managed-environment
× This environment is externally managed
╰─> To install Python packages system-wide, try apt install
python3-xyz, where xyz is the package you are trying to
install.
If you wish to install a non-Debian-packaged Python package,
create a virtual environment using python3 -m venv path/to/venv.
Then use path/to/venv/bin/python and path/to/venv/bin/pip. Make
sure you have python3-full installed.
If you wish to install a non-Debian packaged Python application,
it may be easiest to use pipx install xyz, which will manage a
virtual environment for you. Make sure you have pipx installed.
See /usr/share/doc/python3.12/README.venv for more information.
```
There are multiple solutions to this as proposed in the error message:
- Use `sudo apt install python3-...`
- Use a venv with `python -m venv venv`
- Use a venv with `pipx`
- Use the `pip` option `--break-system-packages`
- Use `ubuntu-22.04` instead of `ubuntu-latest`
For the package `build`, `python3-build` exists, so let's use it. For
`localstripe-*.tar.gz`, as we already are in an isolated container where
breaking Python is meaningless, and as there is nothing in that container that
might actually break when we install Localstripe, let's just use the option
`--break-system-packages`.
After this commit, the CI works again with `ubuntu-latest`.
[^1]: https://github.blog/changelog/2024-09-25-actions-new-images-and-ubuntu-latest-changes/
[^2]: actions/runner-images#10636
bpcreech
force-pushed
the
feat/preauth-is-succeeded
branch
from
e6a4305
to
894cb7c
Compare
Before this change we were erroneously marking pre-auth'd charges as status=pending, when they're actually status=succeeded. We were (accidentally) working around this incorrect behavior in pre-auth'd PaymentIntents. To get this right we have to actually split the _trigger_payment method into two: a check for payment authorization (which we do on construction even for Charges created with capture=false), and a separate routine to actually capture the charge (which we do on construction for non-pre-auth'd charges, and on _api_capture for pre-auth'd charges). We also split the PaymentIntent._api_confirm method into two for more control of error handling. We then adjust the PaymentIntent wrapper to fit. This also fixes a tiny mistake in the Charge refund test; it was asserting the wrong variable.
bpcreech
force-pushed
the
feat/preauth-is-succeeded
branch
from
894cb7c
to
17269d9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this change we were erroneously marking pre-auth'd charges as status=pending, when they're actually status=succeeded. We were (accidentally) working around this incorrect behavior in pre-auth'd PaymentIntents.
To get this right we have to actually split the _trigger_payment method into two: a check for payment authorization (which we do on construction even for Charges created with capture=false), and a separate routine to actually capture the charge (which we do on construction for non-pre-auth'd charges, and on _api_capture for pre-auth'd charges). We then adjust the PaymentIntent wrapper to fit.
This also fixes a tiny mistake in the Charge refund test; it was asserting the wrong variable.