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] Manage deep links and open the app correctly #4191

Merged
merged 10 commits into from
Oct 26, 2023
Merged

[FEATURE REQUEST] Manage deep links and open the app correctly #4191

merged 10 commits into from
Oct 26, 2023

Conversation

manuelplazaspalacio
Copy link
Contributor

@manuelplazaspalacio manuelplazaspalacio commented Oct 17, 2023 •
edited by JuancaG05

Related Issues

App: #4181

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

QA

@manuelplazaspalacio manuelplazaspalacio linked an issue Oct 17, 2023 that may be closed by this pull request
[FEATURE REQUEST] Manage deep links and open the app correctly #4181
Closed
10 tasks
@manuelplazaspalacio manuelplazaspalacio marked this pull request as ready for review October 17, 2023 09:26
@JuancaG05 JuancaG05 mentioned this pull request Oct 17, 2023
Closed
1 task
@manuelplazaspalacio manuelplazaspalacio changed the title Feature/deep link open app [FEATURE REQUEST] Manage deep links and open the app correctly Oct 17, 2023
@manuelplazaspalacio manuelplazaspalacio force-pushed the feature/deep_link_open_app branch 5 times, most recently from b13d49d to 6455b07 Compare October 24, 2023 12:29
@jesmrec
Copy link
Collaborator

jesmrec commented Oct 25, 2023

Code review is not totally complete, but i advanced to QA.

Functionally, feature is pretty correct, opening correctly the links based in the registered scheme.

Just a couple of comments about the error messages:

If no accounts are attached to the app, after open link, this is the displayed message:

You need a login user to open the link

I'd replace for:

Account not available (or similar)

because it could be used also for the case in which there are accounts in the app, but not the one that contains the item. Does it make sense? If not, i'd use: You need a logged account to open the link instead.

On the other hand, if the link format is not correct, error message displayed : Invalid deep link format. "Deep link" is a technical concept that should not be used in UI (confusing for non-technical users). Invalid link format sounds more friendly for every kind of user.

@manuelplazaspalacio
Copy link
Contributor Author

Finally I used an exiting text inside strings "No account found", to reuse that string. I changed the error message too.

JuancaG05
JuancaG05 requested changes Oct 26, 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 changes requested here @manuelplazaspalacio! Good job! 👍

owncloudApp/src/main/java/com/owncloud/android/presentation/authentication/LoginActivity.kt Outdated Show resolved Hide resolved
.../src/main/java/com/owncloud/android/presentation/files/operations/FileOperationsViewModel.kt Outdated Show resolved Hide resolved
.../src/main/java/com/owncloud/android/presentation/files/operations/FileOperationsViewModel.kt Show resolved Hide resolved
owncloudDomain/src/main/java/com/owncloud/android/domain/exceptions/DeepLinkException.kt Outdated Show resolved Hide resolved
owncloudDomain/src/main/java/com/owncloud/android/domain/exceptions/DeepLinkException.kt Outdated Show resolved Hide resolved
...loudDomain/src/main/java/com/owncloud/android/domain/files/usecases/ManageDeepLinkUseCase.kt Outdated Show resolved Hide resolved
...loudDomain/src/main/java/com/owncloud/android/domain/files/usecases/ManageDeepLinkUseCase.kt Outdated Show resolved Hide resolved
JuancaG05
JuancaG05 approved these changes Oct 26, 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!!

@jesmrec jesmrec merged commit b85c098 into master Oct 26, 2023
5 checks passed
@jesmrec jesmrec deleted the feature/deep_link_open_app branch October 26, 2023 10:45
@jesmrec jesmrec mentioned this pull request Jan 23, 2024
Closed
9 tasks
Aitorbp pushed a commit that referenced this pull request Feb 5, 2024
@jesmrec @Aitorbp
Merge pull request #4191 from owncloud/feature/deep_link_open_app
e89341d 
[FEATURE REQUEST] Manage deep links and open the app correctly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JuancaG05 JuancaG05 JuancaG05 approved these changes

Assignees
No one assigned
Labels
Approved by qa
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Manage deep links and open the app correctly
3 participants
@manuelplazaspalacio @jesmrec @JuancaG05