fix: address PR comments by Hamza

openedx · Feb 2, 2024 · dfe368c · dfe368c
1 parent db8bf49
commit dfe368c
Show file tree

Hide file tree

Showing 14 changed files with 54 additions and 44 deletions.
diff --git a/app/src/main/java/org/openedx/app/AppRouter.kt b/app/src/main/java/org/openedx/app/AppRouter.kt
@@ -80,11 +80,7 @@ class AppRouter : AuthRouter, DiscoveryRouter, DashboardRouter, CourseRouter, Di
         replaceFragmentWithBackStack(fm, NativeDiscoveryFragment.newInstance(querySearch))
     }
 
-    override fun navigateToWebDiscoverCourses(
-        fm: FragmentManager,
-        querySearch: String,
-        isPreLogin: Boolean
-    ) {
+    override fun navigateToWebDiscoverCourses(fm: FragmentManager, querySearch: String) {
         replaceFragmentWithBackStack(fm, WebViewDiscoveryFragment.newInstance(querySearch))
     }
 

diff --git a/app/src/main/java/org/openedx/app/MainFragment.kt b/app/src/main/java/org/openedx/app/MainFragment.kt
@@ -83,17 +83,20 @@ class MainFragment : Fragment(R.layout.fragment_main) {
             }
         }
 
-        requireArguments().let { bundle ->
-            bundle.getString(ARG_COURSE_ID, null)?.apply {
-                val infoType = bundle.getString(ARG_INFO_TYPE, null)
+        requireArguments().run {
+            getString(ARG_COURSE_ID)?.let { courseId ->
+                val infoType = getString(ARG_INFO_TYPE)
+
                 if (config.getDiscoveryConfig().isViewTypeWebView() && infoType != null) {
-                    router.navigateToCourseInfo(parentFragmentManager, this, infoType)
+                    router.navigateToCourseInfo(parentFragmentManager, courseId, infoType)
                 } else {
-                    router.navigateToCourseDetail(parentFragmentManager, this)
+                    router.navigateToCourseDetail(parentFragmentManager, courseId)
                 }
+
+                // Clear arguments after navigation
+                putString(ARG_COURSE_ID, null)
+                putString(ARG_INFO_TYPE, null)
             }
-            bundle.putString(ARG_COURSE_ID, null)
-            bundle.putString(ARG_INFO_TYPE, null)
         }
     }
 

diff --git a/app/src/main/java/org/openedx/app/di/ScreenModule.kt b/app/src/main/java/org/openedx/app/di/ScreenModule.kt
@@ -80,8 +80,8 @@ val screenModule = module {
             infoType,
         )
     }
-    viewModel { (courseId: String?) ->
-        SignUpViewModel(get(), get(), get(), get(), get(), courseId)
+    viewModel { (courseId: String?, infoType: String?) ->
+        SignUpViewModel(get(), get(), get(), get(), get(), courseId, infoType)
     }
     viewModel { RestorePasswordViewModel(get(), get(), get(), get()) }
 

diff --git a/auth/src/main/java/org/openedx/auth/presentation/AuthRouter.kt b/auth/src/main/java/org/openedx/auth/presentation/AuthRouter.kt
@@ -16,11 +16,7 @@ interface AuthRouter {
 
     fun navigateToWhatsNew(fm: FragmentManager, courseId: String? = null, infoType: String? = null)
 
-    fun navigateToWebDiscoverCourses(
-        fm: FragmentManager,
-        querySearch: String,
-        isPreLogin: Boolean = false
-    )
+    fun navigateToWebDiscoverCourses(fm: FragmentManager, querySearch: String)
 
     fun navigateToNativeDiscoverCourses(fm: FragmentManager, querySearch: String)
 

diff --git a/auth/src/main/java/org/openedx/auth/presentation/logistration/LogistrationFragment.kt b/auth/src/main/java/org/openedx/auth/presentation/logistration/LogistrationFragment.kt
@@ -81,8 +81,7 @@ class LogistrationFragment : Fragment() {
                         if (isDiscoveryTypeWebView) {
                             router.navigateToWebDiscoverCourses(
                                 parentFragmentManager,
-                                querySearch,
-                                true
+                                querySearch
                             )
                         } else {
                             router.navigateToNativeDiscoverCourses(

diff --git a/auth/src/main/java/org/openedx/auth/presentation/signup/SignUpFragment.kt b/auth/src/main/java/org/openedx/auth/presentation/signup/SignUpFragment.kt
@@ -104,7 +104,10 @@ import org.openedx.core.ui.windowSizeValue
 class SignUpFragment : Fragment() {
 
     private val viewModel by viewModel<SignUpViewModel> {
-        parametersOf(requireArguments().getString(ARG_COURSE_ID, ""))
+        parametersOf(
+            requireArguments().getString(ARG_COURSE_ID, ""),
+            requireArguments().getString(ARG_INFO_TYPE, "")
+        )
     }
     private val router by inject<AuthRouter>()
 
@@ -148,7 +151,11 @@ class SignUpFragment : Fragment() {
                     LaunchedEffect(successLogin) {
                         if (successLogin == true) {
                             router.clearBackStack(requireActivity().supportFragmentManager)
-                            router.navigateToMain(parentFragmentManager, viewModel.courseId, null)
+                            router.navigateToMain(
+                                parentFragmentManager,
+                                viewModel.courseId,
+                                viewModel.infoType
+                            )
                         }
                     }
                 } else {

diff --git a/auth/src/main/java/org/openedx/auth/presentation/signup/SignUpViewModel.kt b/auth/src/main/java/org/openedx/auth/presentation/signup/SignUpViewModel.kt
@@ -25,6 +25,7 @@ class SignUpViewModel(
     private val preferencesManager: CorePreferences,
     private val appUpgradeNotifier: AppUpgradeNotifier,
     val courseId: String?,
+    val infoType: String?,
 ) : BaseViewModel() {
 
     private val _uiState = MutableLiveData<SignUpUIState>(SignUpUIState.Loading)

diff --git a/core/src/main/java/org/openedx/core/utils/UrlUtils.kt b/core/src/main/java/org/openedx/core/utils/UrlUtils.kt
@@ -5,7 +5,9 @@ import android.content.Intent
 import android.net.Uri
 
 object UrlUtils {
+
     const val QUERY_PARAM_SEARCH = "q"
+
     fun openInBrowser(activity: Context, apiHostUrl: String, url: String) {
         if (url.isEmpty()) {
             return
@@ -30,7 +32,8 @@ object UrlUtils {
      * Ref: https://stackoverflow.com/a/56108097
      *
      * @param url        that needs to update
-     * @param queryParam that needs to remove from the url
+     * @param queryParam that needs to remove from the URL
+     * @return The URL after removing the given params
      */
     private fun removeQueryParameterFromURL(url: String, queryParam: String): String {
         val uri = Uri.parse(url)

diff --git a/course/src/main/java/org/openedx/course/presentation/info/CourseInfoFragment.kt b/course/src/main/java/org/openedx/course/presentation/info/CourseInfoFragment.kt
@@ -109,7 +109,7 @@ class CourseInfoFragment : Fragment() {
                             fragmentManager = requireActivity().supportFragmentManager,
                             courseId = uiState.enrollmentSuccess.get(),
                         )
-                        // to restrict the relaunch of
+                        // Clear after navigation
                         uiState.enrollmentSuccess.set("")
                     }
                 }
@@ -349,7 +349,7 @@ fun CourseInfoScreenPreview() {
             uiState = CourseInfoUIState(
                 initialUrl = "https://www.example.com/",
                 false,
-                AtomicReference("")
+                enrollmentSuccess = AtomicReference("")
             ),
             uiMessage = null,
             uriScheme = "",

diff --git a/course/src/main/java/org/openedx/course/presentation/info/CourseInfoViewModel.kt b/course/src/main/java/org/openedx/course/presentation/info/CourseInfoViewModel.kt
@@ -37,8 +37,6 @@ class CourseInfoViewModel(
     val infoType: String,
 ) : BaseViewModel() {
 
-    private val ARG_PATH_ID = "path_id"
-
     private val _uiState =
         MutableStateFlow(
             CourseInfoUIState(
@@ -76,7 +74,6 @@ class CourseInfoViewModel(
         }
     }
 
-
     fun enrollInACourse(courseId: String) {
         viewModelScope.launch {
             _showAlert.emit(false)
@@ -133,4 +130,8 @@ class CourseInfoViewModel(
     fun navigateToSignIn(fragmentManager: FragmentManager, courseId: String, infoType: String) {
         router.navigateToSignIn(fragmentManager, courseId, infoType)
     }
+
+    companion object {
+        private const val ARG_PATH_ID = "path_id"
+    }
 }
diff --git a/discovery/src/main/java/org/openedx/discovery/presentation/DiscoveryNavigator.kt b/discovery/src/main/java/org/openedx/discovery/presentation/DiscoveryNavigator.kt
@@ -7,9 +7,9 @@ class DiscoveryNavigator(
 ) {
     fun getDiscoveryFragment(): Fragment {
         return if (isDiscoveryTypeWebView) {
-            WebViewDiscoveryFragment.newInstance(querySearch = "")
+            WebViewDiscoveryFragment.newInstance()
         } else {
-            NativeDiscoveryFragment.newInstance(querySearch = "")
+            NativeDiscoveryFragment.newInstance()
         }
     }
 }
diff --git a/discovery/src/main/java/org/openedx/discovery/presentation/NativeDiscoveryFragment.kt b/discovery/src/main/java/org/openedx/discovery/presentation/NativeDiscoveryFragment.kt
@@ -175,7 +175,7 @@ class NativeDiscoveryFragment : Fragment() {
 
     companion object {
         private const val ARG_SEARCH_QUERY = "query_search"
-        fun newInstance(querySearch: String): NativeDiscoveryFragment {
+        fun newInstance(querySearch: String = ""): NativeDiscoveryFragment {
             val fragment = NativeDiscoveryFragment()
             fragment.arguments = bundleOf(
                 ARG_SEARCH_QUERY to querySearch

diff --git a/discovery/src/main/java/org/openedx/discovery/presentation/WebViewDiscoveryFragment.kt b/discovery/src/main/java/org/openedx/discovery/presentation/WebViewDiscoveryFragment.kt
@@ -61,7 +61,6 @@ import org.openedx.core.ui.statusBarsInset
 import org.openedx.core.ui.theme.OpenEdXTheme
 import org.openedx.core.ui.theme.appColors
 import org.openedx.core.ui.windowSizeValue
-import org.openedx.core.utils.UrlUtils
 import org.openedx.discovery.R
 import org.openedx.core.R as CoreR
 
@@ -83,16 +82,10 @@ class WebViewDiscoveryFragment : Fragment() {
                 var hasInternetConnection by remember {
                     mutableStateOf(viewModel.hasInternetConnection)
                 }
-                var discoveryUrl = viewModel.discoveryUrl
-                if (viewModel.querySearch.isNotBlank()) {
-                    val queryParams: MutableMap<String, String> = HashMap()
-                    queryParams[UrlUtils.QUERY_PARAM_SEARCH] = viewModel.querySearch
-                    discoveryUrl = UrlUtils.buildUrlWithQueryParams(discoveryUrl, queryParams)
-                }
                 WebViewDiscoveryScreen(
                     windowSize = windowSize,
                     isPreLogin = viewModel.isPreLogin,
-                    contentUrl = discoveryUrl,
+                    contentUrl = viewModel.discoveryUrl,
                     uriScheme = viewModel.uriScheme,
                     hasInternetConnection = hasInternetConnection,
                     checkInternetConnection = {
@@ -144,8 +137,10 @@ class WebViewDiscoveryFragment : Fragment() {
     }
 
     companion object {
+
         private const val ARG_SEARCH_QUERY = "query_search"
-        fun newInstance(querySearch: String): WebViewDiscoveryFragment {
+
+        fun newInstance(querySearch: String = ""): WebViewDiscoveryFragment {
             val fragment = WebViewDiscoveryFragment()
             fragment.arguments = bundleOf(ARG_SEARCH_QUERY to querySearch)
             return fragment

diff --git a/discovery/src/main/java/org/openedx/discovery/presentation/WebViewDiscoveryViewModel.kt b/discovery/src/main/java/org/openedx/discovery/presentation/WebViewDiscoveryViewModel.kt
@@ -5,24 +5,33 @@ import org.openedx.core.BaseViewModel
 import org.openedx.core.config.Config
 import org.openedx.core.data.storage.CorePreferences
 import org.openedx.core.system.connection.NetworkConnection
+import org.openedx.core.utils.UrlUtils
 
 class WebViewDiscoveryViewModel(
     private val config: Config,
     private val networkConnection: NetworkConnection,
     private val corePreferences: CorePreferences,
     private val router: DiscoveryRouter,
-    val querySearch: String,
+    private val querySearch: String,
 ) : BaseViewModel() {
 
     val uriScheme: String get() = config.getUriScheme()
 
-    val webViewConfig get() = config.getDiscoveryConfig().webViewConfig
+    private val webViewConfig get() = config.getDiscoveryConfig().webViewConfig
 
     val isPreLogin get() = config.isPreLoginExperienceEnabled() && corePreferences.user == null
 
     private var _discoveryUrl = webViewConfig.baseUrl
     val discoveryUrl: String
-        get() = _discoveryUrl
+        get() {
+            return if (querySearch.isNotBlank()) {
+                val queryParams: MutableMap<String, String> = HashMap()
+                queryParams[UrlUtils.QUERY_PARAM_SEARCH] = querySearch
+                UrlUtils.buildUrlWithQueryParams(_discoveryUrl, queryParams)
+            } else {
+                _discoveryUrl
+            }
+        }
 
     val hasInternetConnection: Boolean
         get() = networkConnection.isOnline()