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

feat: Handle app link intent for password reset and add unit test #2081

Merged
merged 1 commit into from
Jul 12, 2019

Conversation

liveHarshit
Copy link
Member

@liveHarshit liveHarshit commented Jul 8, 2019

Fixes #2079

  • Add robolectric library for the test implementation
  • Remove unnecessary library and methods from event unit test

@auto-label auto-label bot added the feature label Jul 8, 2019
@@ -212,7 +212,7 @@ val apiModule = module {

val viewModelModule = module {
viewModel { LoginViewModel(get(), get(), get()) }
viewModel { EventsViewModel(get(), get(), get(), get(), get(), get(), get()) }
viewModel { EventsViewModel(get(), get(), get(), get(), get(), get(), get(), get()) }
Copy link
Member

Choose a reason for hiding this comment

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

This is getting too big. Open issue to refactor and break into smaller ViewModels

For e.g, similar event functionality should be in its own ViewModel

Copy link
Member

Choose a reason for hiding this comment

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

Where is the issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Where is the issue?

#2083

Copy link
Member Author

Choose a reason for hiding this comment

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

similar event functionality should be in its own ViewModel

It's not there, and there are only 4-5 methods, the view model is not so long. We can separate authentication related functions only.

Copy link
Member

Choose a reason for hiding this comment

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

So why 8 parameters are required in it?

Copy link
Member Author

Choose a reason for hiding this comment

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

private val eventService: EventService,
    private val preference: Preference,
    private val resource: Resource,
    private val mutableConnectionLiveData: MutableConnectionLiveData,
    private val authHolder: AuthHolder,
    private val authService: AuthService,
    private val notificationService: NotificationService,
    private val config: PagedList.Config

Copy link
Member Author

Choose a reason for hiding this comment

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

Can this be merged, issue #2083 is opened for it, will work on that.

else -> {
bundle.putString(EVENT_IDENTIFIER, appLinkData.lastPathSegment)
navController.navigate(R.id.eventDetailsFragment, bundle)
}
Copy link
Member

Choose a reason for hiding this comment

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

Add a separate class to parse and navigate and add tests

@liveHarshit
Copy link
Member Author

@iamareebjamal Updated

@liveHarshit liveHarshit changed the title feat: Handle app link intent for password reset feat: Handle app link intent for password reset and add unit test Jul 9, 2019
@fossasia fossasia deleted a comment Jul 9, 2019
@fossasia fossasia deleted a comment Jul 9, 2019
@liveHarshit liveHarshit requested a review from anhanh11001 July 9, 2019 13:11
anhanh11001
anhanh11001 previously approved these changes Jul 9, 2019
@liveHarshit
Copy link
Member Author

Updated

@fossasia fossasia deleted a comment Jul 11, 2019
@fossasia fossasia deleted a comment Jul 11, 2019

class AppLinkUtilsTest {

private fun getAppLink(type: String): String {
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the function and directly write URLs, as I don't think there is any repetition

object AppLinkUtils {
fun getData(uri: String): AppLinkData? {
val value = uri.split("/", "?", "=").last()
return when (uri.split("/", "?", "=")[3]) {
Copy link
Member

Choose a reason for hiding this comment

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

Why split 2 times, just do once

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

@iamareebjamal iamareebjamal merged commit 1e35275 into fossasia:development Jul 12, 2019
@liveHarshit liveHarshit deleted the 2079 branch July 22, 2019 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add app link for reset password
4 participants