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

[FEATURE REQUEST] Manual removal of local storage #4334

Merged
merged 15 commits into from
Apr 5, 2024

Conversation

Aitorbp
Copy link
Contributor

@Aitorbp Aitorbp commented Mar 5, 2024

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

Several things to check here yet @Aitorbp

@Aitorbp Aitorbp force-pushed the feature/manual_removal_local_storage branch from b05ec0d to 3698f48 Compare March 26, 2024 18:45
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 more changes and ready to go @Aitorbp

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) }
}
Copy link
Collaborator

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

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

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 1, 2024

Let's QA this one...

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 1, 2024

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

jesmrec commented Apr 1, 2024

(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
Copy link
Collaborator

jesmrec commented Apr 1, 2024

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

tbsbdr commented Apr 3, 2024

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

jesmrec commented Apr 4, 2024

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

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 4, 2024

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 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
6 checks passed
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Manual removal of local storage
4 participants