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.

Sign up for GitHub

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

Jump to bottom

[FEATURE REQUEST] New field "last usage" #4187

Merged
merged 11 commits into from
Nov 3, 2023
Merged

[FEATURE REQUEST] New field "last usage" #4187

merged 11 commits into from
Nov 3, 2023

Conversation

Aitorbp
Copy link
Contributor

@Aitorbp Aitorbp commented Oct 15, 2023 •
edited by jesmrec

Related Issues

App: #4173

  • Added changelog files for the fixed issues in folder changelog/unreleased. More info here

QA

#4187 (comment)

@Aitorbp Aitorbp self-assigned this Oct 15, 2023
@Aitorbp Aitorbp linked an issue Oct 15, 2023 that may be closed by this pull request
[FEATURE REQUEST] New field "last usage" #4173
Closed
7 tasks
@Aitorbp Aitorbp force-pushed the feature/new_field_last_usage branch from 582e922 to 892d97b Compare October 16, 2023 12:26
manuelplazaspalacio
manuelplazaspalacio suggested changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@manuelplazaspalacio manuelplazaspalacio left a comment

Choose a reason for hiding this comment

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

Some changes here.

owncloudApp/src/main/java/com/owncloud/android/dependecyinjection/ViewModelModule.kt Outdated Show resolved Hide resolved
...loudApp/src/main/java/com/owncloud/android/presentation/files/details/FileDetailsFragment.kt Outdated Show resolved Hide resolved
...oudApp/src/main/java/com/owncloud/android/presentation/files/details/FileDetailsViewModel.kt Outdated Show resolved Hide resolved
...oudApp/src/main/java/com/owncloud/android/presentation/files/details/FileDetailsViewModel.kt Outdated Show resolved Hide resolved
owncloudData/src/main/java/com/owncloud/android/data/files/db/FileDao.kt Outdated Show resolved Hide resolved
owncloudDomain/src/main/java/com/owncloud/android/domain/files/usecases/RemoveFileUseCase.kt Outdated Show resolved Hide resolved
owncloudDomain/src/main/java/com/owncloud/android/domain/files/usecases/RemoveFileUseCase.kt Outdated Show resolved Hide resolved
owncloudDomain/src/main/java/com/owncloud/android/domain/files/usecases/RenameFileUseCase.kt Outdated Show resolved Hide resolved
owncloudDomain/src/main/java/com/owncloud/android/domain/files/usecases/RenameFileUseCase.kt Outdated Show resolved Hide resolved
...udDomain/src/main/java/com/owncloud/android/domain/files/usecases/SetLastUsageFileUseCase.kt Outdated Show resolved Hide resolved
@jesmrec
Copy link
Collaborator

jesmrec commented Oct 20, 2023 •
edited

QA

Checks to be done:

  • New field created in DB, table of files
    • Fresh installation -> NULL
    • Migration -> Migration date for downloaded, NULL for non-downloaded
  • Download file -> last_usage updated
  • Preview non-downloaded file -> last_usage updated
  • Preview downloaded file
  • Rename file downlaoded -> last_usage updated
  • Move file downloaded -> last_usage updated
  • Rename file non-downlaoded -> last_usage not updated
  • Move file non-downloaded -> last_usage not updated
  • Copy file (downloaded and non-downloaded) -> last_usage not updated
  • Remove file locally -> last_usage to NULL
  • Remove folder locally
  • Open with - file downloaded
  • Open with - file not downloaded
  • Open with web - file downloaded
  • Open with web - file not downloaded - NA

Tested with

Pixel2 Android11
Galaxy A8 Android13

@manuelplazaspalacio
Copy link
Contributor

LGTM!!

@Aitorbp Aitorbp force-pushed the feature/new_field_last_usage branch from f09cd2c to 69abb24 Compare October 25, 2023 08:48
@jesmrec
Copy link
Collaborator

jesmrec commented Oct 25, 2023 •
edited

(1) [FIXED]

  1. Download a previewable file (txt, image) -> lastUsage is updated
  2. Go back to the list of files
  3. Preview again the same file in 1.

Current: lastUsage is not updated
Expected: file is opened again, so lastUsage date must be updated

Galaxy Tab A8
d7879fc0233

@jesmrec
Copy link
Collaborator

jesmrec commented Oct 25, 2023 •
edited

(2) [FIXED]

There is one missing case we did not comment in the feature planning:

  1. Download files that are inside a folder -> lastUsage is set for the files
  2. Open the options card of the folder and select Remove and Local only

Current: folder content is cleaned up locally, but the lastUsage field of every file inside the folder is still set.
Expected: folder content is cleaned up locally, so the lastUsage field of every file inside the folder should be set to NULL

Galaxy Tab A8
d7879fc0233

JuancaG05
JuancaG05 requested changes Oct 27, 2023
View reviewed changes
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment •
edited

Choose a reason for hiding this comment

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

Some changes needed here @Aitorbp!
Also, we need tests of the new classes and methods you added so that we don't keep track of the tests we are updating in other issues. Let's create at least the datasources tests 👍

changelog/unreleased/4187 Outdated Show resolved Hide resolved
owncloudApp/src/main/java/com/owncloud/android/db/ProviderMeta.java Outdated Show resolved Hide resolved
...oudApp/src/main/java/com/owncloud/android/presentation/files/details/FileDetailsViewModel.kt Outdated Show resolved Hide resolved
owncloudApp/src/main/java/com/owncloud/android/providers/FileContentProvider.kt Outdated Show resolved Hide resolved
owncloudApp/src/main/java/com/owncloud/android/workers/UploadFileFromFileSystemWorker.kt Outdated Show resolved Hide resolved
owncloudData/src/main/java/com/owncloud/android/data/files/db/FileDao.kt Outdated Show resolved Hide resolved
owncloudData/src/main/java/com/owncloud/android/data/files/db/OCFileEntity.kt Outdated Show resolved Hide resolved
owncloudData/src/main/java/com/owncloud/android/data/files/repository/OCFileRepository.kt Outdated Show resolved Hide resolved
owncloudData/src/main/java/com/owncloud/android/data/migrations/Migration_42.kt Outdated Show resolved Hide resolved
owncloudDomain/src/main/java/com/owncloud/android/domain/files/usecases/RemoveFileUseCase.kt Outdated Show resolved Hide resolved
@Aitorbp Aitorbp force-pushed the feature/new_field_last_usage branch from dbc827b to 664bab4 Compare October 30, 2023 10:40
JuancaG05
JuancaG05 requested changes Oct 30, 2023
View reviewed changes
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

Some last changes here and we're ready to go @Aitorbp 🚀

...udApp/src/main/java/com/owncloud/android/presentation/files/filelist/MainFileListFragment.kt Outdated Show resolved Hide resolved
...udDomain/src/main/java/com/owncloud/android/domain/files/usecases/SetLastUsageFileUseCase.kt Outdated Show resolved Hide resolved
...udData/src/test/java/com/owncloud/android/data/file/datasources/OCLocalFileDataSourceTest.kt Outdated Show resolved Hide resolved
JuancaG05
JuancaG05 approved these changes Oct 30, 2023
View reviewed changes
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

LGTM now!! Good job @Aitorbp 🚀

@Aitorbp Aitorbp force-pushed the feature/new_field_last_usage branch from b756d50 to 80ea9fa Compare October 31, 2023 09:59
@jesmrec
Copy link
Collaborator

jesmrec commented Nov 2, 2023

3 calens commits (agreed 2). Please fix it.

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 2, 2023 •
edited

(3) [FIXED]

After checking some use cases, i think we should add another one: update lastUsage when Open with and Open with web option is clicked over a downloaded file. That means, we are opening the file with a 3rd party app and should count as an use. What do you think?

@Aitorbp
Copy link
Contributor Author

Aitorbp commented Nov 2, 2023

Yes, I think it would be good to add this use case too. Because of It is a file action too. I will work to add it.

@Aitorbp Aitorbp force-pushed the feature/new_field_last_usage branch from b4faeb0 to 3d79053 Compare November 2, 2023 11:54
@Aitorbp Aitorbp force-pushed the feature/new_field_last_usage branch from 0f41244 to d83e9c6 Compare November 2, 2023 12:11
@Aitorbp
Copy link
Contributor Author

Aitorbp commented Nov 3, 2023

@jesmrec can test again the issue. I added the new use case.

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 3, 2023

@jesmrec can test again the issue. I added the new use case.

thanks. I noticed:

  • "open with" works fine
  • "open with web" (app providers) does not update the lastUsage

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 3, 2023

@jesmrec can test again the issue. I added the new use case.

thanks. I noticed:

  • "open with" works fine
  • "open with web" (app providers) does not update the lastUsage

problem with "open with web" related with downloaded files. If file is downloaded, lastUsage is updated, otherwise, it doesn't.

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 3, 2023

problem with "open with web" related with downloaded files. If file is downloaded, lastUsage is updated, otherwise, it doesn't.

that's expected, since non-downloaded files do not affect the issue.

@jesmrec
Copy link
Collaborator

jesmrec commented Nov 3, 2023

Approved on my side!

@Aitorbp Aitorbp merged commit 5954867 into master Nov 3, 2023
5 checks passed
@Aitorbp Aitorbp deleted the feature/new_field_last_usage branch November 3, 2023 09:15
Aitorbp added a commit that referenced this pull request Feb 5, 2024
@Aitorbp
Merge pull request #4187 from owncloud/feature/new_field_last_usage
7a6f1a1 
[FEATURE REQUEST] New field "last usage"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manuelplazaspalacio manuelplazaspalacio manuelplazaspalacio approved these changes

@JuancaG05 JuancaG05 JuancaG05 approved these changes

Assignees

@Aitorbp Aitorbp

Labels
Approved by qa
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] New field "last usage"
4 participants
@Aitorbp @jesmrec @manuelplazaspalacio @JuancaG05