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: [FC-0047] xBlock offline mode #474

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

IvanStepanok
Copy link
Contributor

@IvanStepanok IvanStepanok commented Jun 25, 2024

πŸ›œ Offline mode – Quick Preview

Downloading

1downloading.1.mov

Working with blocks without an internet connection

Screen.Recording.2024-06-25.at.20.14.22.1.mov

Offline content is synchronized immediately after connecting to the network

offline.content.are.synced.1.mov

Environment for testing:

API_HOST_URL: 'https://axim-mobile.raccoongang.net'
ENVIRONMENT_DISPLAY_NAME: 'lms-axim-stage'
FEEDBACK_EMAIL_ADDRESS: 'support@example.com'
OAUTH_CLIENT_ID: 'zP3vPz00c8fTRpYjNbVSlA1fxt9LnCxTM4JK1KQ0'
DISCOVERY:
    TYPE: "native"
    
FIREBASE:
    ENABLED: false

UI_COMPONENTS:
    COURSE_UNIT_PROGRESS_ENABLED: false
    COURSE_NESTED_LIST_ENABLED: false

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 25, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Jun 25, 2024

Thanks for the pull request, @IvanStepanok!

What's next?

Please work through the following steps to get your changes ready for engineering review:

πŸ”˜ Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

πŸ”˜ Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

πŸ”˜ Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

πŸ”˜ Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/openedx-mobile-maintainers. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

πŸ’‘ As a result it may take up to several weeks or months to complete a review and merge your PR.

@IvanStepanok IvanStepanok changed the base branch from main to develop June 26, 2024 08:52
@volodymyr-chekyrta volodymyr-chekyrta changed the title Feat/course offline mode feat: [FC-0047] xBlock offline mode Jun 26, 2024
@volodymyr-chekyrta volodymyr-chekyrta marked this pull request as ready for review July 23, 2024 08:38
@mphilbrick211 mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Jul 30, 2024
@rnr
Copy link
Contributor

rnr commented Sep 3, 2024

@IvanStepanok Thank you for your work on this feature - looks cool!
I have a few comments on the UI - will place here

@rnr
Copy link
Contributor

rnr commented Sep 3, 2024

  1. While the app is downloading for offline, the user stops the app and starts it again; after navigating to the course -> offline user sees the β€˜Download All’ button, but the download continues.
offline_bug.mov

@rnr
Copy link
Contributor

rnr commented Sep 3, 2024

  1. All content in course is loaded. User deletes only one block. After tap on "Download all" button it shows like user will get whole course again.
Screen.Recording.2024-09-03.at.14.58.31.mov

@rnr
Copy link
Contributor

rnr commented Sep 3, 2024

  1. All content in course is loaded. Some chapter contains youtube videos only (which are not downloadable). When goes to Videos Tab and try to remove such chapter it shows "Removing this content will free up 0MB" - do we need to show 'downloaded' state for chapters in Videos which don't contain downloaded videos?

@rnr
Copy link
Contributor

rnr commented Sep 4, 2024

  1. A user has downloaded a course. The user answered a few questions offline, went online, the answers were synchronised, and now we see them in the online version. (it's working right)
    When the user goes to that test again offline, he don't see the answers - it looks like the user is seeing the downloaded version BEFORE he gave the answers.
RPReplay_Final1725451531.MP4

@rnr
Copy link
Contributor

rnr commented Sep 4, 2024

  1. The user can't select a previously selected option in an offline quiz. For example: select option 3 - submit - change your mind and select option 4 - submit - change your mind again and try to select option 3 - unable to select option 3 again
RPReplay_Final1725452770.MP4

@IvanStepanok
Copy link
Contributor Author

IvanStepanok commented Sep 5, 2024

Hi @rnr, and thank you for the review! It is very valuable to me to get a view from another angle. I appreciate it.

  1. βœ… Fixed
Screen.Recording.2024-09-05.at.12.23.40.mov
  1. βœ…Fixed (Pay attention. We don't show download notification, less than 100 MB when connected to Wi-Fi)
RPReplay_Final1725544125.MP4
  1. βœ… Fixed
Screen.Recording.2024-09-05.at.16.54.28.mov

I also noticed that if we delete one file, when downloading, the dialog box shows us the size of the entire group instead of a single undownloaded file. This is also fixed.

  1. πŸ€·β€β™‚οΈ This is a known issue, the Open EDX team plans to improve this flow in the future. Currently, the backend has IOSBridge only for offline mode. As soon as they finalize the backend, it will work for us immediately.

  2. πŸ€·β€β™‚οΈ This is also the behavior that was set on the backend side, on our side we cannot influence it in any way.

@rnr
Copy link
Contributor

rnr commented Sep 9, 2024

@IvanStepanok Thank you for the fixes and clarifications
Looks like we have issue for video:
2. It seems that the video file is not counted in the total size, even though the dialogue says that this space will be freed up.

Screen.Recording.2024-09-09.at.14.29.58.mov

@IvanStepanok
Copy link
Contributor Author

Hi, @rnr, and thanks for the question. In this case, we don't know the size of a video until we download it because the server returns 0 megabytes. We can measure the actual size of the videos only after downloading. To prevent this behavior, we need to receive the actual video size in the server response.

Copy link
Contributor

@rnr rnr left a comment

Choose a reason for hiding this comment

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

Amazing work done. Please fix some small comments

if let pdfData = try? Data(contentsOf: url) {
webview.load(
pdfData,
mimeType: "application/pdf",
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe there are some other types of files we may want to open (which webview also supports)? Maybe we need to create some Model (with mimeType and extension parameters, maybe also isWebViewViewable could be determined there) for file and use here and inside WebView->decidePolicyFor while check file extension? WDYT?

}
} catch let error {
if error is NoWiFiError {
errorMessage = CoreLocalization.Error.wifi
}
}
}


private func presentNoInternetAlert(sequentials: [CourseSequential]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will be this Alert showed anytime? As I see it should be showed if !connectivity.isInternetAvaliable { inside downloadAll() function but downloadAll button (which fires this function) is hiding in case if !connectivity.isInternetAvaliable {. So user has no chance to tap on 'Download All' button in offline. WDYT?

.padding(.top, 8)
.padding(.bottom, 16)
}
downloadAll
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like redundant space here

@ViewBuilder
private var downloadAll: some View {
if viewModel.connectivity.isInternetAvaliable
&& (viewModel.totalFilesSize - viewModel.downloadedFilesSize != 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the second part (after &&) be included in the common brackets?

.stroke(
viewModel.totalFilesSize == 0
? .clear
: viewModel.downloadAllButtonState.color, lineWidth: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

please move lineWidth on next line

childs: [],
sequentialProgress: nil,
due: nil
),
Copy link
Contributor

Choose a reason for hiding this comment

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

trailing comma violation

}.contains(where: { $0.isDownloadable })
guard isDownloadable else { return false }
}
if let _ = viewModel.sequentialsDownloadState[sequential.id] {
Copy link
Contributor

Choose a reason for hiding this comment

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

change to !=nil please

@@ -413,7 +429,7 @@ public struct CourseUnitView: View {
Spacer()
}
VStack {
if !isHorizontal {
if (!isHorizontal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need brackets here?

}
.padding(24)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need preview for this view?

// Background progress update

func registerBackgroundTask() {
let isRegistered = BGTaskScheduler.shared.register(forTaskWithIdentifier: Self.bgAppTaskId, using: nil) { task in
Copy link
Contributor

Choose a reason for hiding this comment

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

line length violation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FC Relates to an Axim Funded Contribution project open-source-contribution PR author is not from Axim or 2U
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

4 participants