feat: webview based discovery implementation #219

saeedbashir · 2024-01-08T10:29:23Z

This PR adds support for web-view-based discovery. The switch between native discovery and web-view-based discovery is being controlled from the config file.

Note:
By default, the discovery will be disabled because some providers don't want to provide discovery to their users.

Sample Config Foe WebDiscovery:

DISCOVERY:
    TYPE: 'native'

Sample Config For WebDiscovery:

DISCOVERY:
    TYPE: 'webview'
    WEBVIEW:
        BASE_URL: 'base discovery url'
        COURSE_DETAIL_TEMPLATE: 'discovery detail template URL'
        PROGRAM_DETAIL_TEMPLATE: 'program detail template URL'

Working Video:

web.discovery.mov

rnr · 2024-01-08T17:31:46Z

@saeedbashir could you please provide properly configured config with added parameters to test this? Thank you

saeedbashir · 2024-01-09T05:02:28Z

@saeedbashir could you please provide properly configured config with added parameters to test this? Thank you

@rnr Our prod config is already configured for the web view based discovery. You can use that to test it.

…adding in discovery

rnr · 2024-01-10T12:35:43Z

Hello @saeedbashir
I am facing a problem while testing the Discovery web view. The first time it works, but when I go to Dashboard and go back to Discovery, it stops working. I tested on the device and on the simulator. Please look on video below.

Screen.Recording.2024-01-10.at.13.31.11.mov

saeedbashir · 2024-01-10T15:04:33Z

Hello @saeedbashir I am facing a problem while testing the Discovery web view. The first time it works, but when I go to Dashboard and go back to Discovery, it stops working. I tested on the device and on the simulator. Please look on video below.

Screen.Recording.2024-01-10.at.13.31.11.mov

@rnr Can you please try it now?

rnr · 2024-01-11T10:28:55Z

Hello @saeedbashir I am facing a problem while testing the Discovery web view. The first time it works, but when I go to Dashboard and go back to Discovery, it stops working. I tested on the device and on the simulator. Please look on video below.
Screen.Recording.2024-01-10.at.13.31.11.mov

@rnr Can you please try it now?

@saeedbashir now it's working fine. thank you

rnr · 2024-01-11T10:36:03Z

Core/Core/View/Base/WebView.swift

@@ -139,8 +188,8 @@ public struct WebView: UIViewRepresentable {
        webView.backgroundColor = .clear
        webView.scrollView.backgroundColor = Theme.Colors.white.uiColor()
        webView.scrollView.alwaysBounceVertical = false
-        webView.scrollView.layer.cornerRadius = 24
-        webView.scrollView.layer.maskedCorners = [.layerMinXMinYCorner, .layerMaxXMinYCorner]
+//        webView.scrollView.layer.cornerRadius = 24


@saeedbashir do we need this commented out code?

rnr · 2024-01-11T10:43:54Z

Core/Core/View/Base/WebView.swift

+            let isWebViewDelegateHandled = await (
+                parent.webViewNavDelegate?.webView(
+                    webView,
+                    shouldLoad: navigationAction.request, navigationAction: navigationAction) ?? false


could you follow style 'each argument on its own line' here please

rnr · 2024-01-11T10:52:00Z

Discovery/Discovery/Presentation/WebDiscovery/DiscoverWebviewModel.swift

+    var sourceScreen: LogistrationSourceScreen
+
+    var userloggedIn: Bool {
+        return !(storage.user?.username?.isEmpty ?? true)


isn't storage.user?.username?.isEmpty == false more clear here?

rnr · 2024-01-11T11:01:00Z

Discovery/Discovery/Presentation/DiscoveryRouter.swift

    func showCourseDetais(courseID: String, title: String)
+    func showWebDiscoveryDetails(pathID: String, discoveryType: DiscoveryWebviewType, sourceScreen: LogistrationSourceScreen)


fix please SwiftLint line length warning here

rnr · 2024-01-11T11:01:07Z

Discovery/Discovery/Presentation/DiscoveryRouter.swift

@@ -23,8 +32,18 @@ public class DiscoveryRouterMock: BaseRouterMock, DiscoveryRouter {
    public override init() {}

    public func showCourseDetais(courseID: String, title: String) {}
+    public func showWebDiscoveryDetails(pathID: String, discoveryType: DiscoveryWebviewType, sourceScreen: LogistrationSourceScreen) {}


fix please SwiftLint line length warning here

rnr · 2024-01-11T11:10:37Z

Discovery/Discovery/Presentation/WebDiscovery/URL+PathExtension.swift

+    }
+
+    var isValidAppURLScheme: Bool {
+        return scheme ?? "" == URIString.appURLScheme.rawValue ? true : false


doesn't just scheme ?? "" == URIString.appURLScheme.rawValue work here?

empty scheme ("") is valid too here (in case URIString.appURLScheme gets empty someway)?
maybe need to add && scheme?.isEmpty == false

empty scheme ("") is valid too here (in case URIString.appURLScheme gets empty someway)?

It's a defined value, so it shouldn't be needed to address it at the moment.

rnr · 2024-01-11T11:21:39Z

Discovery/Discovery/en.lproj/Localizable.strings

+"ALERT.LEAVING_APP_TITLE" = "Leaving the app";
+"ALERT.LEAVING_APP_MESSAGE" = "You are now leaving the app and opening a browser";
+
+"DETAILS.TITLE" = "Course details";


please add these to uk.lproj too

rnr · 2024-01-11T11:28:39Z

OpenEdX/Data/DiscoveryPersistence.swift

+        let request = CDCourseDetails.fetchRequest()
+        request.predicate = NSPredicate(format: "courseID = %@", courseID)
+        guard let courseDetails = try? context.fetch(request).first else { throw NoCachedDataError() }
+        return CourseDetails(courseID: courseDetails.courseID ?? "",


please format to have parameter on its own line

rnr · 2024-01-11T11:30:49Z

OpenEdX/Router.swift

+        let view = DiscoveryWebview(
+            viewModel: Container.shared.resolve(
+                DiscoveryWebviewModel.self,
+                argument: sourceScreen)!,


I guess it's fine because having the ( brackets of the argument on the next line breaks the arguments flow.

rnr · 2024-01-11T11:32:13Z

OpenEdX/View/MainScreenView.swift

+                        DiscoveryWebview(
+                            viewModel: Container.shared.resolve(
+                                DiscoveryWebviewModel.self,
+                                argument: viewModel.sourceScreen)!,


#219 (comment)

volodymyr-chekyrta · 2024-01-15T08:59:31Z

@saeedbashir, could you please fix the tests?
Here is the log from CI.

Testing failed:
	Expected expression after operator
	Command SwiftCompile failed with a nonzero exit code
	Testing cancelled because the build failed.

** TEST FAILED **


The following build commands failed:
	SwiftCompile normal x86_64 Compiling\ DiscoveryViewModel.swift,\ DiscoveryInteractor.swift,\ DiscoveryView.swift,\ CourseDetailsViewModel.swift,\ DiscoveryRouter.swift,\ URL+PathExtension.swift,\ Data_CourseDetailsResponse.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/NativeDiscovery/DiscoveryViewModel.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Domain/DiscoveryInteractor.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/NativeDiscovery/DiscoveryView.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/NativeDiscovery/CourseDetailsViewModel.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/DiscoveryRouter.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/WebDiscovery/URL+PathExtension.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Data/Model/Data_CourseDetailsResponse.swift (in target 'Discovery' from project 'Discovery')
	SwiftCompile normal x86_64 /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/WebDiscovery/URL+PathExtension.swift (in target 'Discovery' from project 'Discovery')
	SwiftCompile normal x86_64 Compiling\ UpdateRecommendedView.swift,\ CourseDetails.swift,\ DiscoveryEndpoint.swift,\ UpdateRequiredView.swift,\ SearchViewModel.swift,\ CourseDetailsView.swift,\ DiscoveryURIDetails.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/UpdateViews/UpdateRecommendedView.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Data/Model/CourseDetails.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Data/Network/DiscoveryEndpoint.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/UpdateViews/UpdateRequiredView.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/NativeDiscovery/SearchViewModel.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/NativeDiscovery/CourseDetailsView.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/WebDiscovery/DiscoveryURIDetails.swift (in target 'Discovery' from project 'Discovery')
(3 failures)
[08:08:11]: Exit status: 65

rnr · 2024-01-15T10:47:56Z

@saeedbashir, could you please fix the tests? Here is the log from CI.

Testing failed:
	Expected expression after operator
	Command SwiftCompile failed with a nonzero exit code
	Testing cancelled because the build failed.

** TEST FAILED **


The following build commands failed:
	SwiftCompile normal x86_64 Compiling\ DiscoveryViewModel.swift,\ DiscoveryInteractor.swift,\ DiscoveryView.swift,\ CourseDetailsViewModel.swift,\ DiscoveryRouter.swift,\ URL+PathExtension.swift,\ Data_CourseDetailsResponse.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/NativeDiscovery/DiscoveryViewModel.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Domain/DiscoveryInteractor.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/NativeDiscovery/DiscoveryView.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/NativeDiscovery/CourseDetailsViewModel.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/DiscoveryRouter.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/WebDiscovery/URL+PathExtension.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Data/Model/Data_CourseDetailsResponse.swift (in target 'Discovery' from project 'Discovery')
	SwiftCompile normal x86_64 /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/WebDiscovery/URL+PathExtension.swift (in target 'Discovery' from project 'Discovery')
	SwiftCompile normal x86_64 Compiling\ UpdateRecommendedView.swift,\ CourseDetails.swift,\ DiscoveryEndpoint.swift,\ UpdateRequiredView.swift,\ SearchViewModel.swift,\ CourseDetailsView.swift,\ DiscoveryURIDetails.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/UpdateViews/UpdateRecommendedView.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Data/Model/CourseDetails.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Data/Network/DiscoveryEndpoint.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/UpdateViews/UpdateRequiredView.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/NativeDiscovery/SearchViewModel.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/NativeDiscovery/CourseDetailsView.swift /Users/runner/work/openedx-app-ios/openedx-app-ios/Discovery/Discovery/Presentation/WebDiscovery/DiscoveryURIDetails.swift (in target 'Discovery' from project 'Discovery')
(3 failures)
[08:08:11]: Exit status: 65

@saeedbashir project isn't compilable after fix (there is misspelling in URL+PathExtension.swift)

Discovery/Discovery/Presentation/WebDiscovery/URL+PathExtension.swift

Discovery/Discovery/Presentation/WebDiscovery/DiscoverWebviewModel.swift

touchapp · 2024-01-16T13:21:46Z

Hi @volodymyr-chekyrta any other info needed to review this PR?

volodymyr-chekyrta · 2024-01-16T13:25:39Z

Hi @volodymyr-chekyrta any other info needed to review this PR?

Hi @touchapp, I'll do my best to finish review today

Discovery/Discovery/Presentation/WebDiscovery/DiscoveryURIDetails.swift

Core/Core/View/Base/Webview/WebView.swift

shafqat-muneer · 2024-01-17T11:41:11Z

Core/Core/uk.lproj/Localizable.strings

@@ -69,6 +69,7 @@

 "WEBVIEW.ALERT.OK" = "Так";
 "WEBVIEW.ALERT.CANCEL" = "Скасувати";
+"WEBVIEW.ALERT.CONTINUE" = "Continue";


Is it necessary to have a localized version of the string at this moment?

We don't add translations on the go. Files will be translated later.

shafqat-muneer · 2024-01-17T11:53:52Z

Discovery/Discovery/Data/DiscoveryRepository.swift

+        if enroll.statusCode == 200 {
+            return true
+        } else {
+            return false
+        }


We can refactor it like:

return enroll.statusCode == 200

shafqat-muneer · 2024-01-17T11:59:33Z

Discovery/Discovery/Data/Network/DiscoveryEndpoint.swift


    var path: String {
        switch self {
        case .getDiscovery:
            return "/api/courses/v1/courses/"
        case .searchCourses:
            return "/api/courses/v1/courses/"
+        case .getCourseDetail(courseID: let courseID, _):
+            return "/api/courses/v1/courses/\(courseID)"
+        case .enrollToCourse(courseID: _):


Getting warning in code:

Empty Enum Arguments Violation: Arguments can be omitted when matching enums with associated values if they are not used (empty_enum_arguments)

shafqat-muneer · 2024-01-17T12:00:05Z

Discovery/Discovery/Data/Network/DiscoveryEndpoint.swift

        }
    }

    var httpMethod: HTTPMethod {
        switch self {
        case .getDiscovery, .searchCourses:
            return .get
+        case .getCourseDetail(courseID: _, username: _):


Same warning here:

Empty Enum Arguments Violation: Arguments can be omitted when matching enums with associated values if they are not used (empty_enum_arguments)

shafqat-muneer · 2024-01-17T12:09:37Z

Discovery/Discovery/Presentation/WebDiscovery/URL+PathExtension.swift

+    }
+
+    var isValidAppURLScheme: Bool {
+        return scheme ?? "" == URIString.appURLScheme.rawValue 


Please address warning:

Trailing Whitespace Violation: Lines should not have trailing whitespace (trailing_whitespace)

shafqat-muneer

LGTM 👍

feat: webview discovery implimentation

d656376

saeedbashir force-pushed the saeed/web_discovery branch from 3f4fef5 to d656376 Compare January 8, 2024 10:30

saeedbashir changed the title ~~chore: webview based discovery~~ chore: webview based discovery implementation Jan 8, 2024

saeedbashir requested review from shafqat-muneer, volodymyr-chekyrta and rnr and removed request for volodymyr-chekyrta January 8, 2024 10:36

fix: update framework settings

c542202

saeedbashir added 3 commits January 10, 2024 09:59

chore: moving course detail related code to Discovery framwrok

634b73b

chore: regenerating tests

6666cb8

refactor: moving course detail tests to discovery framework

cec1608

saeedbashir requested a review from volodymyr-chekyrta January 10, 2024 06:08

refactor: mistankly course detail tests was added in discussion, now …

f19b0e6

…adding in discovery

fix: fix the unresponsive clicks of webview

4779971

rnr reviewed Jan 11, 2024

View reviewed changes

refactor: address review feedback

80c7799

saeedbashir requested a review from rnr January 15, 2024 07:58

rnr requested changes Jan 15, 2024

View reviewed changes

Discovery/Discovery/Presentation/WebDiscovery/URL+PathExtension.swift Outdated Show resolved Hide resolved

fix: fix typo

370d2c7

rnr previously approved these changes Jan 15, 2024

View reviewed changes

volodymyr-chekyrta reviewed Jan 15, 2024

View reviewed changes

Discovery/Discovery/Presentation/WebDiscovery/DiscoverWebviewModel.swift Outdated Show resolved Hide resolved

volodymyr-chekyrta reviewed Jan 15, 2024

View reviewed changes

Discovery/Discovery/Presentation/WebDiscovery/DiscoverWebviewModel.swift Outdated Show resolved Hide resolved

refactor: rename class

8d28140

saeedbashir dismissed rnr’s stale review via 8d28140 January 15, 2024 16:31

fix: add the missing import

4cc33df

saeedbashir requested a review from volodymyr-chekyrta January 15, 2024 16:35

saeedbashir changed the title ~~chore: webview based discovery implementation~~ feat: webview based discovery implementation Jan 16, 2024

This was linked to issues Jan 16, 2024

[iOS] Pre-Login Mobile App Exploration #109

Open

[iOS] Webview Discovery Experience #184

Open

volodymyr-chekyrta reviewed Jan 16, 2024

View reviewed changes

Discovery/Discovery/Presentation/WebDiscovery/DiscoveryURIDetails.swift Outdated Show resolved Hide resolved

refactor: removing extra space

4c9ab03

saeedbashir requested a review from volodymyr-chekyrta January 16, 2024 14:06

Merge branch 'develop' into saeed/web_discovery

86205d5

rnr removed a link to an issue Jan 16, 2024

[iOS] Pre-Login Mobile App Exploration #109

Open

volodymyr-chekyrta reviewed Jan 16, 2024

View reviewed changes

Core/Core/View/Base/Webview/WebView.swift Outdated Show resolved Hide resolved

refactor: move notication name to central place

acec545

saeedbashir requested a review from volodymyr-chekyrta January 17, 2024 05:39

volodymyr-chekyrta previously approved these changes Jan 17, 2024

View reviewed changes

rnr previously approved these changes Jan 17, 2024

View reviewed changes

shafqat-muneer requested changes Jan 17, 2024

View reviewed changes

refactor: address review feedback

574b74f

saeedbashir dismissed stale reviews from rnr and volodymyr-chekyrta via 574b74f January 17, 2024 13:05

saeedbashir requested a review from shafqat-muneer January 17, 2024 13:05

shafqat-muneer approved these changes Jan 17, 2024

View reviewed changes

saeedbashir merged commit 06f0993 into openedx:develop Jan 17, 2024
3 checks passed

saeedbashir deleted the saeed/web_discovery branch January 17, 2024 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: webview based discovery implementation #219

feat: webview based discovery implementation #219

saeedbashir commented Jan 8, 2024 •

edited

Loading

rnr commented Jan 8, 2024

saeedbashir commented Jan 9, 2024

rnr commented Jan 10, 2024

saeedbashir commented Jan 10, 2024

rnr commented Jan 11, 2024

rnr Jan 11, 2024

rnr Jan 11, 2024

rnr Jan 11, 2024

rnr Jan 11, 2024

rnr Jan 11, 2024

rnr Jan 11, 2024

rnr Jan 11, 2024

saeedbashir Jan 15, 2024

rnr Jan 11, 2024

saeedbashir Jan 15, 2024

rnr Jan 11, 2024

rnr Jan 11, 2024

saeedbashir Jan 15, 2024

rnr Jan 11, 2024

saeedbashir Jan 15, 2024

volodymyr-chekyrta commented Jan 15, 2024

rnr commented Jan 15, 2024

touchapp commented Jan 16, 2024

volodymyr-chekyrta commented Jan 16, 2024

shafqat-muneer Jan 17, 2024

saeedbashir Jan 17, 2024

shafqat-muneer Jan 17, 2024

shafqat-muneer Jan 17, 2024

shafqat-muneer Jan 17, 2024

shafqat-muneer Jan 17, 2024

shafqat-muneer left a comment

		func showCourseDetais(courseID: String, title: String)
		func showWebDiscoveryDetails(pathID: String, discoveryType: DiscoveryWebviewType, sourceScreen: LogistrationSourceScreen)

feat: webview based discovery implementation #219

feat: webview based discovery implementation #219

Conversation

saeedbashir commented Jan 8, 2024 • edited Loading

rnr commented Jan 8, 2024

saeedbashir commented Jan 9, 2024

rnr commented Jan 10, 2024

saeedbashir commented Jan 10, 2024

rnr commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volodymyr-chekyrta commented Jan 15, 2024

rnr commented Jan 15, 2024

touchapp commented Jan 16, 2024

volodymyr-chekyrta commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shafqat-muneer left a comment

Choose a reason for hiding this comment

saeedbashir commented Jan 8, 2024 •

edited

Loading