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

fix: adds the missing expire attribute to ListSessions call #4872

Conversation

CoderMayhem
Copy link

@CoderMayhem CoderMayhem commented Dec 16, 2022

What does this PR do?

Adds the missing expire attribute to the ListSessions response. Linked to issue #4846

Test Plan

Changes were done as suggested in the issue discussion thread by @stnguyen90. I couldn't figure out a way to test it and would appreciate any help in setting up a dummy call to the API and check for ListSessions output.

Related PRs and Issues

Fixes #4846

Have you added your change to the Changelog?

Yes

(The CHANGES.md file tracks all the changes that make it to the main branch. Add your change to this file in the following format)

  • One line description of your PR [#pr_number](Link to your PR)

Have you read the Contributing Guidelines on issues?

Yes

@@ -1342,6 +1342,7 @@

$session->setAttribute('countryName', $countryName);
$session->setAttribute('current', ($current == $session->getId()) ? true : false);
$session->setAttribute('expire', DateTime::addSeconds(new \DateTime($session->getCreatedAt()), $authDuration));
Copy link
Contributor

Choose a reason for hiding this comment

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

The value should be formatted like an ISO string as well.

Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

Great PR! 🤯 We left some comments during the review, please check them out.

@stnguyen90
Copy link
Contributor

@CoderMayhem are you still able to work on this PR or should this be reassigned?

@CoderMayhem
Copy link
Author

Hey @stnguyen90, looks like I missed your comment. I apologize. I'll do the changes requested and push them.

@stnguyen90
Copy link
Contributor

@CoderMayhem, how's your progress on this? Also, it looks like there's a merge conflict now.

@stnguyen90
Copy link
Contributor

@CoderMayhem, FYI, I'll need to close this due to inactivity soon.

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.

🐛 Bug Report: expire is empty from account.listSessions()
2 participants