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

[New arch] Complete login #2826

Merged
merged 49 commits into from
Apr 14, 2020
Merged

Conversation

davigonz
Copy link
Contributor

@davigonz davigonz commented Feb 26, 2020

Implements:

Needs owncloud/android-library#309

Improvements

We have replaced old png icons with vectored ones

We have included an assert to check connectivity before calling a network operation in viewModels (included in new architecture).

Uses that require connection until this PR:

  • OCShareViewModel: RefreshSharesFromServerAsyncUseCase
  • OCShareViewModel: DeleteShareAsyncUseCase
  • OCShareViewModel: EditPrivateShareAsyncUseCase
  • OCShareViewModel: CreatePrivateShareAsyncUseCase
  • OCShareViewModel: CreatePublicShareAsyncUseCase
  • OCShareViewModel: EditPublicShareAsyncUseCase
  • OCCapabilityViewModel: RefreshCapabilitiesFromServerAsyncUseCase

Included in this PR:

  • OCAuthenticationViewModel: LoginBasicAsyncUseCase
  • OCAuthenticationViewModel: LoginOAuthAsyncUseCase
  • OCAuthenticationViewModel: GetServerInfoAsyncUseCase

QA

Test plan:

https://github.com/owncloud/QA/blob/master/Mobile/Android/Release_2.15/2826-Authentication.md

BUGS & IMPROVEMENTS

@jesmrec
Copy link
Collaborator

jesmrec commented Mar 17, 2020

(7) [FIXED]

  1. Enter an OAuth2 account
  2. Go to edit account and click the key icon
  3. Switch user and enter correct credentials of other different user

Current: account with new user is created
Expected: At least, the previous behaviour is preventing overwriting accounts. Username can not be changed (as well as in report 6.). When the other user credentials were entered, the message displayed was The entered user does not match the user of this account

Android 10

@jesmrec
Copy link
Collaborator

jesmrec commented Mar 18, 2020

(8) [FIXED]

About redirections

  1. Set an URL with a 301 redirection
  2. Enter correct credentials

Current: error: it was not found
Expected: correct login

I guess the problem is in the GET request just after clicking "Log in". This is the sent request:

GET http://<redirected_url>/remote.php/dav/files//ocs/v2.php/cloud/user?format=json

and this is the one sent in stable 2.14.2 version with the same scenario:

GET http://<redirected_url>/ocs/v2.php/cloud/user?format=json

Android 10

@jesmrec
Copy link
Collaborator

jesmrec commented Mar 18, 2020

(9) [FIXED]

Following (8)

With an URL with 302 redirection and performing the same steps, login is not posible either.

Error message: uriString

The problem might be the same as (8), but checking the requests flow, i did not see any 40x response, so it looks a little weird.

Android 10

@abelgardep
Copy link
Contributor

(1)

Check how is the "New to owncloud" link placed in the login view, in portrait:
2.14.2 current
Screenshot_1584448263 device-2020-03-17-133222

IMHO, the link placed there makes the stuff (icon, edit text, link) to be too close together. I'd go for the link to be placed as before (a little below)

What do you think?

Previously, server_auth_text visibility changed from visible to invisible and vice versa
Currently, visibility is changing from gone to visible and vice versa

That's why it is a little change there, i can update it to keep the previous behaviour.

@abelgardep
Copy link
Contributor

abelgardep commented Mar 18, 2020

(2)

1. Set a server in maintenance mode (`docker exec -ti <cont_id> occ maintenance:mode --on `)

2. Type the URL

Current: The server does nor support this authentication method
Expected: Server in maintenance mode (or similar)

Android 10

About this, previous behaviour shows message from response body directly
Let's do the same 👍

@abelgardep
Copy link
Contributor

abelgardep commented Mar 18, 2020

(3)

1. Enter an https url which server contains a self-signed certificate -> Certificate Dialog is shown.

2. Click "No"

Current: Status message stays on Testing connectionforever
Expected: Specific/static message about the status after denying. In the current stable, the message is Server certificate is not trusted.

Android 10

Sure

Tasks to solve this:

  • Update server_status_text text at onCancelCertificate (at LoginActivity line 467)

@abelgardep
Copy link
Contributor

network_error_socket_exception

(4)

When the connection is not stablished, you are using the following string:

<string name="network_error_socket_exception">an error while connecting with the server</string>

that lacks a verb, or the initial an is leftover.

That message is displayed in two different cases:

* Server not reachable

* No internet connection

any way to distinguish?

We receive a ResultCode.WRONG_CONNECTION at RemoteOperationHandler, any idea @davigonz ?

@abelgardep
Copy link
Contributor

(5)

This is in the same path as (2)

1. Type correct URL

2. Once URL is validated and credentials' fields are displayed, set maintenance mode

3. Type credentials

Current: unknown error
Expected: Server in maintenance mode (or similar)

Android 10

Related with #2826 (comment)

We are not checking ResultCode.SERVICE_UNAVAILABLE (503 Service Unavailable) at ThrowableExt, this is the resultCode we receive when server is under maintenance. We could show something like "Service currently not available" or similar in that case. @davigonz @jesmrec

@davigonz
Copy link
Contributor Author

davigonz commented Mar 18, 2020

(5)

This is in the same path as (2)

1. Type correct URL

2. Once URL is validated and credentials' fields are displayed, set maintenance mode

3. Type credentials

Current: unknown error
Expected: Server in maintenance mode (or similar)
Android 10

Related with #2826 (comment)

We are not checking ResultCode.SERVICE_UNAVAILABLE (503 Service Unavailable) at ThrowableExt, this is the resultCode we receive when server is under maintenance. We could show something like "Service currently not available" or similar in that case. @davigonz @jesmrec

Let's keep it with the same behaviour we had before, i.e. showing the response body message.

@davigonz
Copy link
Contributor Author

davigonz commented Mar 18, 2020

(10) [FIXED]

  1. Log in the app
  2. Disable wifi
  3. Swipe down to refresh the list of files

Current: crash appears
Expected: list of files properly refreshed

Android 10

@abelgardep
Copy link
Contributor

Changes applied, @jesmrec. Ready to test again 👍

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 8, 2020

Most of the reports are fixed, but a couple of glitches:

(3)

Problem is fixed, message is correct after denying the certificate. But, the icon in the status message is he following one:

Screenshot 2020-04-08 at 12 47 16

is that correct? in stable the icon there is like the following one:

https://github.com/owncloud/android/blob/master/owncloudApp/src/main/res/drawable-hdpi/ic_warning.png

(7)

Performing the steps, the functional behaviour is correct, but, a blank space appears between the URL and the status message:

Screenshot 2020-04-08 at 12 57 03

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 13, 2020

both (3) and (7) fixed, let's start the 2nd round

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 13, 2020

Everything finished, completed and fixed. This is approved on my side.

@abelgardep abelgardep merged commit 591d5d1 into new_arch/login Apr 14, 2020
@abelgardep abelgardep deleted the new_arch/complete_login_flow branch April 14, 2020 06:19
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.

[New arch] Refactor AuthenticatorActivity [New Arch] Account manager - account creation after login
3 participants