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

[TECHNICAL] Technical improvements for user quota #4525

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

[TECHNICAL] Technical improvements for user quota #4525

wants to merge 9 commits into from

Conversation

joragua
Copy link
Collaborator

@joragua joragua commented Dec 11, 2024 •
edited
Loading

Related Issues

App: #4521

  • 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

@joragua joragua force-pushed the technical/technical_improvements_for_quota branch from 8e7bfe9 to 2f0df52 Compare December 11, 2024 09:38
@joragua joragua self-assigned this Dec 11, 2024
@joragua joragua linked an issue Dec 11, 2024 that may be closed by this pull request
[TECHNICAL] Technical improvements for user quota #4521
Open
11 tasks
@joragua joragua force-pushed the technical/technical_improvements_for_quota branch from 1f95f15 to 8d5c35f Compare December 11, 2024 10:45
@joragua joragua marked this pull request as ready for review December 11, 2024 12:12
@joragua joragua requested a review from JuancaG05 December 11, 2024 12:12
JuancaG05
JuancaG05 requested changes Dec 12, 2024
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.

Good @joragua!! Let's do some changes here 👍

owncloudApp/src/main/java/com/owncloud/android/presentation/common/DrawerViewModel.kt Outdated
useCase = getStoredQuotaUseCase,
useCaseParams = GetStoredQuotaUseCase.Params(accountName = accountName)
)
val userQuota: Flow<UserQuota> = getStoredQuotaAsStreamUseCase(GetStoredQuotaAsStreamUseCase.Params(accountName))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, not sure if I like this... I think it would be better being encapsulated with a private variable. Otherwise everytime we access userQuota, the use case might be executed, and that's not what we want. Check for example SettingsPictureUploadsViewModel to see how this is done with the pictureUploads variable 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done! 😄

owncloudApp/src/main/java/com/owncloud/android/ui/activity/DrawerActivity.kt
private val drawerViewModel by viewModel<DrawerViewModel>()
private val drawerViewModel by viewModel<DrawerViewModel> {
parametersOf(
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.

What if account is null? Did you check where this value is set in BaseActivity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case, account variable cannot be null because is set in swapToDefaultAccount method from BaseActivity before creating the DrawerViewModel. This is the same with other view models like CapabilityViewModel or ShareViewModel

owncloudApp/src/main/java/com/owncloud/android/ui/activity/DrawerActivity.kt
Comment on lines -415 to -416
is UIResult.Loading -> getAccountQuotaText()?.text = getString(R.string.drawer_loading_quota)
is UIResult.Error -> getAccountQuotaText()?.text = getString(R.string.drawer_unavailable_used_storage)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're losing these 2 cases!! We shouldn't, they are so necessary

Copy link
Collaborator Author

@joragua joragua Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've replace BaseUseCase by BaseUseCaseWithResult (GetStoredQuotaAsStreamUseCase) and now, we have same cases than before

owncloudApp/src/main/java/com/owncloud/android/ui/activity/DrawerActivity.kt Outdated
getAccountQuotaStatusText()?.visibility = View.GONE
}
}
collectLatestLifecycleFlow(drawerViewModel.userQuota) { userQuota ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did some changes in all these lines in the Detekt branch, I think just for the MaxLineLength rule. Try to keep them in the new place for these lines so that we don't duplicate work

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done ✅

owncloudData/src/main/java/com/owncloud/android/data/user/db/UserDao.kt Outdated
@Query(SELECT_QUOTA)
fun getQuotaForAccountAsFlow(
accountName: String
): Flow<UserQuotaEntity>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why with the same SQL query you have a nullable result in the previous method but not here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added nullability to keep the same structure in both SQL queries

.../java/com/owncloud/android/data/user/datasources/implementation/OCLocalUserDataSourceTest.kt

ocLocalUserDataSource.deleteQuotaForAccount(OC_ACCOUNT_NAME)
val userQuota = ocLocalUserDataSource.getQuotaForAccountAsFlow(OC_ACCOUNT_NAME).first()
assertEquals(userQuotaEntity.toModel(), userQuota)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assertEquals(userQuotaEntity.toModel(), userQuota)
assertEquals(OC_USER_QUOTA, userQuota)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done ✅

.../java/com/owncloud/android/data/user/datasources/implementation/OCLocalUserDataSourceTest.kt Outdated
} returns flowOf(listOf(userQuotaEntity))

val listOfUserQuotas = ocLocalUserDataSource.getAllUserQuotasAsFlow().first()
assertEquals(listOf(userQuotaEntity.toModel()), listOfUserQuotas)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assertEquals(listOf(userQuotaEntity.toModel()), listOfUserQuotas)
assertEquals(listOf(OC_USER_QUOTA), listOfUserQuotas)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as previous comment 😄

@joragua joragua force-pushed the technical/technical_improvements_for_quota branch from 32ed01c to d7f96a2 Compare December 18, 2024 11:56
@joragua joragua requested a review from JuancaG05 December 18, 2024 12:11
@joragua joragua force-pushed the technical/technical_improvements_for_quota branch from 053a86d to 1503d9d Compare December 18, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JuancaG05 JuancaG05 Awaiting requested review from JuancaG05

Requested changes must be addressed to merge this pull request.

Assignees

@joragua joragua

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[TECHNICAL] Technical improvements for user quota
2 participants
@joragua @JuancaG05