feat: Program Screen Error Handling #448

shafqat-muneer · 2024-06-03T06:07:04Z

LEARNER-9992: feat: Program Screen Error Handling

Light Mode	Dark Mode

saeedbashir

Discovery is not showing the full screen erros
It would be nice if it central align. and any reason to hide reload button here?

saeedbashir · 2024-06-06T02:06:09Z

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

@@ -17,6 +17,8 @@ public protocol WebViewNavigationDelegate: AnyObject {
        shouldLoad request: URLRequest,
        navigationAction: WKNavigationAction
    ) async -> Bool
+
+    func showWebViewError()


It nice and can work with passing the error, but how about having a more detailed method like

func webView( _ webView: WKWebView, didFail navigation: WKNavigation, withError error: Error )

Thoughts?

In our use case, displaying specific error details is not required. We are currently only showing internet error or generic error. Therefore, detailed information is not required at this time. We can reconsider this approach if there arises a need for more detailed error messages in the future.

saeedbashir · 2024-06-06T02:09:19Z

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

@@ -189,7 +199,14 @@ public struct WebView: UIViewRepresentable {

        @objc private func reload() {
            parent.isLoading = true
-            webview?.reload()
+            if webview?.url?.absoluteString.isEmpty ?? true {
+                if let url = URL(string: parent.viewModel.url) {


What will happen if this condition fails? will it reload the webview?

Thank you for bringing this in attention. It’s fixed.

saeedbashir · 2024-06-06T02:15:22Z

Course/Course.xcodeproj/project.pbxproj

@@ -526,13 +525,20 @@
 			path = Mock;
 			sourceTree = "<group>";
 		};
-		97CA95212B875EA200A9EDEA /* Views */ = {
+		9784D4752BF39EEF00AFEFFF /* CalendarSyncProgressView */ = {


Any reason to update this? Asking because it doesn't seem relevant to the work.

Updated location from:

Course/Views/CalendarSyncProgressView.swift Course/Views/DatesSuccessView.swift

To:

Course/Presentation/Subviews/CalendarSyncProgressView/CalendarSyncProgressView.swift Course/Presentation/Subviews/DatesSuccessView/DatesSuccessView.swift

I found that we already have subviews folder and it seems more appropriate to place it there.

shafqat-muneer · 2024-06-06T13:28:05Z

@saeedbashir Feedback addressed. Ready for another round of review 🎉

rnr · 2024-06-07T12:12:47Z

Core/Core/View/Base/FullScreenErrorView/FullScreenErrorView.swift

+                        .scaledToFit()
+
+                    Text(CoreLocalization.Error.Internet.noInternetTitle)
+                                    .font(Theme.Fonts.titleLarge)


too big indent

rnr · 2024-06-07T12:13:26Z

Core/Core/View/Base/FullScreenErrorView/FullScreenErrorView.swift

+                        .scaledToFit()
+
+                    Text(CoreLocalization.View.Snackbar.tryAgainBtn)
+                                    .font(Theme.Fonts.titleLarge)


fix indent here please

rnr · 2024-06-07T12:14:43Z

Core/Core/View/Base/FullScreenErrorView/FullScreenErrorView.swift

+}
+
+#if DEBUG
+//swiftlint:disable all


which swiftlint's rules we disable here?

Thanks for bringing that to my attention. I've removed it.

rnr · 2024-06-07T12:25:44Z

Discovery/Discovery/Presentation/WebPrograms/ProgramWebviewView.swift

@@ -114,3 +130,25 @@ public struct ProgramWebviewView: View {
        .animation(.default, value: viewModel.showError)
    }
 }
+
+#if DEBUG
+//swiftlint:disable all


why do we need to disable swiftlint here?

Thanks for bringing that to my attention. I've removed it.

rnr · 2024-06-07T13:04:27Z

@moiz994 do we need this yellow line on Programs and Discover screens?

rnr · 2024-06-07T13:07:48Z

@shafqat-muneer
When the connection is restored and we get the data on the Program screen (for example) and then go to the Discover screen, there we see a different error ("something went wrong" instead of "no connection"). Is this expected?

RPReplay_Final1717765154.MOV

shafqat-muneer · 2024-06-13T13:51:09Z

@moiz994 do we need this yellow line on Programs and Discover screens?

It appears to be redundant after implementation of full screen error. Just removed it from Programs and Discovery screens.

shafqat-muneer · 2024-06-20T10:53:50Z

@shafqat-muneer When the connection is restored and we get the data on the Program screen (for example) and then go to the Discover screen, there we see a different error ("something went wrong" instead of "no connection"). Is this expected?

RPReplay_Final1717765154.MOV

@rnr It's fixed.

shafqat-muneer · 2024-06-20T10:54:41Z

@rnr @saeedbashir PR is prepared for another round of review. 🎉

saeedbashir

LGTM 👍

shafqat-muneer · 2024-06-21T10:33:26Z

@volodymyr-chekyrta This change is awaiting your review before it can be merged. Thanks

volodymyr-chekyrta · 2024-06-25T10:25:02Z

Core/Core/View/Base/FullScreenErrorView/FullScreenErrorView.swift

+
+    public init(
+        type: ErrorType,
+        reloadAction: @escaping () -> Void


Could we generalize it for the future and call it just action?

Yes, it's updated with a more generic name.

volodymyr-chekyrta · 2024-06-25T10:28:03Z

Core/Core/View/Base/FullScreenErrorView/FullScreenErrorView.swift

+                    UnitButtonView(type: .reload, action: {
+                        self.reloadAction()
+                    })


formatting

UnitButtonView( type: .reload, action: { self.reloadAction() } )

volodymyr-chekyrta · 2024-06-25T10:29:51Z

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

+            DispatchQueue.main.async {
+                self.parent.isLoading = false
+            }
+            parent.webViewNavDelegate?.showWebViewError()


what about

DispatchQueue.main.async { self.parent.isLoading = false self.parent.webViewNavDelegate?.showWebViewError() }

and remove DispatchQueue.main from the showWebViewError function.
wdyt?

Agreed, it's more appropriate I believe. Thanks

shafqat-muneer · 2024-06-25T11:49:28Z

@volodymyr-chekyrta Incorporated the suggestions. Ready for another review. 🎉

volodymyr-chekyrta

LGTM 👍

shafqat-muneer requested a review from saeedbashir June 3, 2024 06:07

feat: Program Screen Error Handling

aaf0cee

shafqat-muneer force-pushed the Shafqat/LEARNER-9992 branch from d8602f5 to aaf0cee Compare June 5, 2024 16:24

saeedbashir requested changes Jun 6, 2024

View reviewed changes

shafqat-muneer requested review from rnr and volodymyr-chekyrta June 6, 2024 07:45

chore: address review feedback

d37230f

rnr reviewed Jun 7, 2024

View reviewed changes

shafqat-muneer added 2 commits June 13, 2024 18:51

chore: address review feedback

f6472c2

chore: address review feedback

25e374f

saeedbashir previously approved these changes Jun 21, 2024

View reviewed changes

rnr previously approved these changes Jun 21, 2024

View reviewed changes

shafqat-muneer added 2 commits June 24, 2024 17:51

Merge remote-tracking branch 'origin/develop' into Shafqat/LEARNER-9992

e766263

chore: resolved conflicts

8002284

shafqat-muneer dismissed stale reviews from rnr and saeedbashir via 8002284 June 24, 2024 12:56

rnr previously approved these changes Jun 24, 2024

View reviewed changes

volodymyr-chekyrta reviewed Jun 25, 2024

View reviewed changes

chore: address review feedback

954d41b

shafqat-muneer dismissed rnr’s stale review via 954d41b June 25, 2024 11:43

volodymyr-chekyrta approved these changes Jun 26, 2024

View reviewed changes

shafqat-muneer merged commit ea6e24e into openedx:develop Jun 26, 2024
3 checks passed

shafqat-muneer deleted the Shafqat/LEARNER-9992 branch June 26, 2024 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Program Screen Error Handling #448

feat: Program Screen Error Handling #448

shafqat-muneer commented Jun 3, 2024

saeedbashir left a comment

saeedbashir Jun 6, 2024

shafqat-muneer Jun 6, 2024

saeedbashir Jun 6, 2024

shafqat-muneer Jun 6, 2024

saeedbashir Jun 6, 2024

shafqat-muneer Jun 6, 2024

shafqat-muneer commented Jun 6, 2024

rnr Jun 7, 2024

rnr Jun 7, 2024

rnr Jun 7, 2024

shafqat-muneer Jun 13, 2024

rnr Jun 7, 2024

shafqat-muneer Jun 13, 2024

rnr commented Jun 7, 2024

rnr commented Jun 7, 2024

shafqat-muneer commented Jun 13, 2024

shafqat-muneer commented Jun 20, 2024

shafqat-muneer commented Jun 20, 2024

saeedbashir left a comment

shafqat-muneer commented Jun 21, 2024

volodymyr-chekyrta Jun 25, 2024

shafqat-muneer Jun 25, 2024

volodymyr-chekyrta Jun 25, 2024

shafqat-muneer Jun 25, 2024

volodymyr-chekyrta Jun 25, 2024

shafqat-muneer Jun 25, 2024

shafqat-muneer commented Jun 25, 2024

volodymyr-chekyrta left a comment

feat: Program Screen Error Handling #448

feat: Program Screen Error Handling #448

Conversation

shafqat-muneer commented Jun 3, 2024

saeedbashir left a 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

shafqat-muneer commented Jun 6, 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

rnr commented Jun 7, 2024

rnr commented Jun 7, 2024

shafqat-muneer commented Jun 13, 2024

shafqat-muneer commented Jun 20, 2024

shafqat-muneer commented Jun 20, 2024

saeedbashir left a comment

Choose a reason for hiding this comment

shafqat-muneer commented Jun 21, 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 commented Jun 25, 2024

volodymyr-chekyrta left a comment

Choose a reason for hiding this comment