Skip to content

[FEATURE REQUEST] Manual removal of local storage#4334

Merged
Aitorbp merged 15 commits into
masterfrom
feature/manual_removal_local_storage
Apr 5, 2024
Merged

[FEATURE REQUEST] Manual removal of local storage#4334
Aitorbp merged 15 commits into
masterfrom
feature/manual_removal_local_storage

Conversation

@Aitorbp

@Aitorbp Aitorbp commented Mar 5, 2024 •
edited by jesmrec
Loading

Copy link
Copy Markdown
Contributor

Related Issues

App: #4174

  • Add changelog files for the fixed issues in folder changelog/unreleased. More info here
  • Add feature to Release Notes in ReleaseNotesViewModel.kt creating a new ReleaseNote() with String resources (if required)

QA

checks performed: #4334 (comment)

Reports:

@Aitorbp Aitorbp self-assigned this Mar 5, 2024
@Aitorbp Aitorbp linked an issue Mar 5, 2024 that may be closed by this pull request
[FEATURE REQUEST] Manual removal of local storage #4174
Closed
15 tasks
@Aitorbp Aitorbp requested a review from JuancaG05 March 5, 2024 09:24
@Aitorbp Aitorbp force-pushed the feature/manual_removal_local_storage branch 2 times, most recently from c5ffe1a to ec30fb5 Compare March 26, 2024 09:17
JuancaG05
JuancaG05 requested changes Mar 26, 2024
View reviewed changes

@JuancaG05 JuancaG05 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.

Several things to check here yet @Aitorbp

Comment thread changelog/unreleased/4334 Outdated
Comment thread ...udApp/src/main/java/com/owncloud/android/presentation/accounts/AccountsManagementActivity.kt Outdated
Comment thread owncloudApp/src/main/res/values/strings.xml Outdated
Comment thread owncloudApp/src/main/res/values/strings.xml Outdated
Comment thread owncloudApp/src/main/res/values/strings.xml Outdated
Comment thread owncloudData/src/main/java/com/owncloud/android/data/providers/LocalStorageProvider.kt
Comment thread ...java/com/owncloud/android/data/files/datasources/implementation/OCLocalFileDataSourceTest.kt Outdated
Comment thread ...java/com/owncloud/android/data/files/datasources/implementation/OCLocalFileDataSourceTest.kt
Comment thread owncloudDomain/src/main/java/com/owncloud/android/domain/files/FileRepository.kt Outdated
Comment thread owncloudData/src/test/java/com/owncloud/android/data/files/repository/OCFileRepositoryTest.kt
@Aitorbp Aitorbp force-pushed the feature/manual_removal_local_storage branch from b05ec0d to 3698f48 Compare March 26, 2024 18:45
JuancaG05
JuancaG05 reviewed Mar 27, 2024
View reviewed changes
Comment thread owncloudData/src/main/java/com/owncloud/android/data/files/db/FileDao.kt Outdated
Comment thread owncloudData/src/main/java/com/owncloud/android/data/providers/LocalStorageProvider.kt
Comment thread ...java/com/owncloud/android/data/files/datasources/implementation/OCLocalFileDataSourceTest.kt
JuancaG05
JuancaG05 requested changes Mar 27, 2024
View reviewed changes

@JuancaG05 JuancaG05 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.

Some more changes and ready to go @Aitorbp

Comment thread owncloudData/src/test/java/com/owncloud/android/data/files/repository/OCFileRepositoryTest.kt Outdated
Comment on lines +217 to +226
@Test
fun `getDownloadedFilesForAccount returns an empty list when datasource returns an empty list`() {
every { localFileDataSource.getDownloadedFilesForAccount(OC_ACCOUNT_NAME) } returns emptyList()

val result = ocFileRepository.getDownloadedFilesForAccount(OC_ACCOUNT_NAME)

assertEquals(emptyList<OCFile>(), result)

verify(exactly = 1) { localFileDataSource.getDownloadedFilesForAccount(OC_ACCOUNT_NAME) }
}

@JuancaG05 JuancaG05 Mar 27, 2024

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.

This is not needed, the result is what the datasource returns, we're not giving it a special treatment in-between

@Aitorbp Aitorbp Mar 27, 2024

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.

Ok, deleted

@Aitorbp Aitorbp requested a review from JuancaG05 March 27, 2024 10:13
JuancaG05
JuancaG05 approved these changes Mar 27, 2024
View reviewed changes

@JuancaG05 JuancaG05 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 👍

@jesmrec

jesmrec commented Apr 1, 2024

Copy link
Copy Markdown
Contributor

Let's QA this one...

@jesmrec

jesmrec commented Apr 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

QA checks:

  • UI changes
  • Clean up oCIS account
  • Clean up oC10 account
  • Clean up one account out of several
  • Clean up with av. offline (not removed)
  • Clean up while syncing
  • Resume uploads after cleaning up
  • Logs not removed

@jesmrec

jesmrec commented Apr 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

(1) [FIXED]

We have lost the layout adaption for long strings. Is this a regression?:

Screenshot 2024-04-01 at 10 06 03

Pixel 2, Android 11
725873470

@jesmrec

jesmrec commented Apr 1, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

(2) [WONT FIX]

i don't want to look crazy, but the new icon looks like a brush more than a broom, not sure how clear it will be for users. This is how it looks like, any better idea @tbsbdr?:

Screenshot 2024-04-01 at 10 15 54

@tbsbdr

tbsbdr commented Apr 3, 2024 •
edited
Loading

Copy link
Copy Markdown

as suggested by you: lets stick to the icon you suggested for now and show a dialog to that the user gets reassured within the text dialog, what will happen if he/she proceeds.

my perspective is, this icon is just a temporary solution because we don't have a better one as of now.

For the future, I can also think about using a "drawer menu" like you suggested and already made for files. This could be a better way to add more actions because we can fit more in it. In a drawer, we can also put in some text to explain what the icons mean.

Drawer example from material design

image

@jesmrec

jesmrec commented Apr 4, 2024

Copy link
Copy Markdown
Contributor

So, the brush-broom icon will be the first approach. We'll work in future improvements

@jesmrec

jesmrec commented Apr 4, 2024

Copy link
Copy Markdown
Contributor

Approved on my side

@Aitorbp Aitorbp force-pushed the feature/manual_removal_local_storage branch from bd88dcc to 7650be0 Compare April 5, 2024 06:38
Aitorbp added 10 commits April 5, 2024 08:11
@Aitorbp
Added getFilesForAccount in FileDao, FileRepository and LocalFileData…
fa37d6e 
…Source
@Aitorbp
Added usecase RemoveFilesForAccountUseCase and created clearTemporalF…
fc69541 
…ilesForAccount function
@Aitorbp
Added clean icon, dialog and subscriptions in AccountsManagementActiv…
b41c02b 
…ity, listener in AccountsManagementAdapter and cleanAccount function with the use case
@Aitorbp
Added tests in OCFileRepositoryTest and OCLocalFileDataSourceTest
0733b3b
@Aitorbp
Added release notes and some strings related to dialog
15ab24e
@Aitorbp
feat: removed clearTemporalFilesForAccount func
50a0f6e
@Aitorbp
feat: Changed release note
95fc49c
@Aitorbp
feat: Reorder functions in OCLocalFileDataSource
6320491
@Aitorbp
fix: Changes in strings of release notes and clean_data_account_message
e515f9a
@Aitorbp
fix: Improve the SELECT_FILES_FOR_ACCOUNT, changed named of the funct…
de1d04d 
…ions/variables, refactor classes and changes xml
@Aitorbp Aitorbp force-pushed the feature/manual_removal_local_storage branch from 7650be0 to 762ab03 Compare April 5, 2024 07:11
@Aitorbp Aitorbp merged commit e1cf321 into master Apr 5, 2024
@Aitorbp Aitorbp deleted the feature/manual_removal_local_storage branch April 5, 2024 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jesmrec jesmrec jesmrec left review comments

@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] Manual removal of local storage

4 participants

@Aitorbp @jesmrec @tbsbdr @JuancaG05