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.

Sign up for GitHub

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

Jump to bottom

Added server time property to operations #92

Merged
merged 9 commits into from
May 18, 2023
Merged

Added server time property to operations #92

merged 9 commits into from
May 18, 2023

Conversation

kober32
Copy link
Member

@kober32 kober32 commented Feb 13, 2023 •
edited
Loading

Resolved #91

Adding a server time feature that can be retrieved from the operation list

Same feature was already implemented on iOS wultra/mtoken-sdk-ios#112

@kober32 kober32 marked this pull request as draft February 13, 2023 15:22
@kober32 kober32 marked this pull request as ready for review February 14, 2023 14:47
@kober32 kober32 requested a review from TomasKypta February 15, 2023 16:47
@kober32
Copy link
Member Author

kober32 commented Feb 15, 2023

Remarks fixed + I added one more assert to tests

TomasKypta
TomasKypta approved these changes Feb 20, 2023
View reviewed changes
Copy link
Member

@TomasKypta TomasKypta left a comment

Choose a reason for hiding this comment

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

Looks OK

library/src/main/java/com/wultra/android/mtokensdk/operation/OperationsService.kt Outdated Show resolved Hide resolved
library/src/main/java/com/wultra/android/mtokensdk/operation/OperationsService.kt Outdated Show resolved Hide resolved
hvge
hvge requested changes Feb 20, 2023
View reviewed changes
Copy link
Member

@hvge hvge left a comment

Choose a reason for hiding this comment

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

I think that the algorithm for calculating the adjusted time is wrong. See my comment below.

library/src/main/java/com/wultra/android/mtokensdk/operation/OperationsService.kt Outdated Show resolved Hide resolved
@kober32 kober32 requested a review from hvge May 17, 2023 11:55
hvge
hvge approved these changes May 18, 2023
View reviewed changes
Copy link
Member

@hvge hvge left a comment

Choose a reason for hiding this comment

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

Looks OK now. I would like to note that we should also update iOS implementation, because it's slightly different now. There's for example missing test for FORCED_SERVER_TIME_CHANGE_MS

library/src/main/java/com/wultra/android/mtokensdk/operation/OperationsService.kt Show resolved Hide resolved
@kober32 kober32 merged commit 2106f59 into develop May 18, 2023
@kober32 kober32 deleted the 91-server-now-feature branch May 18, 2023 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hvge hvge hvge approved these changes

@TomasKypta TomasKypta TomasKypta approved these changes

@petrdvorak petrdvorak Awaiting requested review from petrdvorak

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement "server now" feature
3 participants
@kober32 @TomasKypta @hvge