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

Auto update bunq context bunq/sdk_php#94 #131

Merged
merged 9 commits into from
Apr 9, 2018

Conversation

OGKevin
Copy link
Contributor

@OGKevin OGKevin commented Apr 7, 2018

References #94

@OGKevin OGKevin added this to the 0.13.5 milestone Apr 7, 2018
@OGKevin OGKevin self-assigned this Apr 7, 2018
@OGKevin OGKevin added this to To do in 1.0.0 - SDK via automation Apr 7, 2018
@OGKevin OGKevin moved this from To do to open PR in 1.0.0 - SDK Apr 7, 2018
@OGKevin OGKevin moved this from open PR to pending review in 1.0.0 - SDK Apr 7, 2018
@OGKevin OGKevin moved this from pending review to open PR in 1.0.0 - SDK Apr 7, 2018
@sandervdo sandervdo self-requested a review April 9, 2018 07:11
static::assertTrue($validApiContext->ensureSessionActive());
static::assertNotEquals($expiredApiContext, $validApiContext);

BunqContext::updateApiContext(clone $expiredApiContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why cloning here again @OGKevin ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@OGKevin removed the asserts intentionally? The one on session state can remain right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because it is triggered deeper in the code as well. So, if a do the assertion on the ensure update method. It will actually update the context. The purpose of this test is to see if the ApiClient indeed updates the context it self and assigns it to BunqContext. So I basically was updating the context instead of ApiClient doing it 🤦‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sandervdo ☝️

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack

@OGKevin
Copy link
Contributor Author

OGKevin commented Apr 9, 2018

@sandervdo pushed, please 👀

@OGKevin OGKevin merged commit 412a695 into develop Apr 9, 2018
1.0.0 - SDK automation moved this from open PR to merged Apr 9, 2018
@OGKevin
Copy link
Contributor Author

OGKevin commented Apr 9, 2018

@andrederoos

@OGKevin OGKevin deleted the auto_update_bunq_context_bunq/sdk_php#94 branch April 9, 2018 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
1.0.0 - SDK
  
merged
Development

Successfully merging this pull request may close these issues.

None yet

2 participants