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

[#44] [#43] [Integrate] [Backend] As a user, I can refresh my token when it's expired #106

Merged
merged 2 commits into from
Jan 17, 2023
Merged

[#44] [#43] [Integrate] [Backend] As a user, I can refresh my token when it's expired #106

merged 2 commits into from
Jan 17, 2023

Conversation

blyscuit
Copy link
Owner

@blyscuit blyscuit commented Jan 3, 2023 •
edited
Loading

What happened

When API returns token expired, call /api/v1/oauth/token to refresh token before calling the API again.

Insight

  • Add Ktor.Auth.refreshTokens.
  • Add RefreshTokenType.

Proof Of Work

Screen Shot 2023-01-03 at 17 27 54

@github-actions
Copy link

github-actions bot commented Jan 3, 2023 •
edited
Loading

6 Warnings
⚠️ Uh oh! RefreshTokenType.kt is under 95% coverage!
⚠️ shared/src/commonMain/kotlin/co/nimblehq/blisskmmic/data/network/core/TokenizedNetworkClient.kt#L47 - The function bearerConfig is too long (21). The maximum length is 20.
⚠️ shared/src/commonMain/kotlin/co/nimblehq/blisskmmic/presentation/modules/surveyselection/SurveySelectionViewModel.kt#L31 - The constructor(getCurrentDateUseCase: GetCurrentDateUseCase, getProfileUseCase: GetProfileUseCase, getAppVersionUseCase: GetAppVersionUseCase, surveyListUseCase: SurveyListUseCase, dateTimeFormatter: DateTimeFormatter) has too many parameters. The current threshold is set to 5.
⚠️ shared/src/commonMain/kotlin/co/nimblehq/blisskmmic/presentation/modules/surveyselection/SurveySelectionViewModel.kt#L31 - Class 'SurveySelectionViewModel' with '13' functions detected. Defined threshold inside classes is set to '10'
⚠️ shared/src/commonTest/kotlin/co/nimblehq/blisskmmic/data/repository/AuthenticationRepositoryTest.kt#L23 - Class 'AuthenticationRepositoryTest' with '10' functions detected. Defined threshold inside classes is set to '10'
⚠️ shared/src/commonTest/kotlin/co/nimblehq/blisskmmic/presentation/modules/surveyselection/SurveySelectionViewModelTest.kt#L34 - Class SurveySelectionViewModelTest is too large. Consider splitting it into smaller pieces.

🧛 shared Code Coverage: 82.61%

Coverage of Modified Files:

File Coverage
NetworkDataSource.kt 100.00%
RefreshTokenType.kt 57.32%
TokenizedNetworkClient.kt 96.81%

Modified Files Not Found In Coverage Report:

NetworkDataSourceTest.kt
NetworkModule.kt
TokenizedNetworkClientTest.kt

Codebase cunningly covered by count Shroud 🧛

Generated by 🚫 Danger

@github-actions
Copy link

github-actions bot commented Jan 4, 2023 •
edited
Loading

Current coverage for Survey is 88.45%

✅ No files affecting coverage found

Powered by xcov

Generated by 🚫 Danger

@blyscuit blyscuit added this to the 0.5.0 milestone Jan 4, 2023
@blyscuit blyscuit changed the title [#43] Add RefreshToken API [#44] [#43] [Integrate] [Backend] As a user, I can refresh my token when it's expired Jan 5, 2023
@blyscuit blyscuit self-assigned this Jan 5, 2023
@blyscuit blyscuit force-pushed the feature/#19-survey-selection-integration branch 3 times, most recently from 54db60f to 2fc5af9 Compare January 5, 2023 09:50
@blyscuit blyscuit force-pushed the feature/#43-refresh-token-backend branch from 0305c46 to 3da38f0 Compare January 6, 2023 11:06
@blyscuit blyscuit marked this pull request as ready for review January 6, 2023 11:10
doannimble
doannimble reviewed Jan 10, 2023
View reviewed changes
Copy link
Collaborator

@doannimble doannimble left a comment •
edited
Loading

Choose a reason for hiding this comment

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

just a tiny suggestion left, rest lgtm

shared/src/commonMain/kotlin/co/nimblehq/blisskmmic/data/network/target/RefreshTokenType.kt

@Serializable
data class RefreshTokenInput(
@SerialName("grant_type") val grantType: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@blyscuit It would be better if we add more line breaks for easier readability, what do you think?

Suggested change
@SerialName("grant_type") val grantType: String,
@SerialName("grant_type")
val grantType: String,

Copy link
Owner Author

Choose a reason for hiding this comment

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

This was a conversation with @minhnimble long time ago, I don't remember the PR. We concluded that we will use this format in this project.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@blyscuit @doannimble this is the conversation. In the end, it was for showing the relationship between the attribute and the SerialName annotation better, especially in a long list of attributes that don't have SerialName and only 1 or 2 attributes need it 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

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

@minhnimble @doannimble Do we have this convention on the Kotlin page, or maybe there was a discussion on the Android chapter?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We might need to double-check this one. I found this one
Screenshot 2023-01-13 at 11 59 14

Copy link
Owner Author

Choose a reason for hiding this comment

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

I like this one most, but I had no valid argument for it, just preference. ❤️

class PaginationMetaApiModel(
    val page: Int,
    val pages: Int,
    @SerialName("page_size")
    val pageSize: Int,
    val records: Int
)

Copy link
Collaborator

@minhnimble minhnimble Jan 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@suho @blyscuit @doannimble actually from our Compass, there is an inspired guideline from Ribot that mention about this annotation case for fields as follow:

  • Annotations applying to fields should be listed on the same line, unless the line reaches the maximum line length.

-> Hope that clear you guys confusion if any 🙏 And maybe we can create a PR to bring this convention to our Compass for transparency 😁

@blyscuit blyscuit requested a review from doannimble January 10, 2023 10:30
@blyscuit blyscuit force-pushed the feature/#19-survey-selection-integration branch from 2fc5af9 to d664cdc Compare January 10, 2023 11:04
@blyscuit blyscuit force-pushed the feature/#43-refresh-token-backend branch from 3da38f0 to 9c2d271 Compare January 10, 2023 11:23
@blyscuit blyscuit requested a review from suho January 10, 2023 11:26
minhnimble
minhnimble approved these changes Jan 13, 2023
View reviewed changes
Copy link
Collaborator

@minhnimble minhnimble left a comment

Choose a reason for hiding this comment

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

all good from me 🚀

blyscuit and others added 2 commits January 13, 2023 11:06
@blyscuit
Merge pull request #102 from blyscuit/feature/#19-survey-selection-in…
e021c3e 
…tegration

[#19] [Integrate] As a user, I can see Survey selection page
@blyscuit
[#43] Add RefreshToken API
0b64003
@blyscuit blyscuit force-pushed the feature/#43-refresh-token-backend branch from 9c2d271 to 0b64003 Compare January 13, 2023 04:09
suho
suho approved these changes Jan 13, 2023
View reviewed changes
shared/src/commonMain/kotlin/co/nimblehq/blisskmmic/data/network/target/RefreshTokenType.kt

@Serializable
data class RefreshTokenInput(
@SerialName("grant_type") val grantType: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@minhnimble @doannimble Do we have this convention on the Kotlin page, or maybe there was a discussion on the Android chapter?

shared/src/commonMain/kotlin/co/nimblehq/blisskmmic/data/network/target/RefreshTokenType.kt

@Serializable
data class RefreshTokenInput(
@SerialName("grant_type") val grantType: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might need to double-check this one. I found this one
Screenshot 2023-01-13 at 11 59 14

@blyscuit blyscuit merged commit 639c3b3 into feature/#19-survey-selection-integration Jan 17, 2023
@blyscuit blyscuit deleted the feature/#43-refresh-token-backend branch January 17, 2023 02:29
@blyscuit blyscuit restored the feature/#43-refresh-token-backend branch January 17, 2023 02:31
@blyscuit blyscuit mentioned this pull request Jan 17, 2023
Merged
@blyscuit blyscuit deleted the feature/#43-refresh-token-backend branch January 17, 2023 03:46
@blyscuit blyscuit mentioned this pull request Jan 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@minhnimble minhnimble minhnimble approved these changes

@suho suho suho approved these changes

@edgarss edgarss Awaiting requested review from edgarss

@doannimble doannimble Awaiting requested review from doannimble

Assignees

@blyscuit blyscuit

Labels
platform: KMM type: feature
Projects
None yet
Milestone
0.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@blyscuit @suho @minhnimble @doannimble