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

fn to expire checkout sessions #334

Merged
merged 1 commit into from
Feb 4, 2023

Conversation

sloganking
Copy link
Contributor

@sloganking sloganking commented Feb 1, 2023

Summary

Fixes #331

Checklist

@sloganking
Copy link
Contributor Author

Looking at CONTRIBUTING.md. I ran

cargo test --features runtime-blocking the 25 unit tests passed. But tests/account.rs failed with

Running tests/account.rs (target/debug/deps/account-b392d7dfeb7da677)

running 1 test
test is_account_listable ... FAILED

failures:

---- is_account_listable stdout ----
thread 'is_account_listable' panicked at 'error communicating with stripe: error trying to connect: tcp connect error: Connection refused (os error 111)', tests/account.rs:9:25
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'is_account_listable' panicked at 'assertion failed: result.is_ok()', tests/mock/mod.rs:13:5


failures:
    is_account_listable

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.04s

error: test failed, to rerun pass `--test account`

I am not sure if I am supposed to put a stripe token somewhere so it will let the test talk to it.

===

I also ran cargo +nightly clippy --all --all-targets -- -D warnings

But it seemed to give back a whole bunch of existing warnings that were not related to my change. So I am not sure if that is actively being followed now.

@sloganking
Copy link
Contributor Author

I'm not sure if taking a mutable reference to a full CheckoutSession and replacing the value with the CheckoutSession returned from POST, would be better here. As that would entirely prevent mistakenly using the old CheckoutSession variable with outdated information. But it is not how the rest of the library functions I have seen here have been doing it.

@arlyon
Copy link
Owner

arlyon commented Feb 2, 2023

You raise a cool idea. I am interested in maybe supporting a trait that allows the id but consumes the object. Raise an issue and we can track it! :)

Regarding tests, a few of them need stripe mock to run properly so I think you're ok.

Clippy is a little more complicated, as you need to pick a runtime to lint against. One of the downsides with different feature-based runtimes...

I have approved CI now so new commits should run it automatically. Could you prefix your commit message with feat:? We pull those out automatically to generate the change logs.

Thanks!

@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Merging #334 (0368c79) into master (260d450) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##           master    #334      +/-   ##
=========================================
- Coverage    6.57%   6.57%   -0.01%     
=========================================
  Files         130     130              
  Lines       20022   20025       +3     
=========================================
  Hits         1317    1317              
- Misses      18705   18708       +3     
Impacted Files Coverage Δ
src/resources/checkout_session_ext.rs 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sloganking
Copy link
Contributor Author

Could you prefix your commit message with feat:? We pull those out automatically to generate the change logs.

@arlyon This should be fixed now.

@arlyon
Copy link
Owner

arlyon commented Feb 4, 2023

All green, thanks for taking the time! :)

@arlyon arlyon merged commit c61ff53 into arlyon:master Feb 4, 2023
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.

CheckoutSession::expire()
2 participants