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

Preserve PDF annotations even when cache is reset #2965

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

suhaibabsi-inst
Copy link
Contributor

@suhaibabsi-inst suhaibabsi-inst commented Nov 11, 2024

refs: MBL-18069
affects: Student
release note: PDF annotations is preserved even when cache is reset

Test Plan

  • Login to Student app
  • Open a PDF file and make some annotations to it, then go back.
  • Close the app entirely.
  • Go to the App's page on Settings app.
  • Turn on "Reset cache on next launch" toggle.
  • Launch the app, then login back to the same user.
  • Open the PDF file that was annotated in step 2.
  • You should see the previously saved annotations on that file.

Video Record

Recording covering PDF annotation persistence scenarios, which are:

  • On logout
  • On cache reset.
pdf_annotations_saving.mov

Checklist

  • Follow-up e2e test ticket created
  • A11y checked
  • Tested on phone
  • Tested on tablet
  • Tested in dark mode
  • Tested in light mode
  • Approve from product

refs: MBL-18069
affects: Student
release note: PDF annotations is preserved even when cache is reset

test plan: See PR description
@inst-danger
Copy link
Contributor

inst-danger commented Nov 11, 2024

Student Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented Nov 11, 2024

Warnings
⚠️ One or more files are below the minimum test coverage 50%

Release Note:

PDF annotations is preserved even when cache is reset

Affected Apps: Student

MBL-18069

Coverage New % Master % Delta
Canvas iOS 91.43% 91.43% 0.01%
Core/Core/Assignments/AssignmentList/ViewModel/AssignmentListViewModel.swift 47.8% 47.8% 0%

Generated by 🚫 dangerJS against ade8f6e

@suhaibabsi-inst suhaibabsi-inst marked this pull request as ready for review November 13, 2024 08:55
@@ -35,7 +35,7 @@ extension LocalFileURLCreator {
if mimeClass == "pdf" {
// If the user already downloaded and modified the file locally, we don't want to download it again.
// Instead, return the url pointing to the locally modified version.
let docsURL = URL.Directories.documents.appendingPathComponent(fileName)
let docsURL = URL.Directories.annotatedPDFs.appendingPathComponent(fileName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Modifying the load url like this will break access to already existing annotated files in the documents directory when the user updates the app to this version. Let's check for the annotated file on the original url as well OR create a job that moves all annotated pdfs to their new directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes. That's correct!
Almost forgot about that.
Sure, will work on that.

@@ -72,6 +72,7 @@ open class AppEnvironment {
OfflineModeAssembly.make()
api = API(session)
currentSession = session
currentSession?.migrateSavedAnnotatedPDFs()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to call this here, and do it per session so to be targeting those folders relevant to this specific feature & session. Thus we do it lazily if there are multiple sessions.
There was an attempt to do it on app launch for all sessions at once, but there was a difficulty fetching all sessions that have files saved to Documents directory. Not all of them get persisted to Keychain as I was expecting.

ndrsszsz
ndrsszsz previously approved these changes Nov 14, 2024
Copy link
Contributor

@ndrsszsz ndrsszsz left a comment

Choose a reason for hiding this comment

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

QA +1
Tested on iPhone 11, iOS 18.1

guard let folderUrl = urls.first(where: { $0.hasDirectoryPath && $0.lastPathComponent == uniqueID })
else { return }

print("Moving previously-saved documents folder for session (\(uniqueID)) ..")
Copy link
Contributor

Choose a reason for hiding this comment

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

A print is left here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants