feat: pre-login mobile app exploration #139

saeedbashir · 2023-10-30T02:48:58Z

Description

This PR add's support for pre-login exploration.

Whats Added/Removed

A new startup screen has been added that will be shown when the user is not signed in or Log out
User can go to signed In screen
User can go to register screen
User can go to discovery screen
User can search a course directly from the start screen
A link to register screen from sign in screen is removed

Here is a screen recording of the feature.

startup.screen.mp4

Note: In the second phase we will be showing the Signin and Register buttons on the discovery and course about pages and handling the relevant edge cases, like Enroll button click will redirect the user to register screen if there won't be a session.

codecov · 2023-10-31T08:52:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (develop@1296cb9). Click here to learn what that means.

❗ Current head 06cf408 differs from pull request most recent head 3d31b0c. Consider uploading reports for the commit 3d31b0c to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##             develop   #139   +/-   ##
========================================
  Coverage           ?      0           
========================================
  Files              ?      0           
  Lines              ?      0           
  Branches           ?      0           
========================================
  Hits               ?      0           
  Misses             ?      0           
  Partials           ?      0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mumer92 · 2023-11-08T10:44:29Z

@saeedbashir please resolve the conflicts.

OpenEdX/RouteController.swift

Authorization/Authorization/Presentation/Startup/StartupViewModel.swift

…cs in future

saeedbashir · 2023-11-22T05:42:04Z

@rnr @volodymyr-chekyrta Can you guys please review this PR? it's resulting in conflicts again and again.

volodymyr-chekyrta · 2023-11-22T10:55:03Z

Authorization/Authorization/Presentation/Login/SignInView.swift

@@ -29,6 +30,20 @@ public struct SignInView: View {
                    .resizable()
                    .edgesIgnoringSafeArea(.top)
            }.frame(maxWidth: .infinity, maxHeight: 200)
+            let config = Container.shared.resolve(ConfigProtocol.self)


Doesn't this break the Preview?
It would be better to pass dependencies to SignInView init or ViewModel

Done, now using it from the viewModel.

volodymyr-chekyrta · 2023-11-22T11:05:28Z

Profile/Profile/Presentation/Profile/ProfileViewModel.swift

-        try? await interactor.logOut()
-        router.showLoginScreen()
-        analytics.userLogout(force: false)
+        do {
+            try await interactor.logOut()
+            router.showStartupScreen()
+            analytics.userLogout(force: false)
+        } catch let error {
+            if error.isInternetError {
+                errorMessage = CoreLocalization.Error.slowOrNoInternetConnection
+            } else {
+                errorMessage = CoreLocalization.Error.unknownError
+            }
+        }


It's better to keep that logic; it's done on purpose.
We must ignore server response for logout for offline and deprecated version cases.

I'm not sure how this gets changed. I'd only change the method. Reverted the code.

Got it. Maybe some merge issue. idk

volodymyr-chekyrta · 2023-11-22T11:07:21Z

OpenEdX/View/MainScreenView.swift

@@ -41,7 +41,7 @@ struct MainScreenView: View {
    var body: some View {
        TabView(selection: $selection) {
            ZStack {
-                DiscoveryView(viewModel: Container.shared.resolve(DiscoveryViewModel.self)!)
+                DiscoveryView(viewModel: Container.shared.resolve(DiscoveryViewModel.self)!, router: Container.shared.resolve(DiscoveryRouter.self)!)


133 Characters line.
We have to limit it to 80-120 characters.

Let's format it to

DiscoveryView( viewModel: Container.shared.resolve(DiscoveryViewModel.self)!, router: Container.shared.resolve(DiscoveryRouter.self)! )

We are partially trying to follow this convention
https://google.github.io/swift/#function-calls
When a function call is line-wrapped, each argument is written on its own line.

I know we have places in the project with different formatting, but I hope to follow this one for all new written code.

Could you please format it to this style?

btw, new XCode code formatting Control + M also format code this way

Thanks for the tip but I guess the way Control + M is formatting we are not following that format in the code base. This is how Control + M formatted the above lines

Yeah, you're right; it's doing a bit too much 😂

But just the main point that I wanted to mention is
When a function call is line-wrapped, each argument is written on its own line.
@rnr @eyatsenkoperpetio FYI

Discovery/Discovery/Presentation/SearchView.swift

saeedbashir · 2023-11-22T14:18:29Z

Yes, it's about the script, please check that you have this commit 1296cb9

Done

volodymyr-chekyrta · 2023-11-22T14:29:04Z

I need a little more time for review; I have a few concerns, trying to understand them.

@saeedbashir, could you please help me? Seems I'm missing something.
Why do you open the Discovery screen to open the Search screen rather than just opening the Search screen?
I'm not entirely sure, it may seem like an unnecessary extra step, but I don't want to risk misunderstanding the concept.

saeedbashir · 2023-11-23T06:09:24Z

Why do you open the Discovery screen to open the Search screen rather than just opening the Search screen?

@volodymyr-chekyrta It's because I'm building the whole flow so that when the user comes back from the search, the discovery will be there.

volodymyr-chekyrta · 2023-11-23T08:55:10Z

Why do you open the Discovery screen to open the Search screen rather than just opening the Search screen?

@volodymyr-chekyrta It's because I'm building the whole flow so that when the user comes back from the search, the discovery will be there.

Android appears to have adopted a similar flow, so is this a typical user experience expectation?

saeedbashir · 2023-11-24T05:58:25Z

Why do you open the Discovery screen to open the Search screen rather than just opening the Search screen?

@volodymyr-chekyrta It's because I'm building the whole flow so that when the user comes back from the search, the discovery will be there.

Android appears to have adopted a similar flow, so is this a typical user experience expectation?

Yes.

volodymyr-chekyrta · 2023-11-24T08:41:33Z

Why do you open the Discovery screen to open the Search screen rather than just opening the Search screen?

@volodymyr-chekyrta It's because I'm building the whole flow so that when the user comes back from the search, the discovery will be there.

Android appears to have adopted a similar flow, so is this a typical user experience expectation?

Yes.

Thank you, got it. I'll provide a few more comments based on this flow

volodymyr-chekyrta · 2023-11-24T08:51:08Z

Discovery/Discovery/Presentation/DiscoveryViewModel.swift

@@ -19,6 +19,7 @@ public class DiscoveryViewModel: ObservableObject {

    @Published var courses: [CourseItem] = []
    @Published var showError: Bool = false
+    @Published var searchQuery: String?


@published is redundant here because this variable does not affect UI changes.
Btw, the DiscoveryViewModel doesn't use this; maybe we can move it to the DiscoveryView?

Done. moved to DiscoveryView.

Discovery/Discovery/Presentation/DiscoveryView.swift

OpenEdX/Router.swift

Authorization/Authorization/Presentation/Startup/LogistrationBottomView.swift

Core/Core/Configuration/Config.swift

saeedbashir · 2023-11-24T11:53:42Z

@volodymyr-chekyrta I've made all the highlighted changes, It's ready for another pass.

volodymyr-chekyrta · 2023-11-30T09:10:16Z

The code looks good to me 👍
But I found one bug or part of missed logic.

When trying to enroll in a course, the user sees an error message.
I understand this is because the user is unauthorized, but maybe we can handle this scenario by redirecting him to log in or just hiding the "Enroll" button.

Simulator.Screen.Recording.-.iPhone.14.-.2023-11-30.at.11.04.04.mp4

saeedbashir · 2023-11-30T14:45:27Z

The code looks good to me 👍 But I found one bug or part of missed logic.

When trying to enroll in a course, the user sees an error message. I understand this is because the user is unauthorized, but maybe we can handle this scenario by redirecting him to log in or just hiding the "Enroll" button.

Simulator.Screen.Recording.-.iPhone.14.-.2023-11-30.at.11.04.04.mp4

We will be doing that in the second phase. In the second phase, we will be showing the Signin and Register buttons on the discovery and course about pages and will redirect the user to the register screen if the user hits on the Enroll button without signin.

I've added a note for second phase in the PR description.

volodymyr-chekyrta · 2023-12-01T15:06:16Z

The code looks good to me 👍 But I found one bug or part of missed logic.
When trying to enroll in a course, the user sees an error message. I understand this is because the user is unauthorized, but maybe we can handle this scenario by redirecting him to log in or just hiding the "Enroll" button.
Simulator.Screen.Recording.-.iPhone.14.-.2023-11-30.at.11.04.04.mp4

We will be doing that in the second phase. In the second phase, we will be showing the Signin and Register buttons on the discovery and course about pages and will redirect the user to the register screen if the user hits on the Enroll button without signin.

I've added a note for second phase in the PR description.

Got it, thank you!

volodymyr-chekyrta · 2023-12-01T15:45:03Z

Looks good to me and is ready to merge 👍
The last concern is still about the Config file.
On GitHub, on the "Files сhanged" page I see the old Config file.
It looks like a wrong merge or something, could we please double-check it before merging?

saeedbashir · 2023-12-01T15:51:08Z

The following image is from my branch, and the config is latest. I'm not sure why it's showing an old copy to you. Can you please checkout my branch and see which config class you see?

volodymyr-chekyrta · 2023-12-01T16:17:22Z

The following image is from my branch, and the config is latest. I'm not sure why it's showing an old copy to you. Can you please checkout my branch and see which config class you see?

I found it. It's file in the folder, and it's not connected to the XCode proj.
Could you please delete it?

saeedbashir · 2023-12-02T08:50:00Z

Could you please delete it?

Sure, I've removed it.

volodymyr-chekyrta · 2023-12-02T13:05:53Z

Could you please delete it?

Sure, I've removed it.

The last commit is from November 24, could you please make sure you pushed the new one?

saeedbashir · 2023-12-02T13:09:08Z

The last commit is from November 24, could you please make sure you pushed the new one?

I've deleted it under this commit, 3d31b0c

saeedbashir requested a review from volodymyr-chekyrta October 31, 2023 08:32

saeedbashir requested a review from mumer92 November 3, 2023 10:06

mumer92 approved these changes Nov 8, 2023

View reviewed changes

miankhalid mentioned this pull request Nov 13, 2023

[iOS] Pre-Login Mobile App Exploration #109

Open

volodymyr-chekyrta reviewed Nov 13, 2023

View reviewed changes

OpenEdX/RouteController.swift Outdated Show resolved Hide resolved

moiz994 assigned saeedbashir Nov 14, 2023

saeedbashir requested a review from volodymyr-chekyrta November 19, 2023 05:02

touchapp requested review from k1rill and rnr and removed request for k1rill November 20, 2023 14:09

rnr previously approved these changes Nov 20, 2023

View reviewed changes

Authorization/Authorization/Presentation/Startup/StartupViewModel.swift Outdated Show resolved Hide resolved

saeedbashir dismissed rnr’s stale review via de87866 November 21, 2023 02:40

saeedbashir requested a review from rnr November 21, 2023 02:42

saeedbashir added 7 commits November 22, 2023 08:05

feat: pre-login mobile app exploration

7719296

fix: generated mock files again to fix the broken tests

0d037cb

refactor: file formatting

f0f8bfc

fix: fix broken code after GitHub resolve conflicts

ba74dd1

chore: making feature configureable via config

534db4f

refactor: remove empty method that was wrote for implementing analyti…

7226877

…cs in future

fix: fixup after rebasing with develop

38cbec7

saeedbashir force-pushed the saeed/pre_login_exploration branch from de87866 to 38cbec7 Compare November 22, 2023 05:36

Merge branch 'develop' into saeed/pre_login_exploration

d72275c

volodymyr-chekyrta reviewed Nov 22, 2023

View reviewed changes

Discovery/Discovery/Presentation/SearchView.swift Show resolved Hide resolved

refactor: address feedback

bda3e89

fix: getting the ci_script changes

09e9432

saeedbashir dismissed rnr’s stale review via 09e9432 November 22, 2023 14:17

Merge branch 'develop' into saeed/pre_login_exploration

8454926