-
Notifications
You must be signed in to change notification settings - Fork 496
Provide survey URL (EXPOSUREAPP-4858) #2307
Provide survey URL (EXPOSUREAPP-4858) #2307
Conversation
* @throws IllegalStateException If no one time password (otp) can be loaded from the OTPRepository of if the | ||
* AppConfig doesn't contain a link to a survey | ||
*/ | ||
suspend fun provideUrl(type: Surveys.Type): String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please consider to put this logic/method to Surveys.kt
we could then validate if the OTP has been used already this month.
If no OTP is there, we can just generate one...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My plan was that after the OTP generation/validation/authentication (Part of Chris' subtask) ,Surveys.kt
can simply call provideUrl()
. At this point, a generated, validated and authenticated OTP should be stored in our OtpRepository
that is then loaded by SurveyUrlProvider
to assemble the URL.
I pushed a new commit where the UrlProvider
is called in Surveys.kt
.
Do you think the UrlProvider
shouldn't be a separate component but a method in Surveys.kt
? What would be the benefit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SurveyUrlProvider could be irritating if one calls it from another class than Surveys.
For e.g. one tries to get an URL for a High_Risk_Survey. He could use the url provider instead of surveys and so accidentally skip the generation/validation/authentication of the OTP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the OTP (re)generated on-demand?
What is the calling classes expected to do when provideUrl
throws IllegalStateException
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BMItter
I added a new commit. Now you need to pass the OTP
to the UrlProvider
and since only the Surveys class is going to generate OTP
s, it shouldn't be called from anywhere else.
@d4rken
I changed the UrlProvider so that we now already have to pass an authenticated OTP
to it.
I guess the calling class should show a "Something went wrong...." error message to the user in this case.
Corona-Warn-App/src/main/java/de/rki/coronawarnapp/datadonation/survey/SurveyUrlProvider.kt
Outdated
Show resolved
Hide resolved
...-App/src/test/java/de/rki/coronawarnapp/datadonation/survey/SurveyHighRiskUrlProviderTest.kt
Outdated
Show resolved
Hide resolved
* @throws IllegalStateException If no one time password (otp) can be loaded from the OTPRepository of if the | ||
* AppConfig doesn't contain a link to a survey | ||
*/ | ||
suspend fun provideUrl(type: Surveys.Type): String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the OTP (re)generated on-demand?
What is the calling classes expected to do when provideUrl
throws IllegalStateException
?
# Conflicts: # Corona-Warn-App/src/main/java/de/rki/coronawarnapp/datadonation/survey/Surveys.kt
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 👌
The component that provides the URL for Surveys.
For our current survey (high-risk cards), the base URL is loaded from our
AppConfig
and then the one-time-password is added as a query parameter. The name of this query parameter is also contained in theAppConfig