Skip to content

feat: delete old videos Directory - #316

Closed
omerhabib26 wants to merge 0 commit into
openedx:developfrom
omerhabib26:omer/LEARNER-9950
Closed

feat: delete old videos Directory#316
omerhabib26 wants to merge 0 commit into
openedx:developfrom
omerhabib26:omer/LEARNER-9950

Conversation

@omerhabib26

@omerhabib26 omerhabib26 commented May 8, 2024

Copy link
Copy Markdown
Contributor

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

@HamzaIsrar12 HamzaIsrar12 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Test cases are failing.

Comment thread app/src/main/java/org/openedx/app/di/ScreenModule.kt Outdated
Comment thread dashboard/src/main/java/org/openedx/dashboard/presentation/DashboardFragment.kt Outdated
Comment thread dashboard/src/main/java/org/openedx/dashboard/presentation/DashboardFragment.kt Outdated
Comment thread core/src/main/java/org/openedx/core/utils/FileUtil.kt Outdated
@omerhabib26
omerhabib26 requested a review from HamzaIsrar12 May 13, 2024 22:14
@HamzaIsrar12

Copy link
Copy Markdown
Contributor

Test Cases are still failing ...

@omerhabib26

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@k1rill k1rill left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@omerhabib26
omerhabib26 dismissed stale reviews from k1rill and HamzaIsrar12 via f6ecde0 May 22, 2024 14:47
@omerhabib26

Copy link
Copy Markdown
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.

@volodymyr-chekyrta volodymyr-chekyrta left a comment

Copy link
Copy Markdown
Contributor

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