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: delete old videos Directory #316

Closed
wants to merge 0 commits into from

Conversation

omerhabib26
Copy link
Contributor

@omerhabib26 omerhabib26 commented May 8, 2024

Description

This PR implements functionality to delete outdated video files stored in the previous app directory upon the first launch of the updated app version. This ensures that storage is efficiently managed, preventing unnecessary space usage by obsolete files, and maintaining consistency with the new data storage structure.

fix: LEARNER-9950

Copy link
Contributor

@HamzaIsrar12 HamzaIsrar12 left a comment

Choose a reason for hiding this comment

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

Test cases are failing.

@HamzaIsrar12
Copy link
Contributor

Test Cases are still failing ...

@omerhabib26
Copy link
Contributor Author

@volodymyr-chekyrta, @k1rill please spare some time to review this PR.
Thanks

HamzaIsrar12
HamzaIsrar12 previously approved these changes May 21, 2024
Comment on lines 139 to 142
if (viewModel.canResetAppDirectory){
viewModel.resetAppDirectory(this)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we just encapsulate all this inside of the AppViewModel#onCreate?
In this case, we would remove the extra variable and this logic from the view.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I'll update it accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

k1rill
k1rill previously approved these changes May 22, 2024
Copy link
Contributor

@k1rill k1rill left a comment

Choose a reason for hiding this comment

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

LGTM

@omerhabib26
Copy link
Contributor Author

Test cases are failing due to context, the issue is already addressed in PR-308. I'll update and merge it once PR-308 is merged.

Copy link
Contributor

@volodymyr-chekyrta volodymyr-chekyrta left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Let's merge it after the #308

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.

4 participants