Conversation
joragua
force-pushed
the
feature/edit_space_link
branch
from
68b51f8 to
02b0e0e
Compare
joragua
force-pushed
the
feature/edit_space_link
branch
3 times, most recently
from
3c30678 to
c5a3011
Compare
jesmrec
force-pushed
the
feature/edit_space_link
branch
from
064f233 to
dc46bde
Compare
joragua
force-pushed
the
feature/edit_space_link
branch
from
dc46bde to
8b936a5
Compare
joragua
force-pushed
the
feature/edit_space_link
branch
from
8b936a5 to
6ac162b
Compare
joragua
force-pushed
the
feature/edit_space_link
branch
3 times, most recently
from
076c8ac to
d51f23a
Compare
…link (password not included)
joragua
force-pushed
the
feature/edit_space_link
branch
from
4822a5c to
6f9dd9c
Compare
| } | ||
|
|
||
| // Do not recreate the edit view after the first iteration | ||
| if (spaceLinksViewModel.addPublicLinkUIState.value?.selectedPermission != null) return |
There was a problem hiding this comment.
Should this condition to be the first in the bock to be checked? if no permission to return even before the name and the button are binded
There was a problem hiding this comment.
This condition is used to avoid re-creating the edit view when the device is rotated, but the button still needs to be bound because the default label is CREATE (it should be SAVE in edit mode). The text binding could be moved below the condition.
New version:
binding.createPublicLinkButton.apply { ... }
// Do not recreate the edit view after the first iteration
if (spaceLinksViewModel.addPublicLinkUIState.value?.selectedPermission != null) return
binding.publicLinkNameEditText.setText(it.displayName)
| putParcelable(ARG_SELECTED_PUBLIC_LINK, selectedPublicLink) | ||
| } | ||
| return AddPublicLinkFragment().apply { | ||
| arguments = args |
There was a problem hiding this comment.
I miss OnDestroyView in this fragment (not part of this specific review but...)
There was a problem hiding this comment.
Right! Added ✅ I've added onDestroyView method in SetPasswordDialogFragment too
| expirationDate = expirationDate | ||
| ).execute(client) | ||
|
|
||
| override fun editPasswordLink(spaceId: String, linkId: String, password: String?): RemoteOperationResult<Unit> = |
There was a problem hiding this comment.
Should be every parameter in a line break like the fun above?
| _addPublicLinkUIState.value?.selectedPermission?.let { | ||
| runUseCaseWithResult( | ||
| coroutineDispatcher = coroutineDispatcherProvider.io, | ||
| flow = _editLinkResultFlow, | ||
| useCase = editLinkUseCase, | ||
| useCaseParams = EditLinkUseCase.Params( | ||
| accountName = accountName, | ||
| spaceId = space.id, | ||
| linkId = linkId, | ||
| displayName = displayName, | ||
| type = it, | ||
| expirationDate = _addPublicLinkUIState.value?.selectedExpirationDate, | ||
| ) | ||
| ) | ||
| if (_addPublicLinkUIState.value?.wasPasswordChanged == true) { | ||
| viewModelScope.launch(coroutineDispatcherProvider.io) { | ||
| editPasswordLinkUseCase(EditPasswordLinkUseCase.Params( | ||
| accountName = accountName, | ||
| spaceId = space.id, | ||
| linkId = linkId, | ||
| password = _addPublicLinkUIState.value?.selectedPassword | ||
| )) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
there are two different calls since there are two request, that's right. But, what happens if the first request fails (for any reason)? imo, the second one should be cancelled. Not sure if this make sense or it is somehow controlled.
On the same way, what happens if EditLinkUseCase successes and EditPasswordLinkUseCase fails?
Even when they are two separate request, should they be handled together as an atomic operation?
2 participants
Related Issues
App: #4756
ReleaseNotesViewModel.ktcreating a newReleaseNote()with String resources (if required)QA