cleanup logout

[ramack]

in #183 we have added a Logout option, which has some trouble, that after logout the login screen is shown, instead of the ManageAccounts...

On the other side it is not even clear why we have it.

Maybe @radass1996 can summarize here, what the use case of the Logout is, as I don't remember why we need it in the UI, for me it could also be good to technically logout on selecting another than the active account, or if the account is deleted.

Edit: found it. @radass1996 described it here Seems to be only relevant for the case after the last account was deleted. So here it is maybe best to just show the loginActivity after the last account was deleted. And the logout on deletion still makes sense, so I think the Logout should just be removed from the UI, while the code is good to keep.

[radass1996]

I think that there are few edge cases.

You can remove one of many signed accounts from your application without calling LOGOUT call. Is this good. Should we not cal Logout call after we removed account ?

When you remove last account. You are still logged. But you can not download any albums or other stuff. I think in this case User should be moved back to Login screen.

From logout activity should be not possible go to Main Menu via back button.

When you are signed in with account A. You try to add new account B in ManageAccount activity and Login screen appears. You change your mind and you wanna log with account A again. And snackbar will appear with message: "This account is already signed on the server" So you are not able to make relog. Then when you try to reinstall app, you are suddenly in Main Activiry again.

[ramack]

ah, thanks for your response. I modified the text above already in parallel.

I agree that we should do a technical LOGOUT via REST in case the account is switched or deleted, that will also be done for the last account in this case, and we should set the "activeAccount" to null. After that I agree, showing the LoginActivity to add a new account is the right thing to do.

We even should consider the case, that the account was modified from the android system settings. So we have to check that the callbacks that are executed in those cases are implemented correct.

Do you want to care about that?

[coolo]

for this to test cleanly we need a little more than unit tests. I'm looking into espresso, but the question is what to test against. What do you think about adding https://github.com/square/okhttp/tree/master/mockwebserver as dependency to verify not only the UI part but also that REST logout was called?

I may be slow at it, but I'd would like to explore this unless you say it's completly nuts.

[ramack]

We are getting little off topic from this issue here but anyhow:

Testing is a very important topic. Currently we have ~0 testing, not for the logout and not for anything else :-(

For the logout we currently do not even have a decided concept and even though I think we should call logout via REST I don't consider it as crucial if we don't have a good test coverage for it.

There are some automated testing approaches, e. g. with firebase as described in #164 which basically do random clicks on the UI and checks that we don't have a crash. This is probably already very helpful for UI testing if we manage to have a publicly accessible gallery with an "account" for firebase. Together with some handwritten test cases in espresso for problems we have had or even better as TDD for new features we would already be quite ok.

On the other side if you are interested in doing a deeper test with a mockweberver for sure that is also highly appreciated!

[coolo]

I don't want to mess with coding logout if I can't test it. I can setup a test instance on my server for development, but running tests against it would put your CI at risk of me screwing up php (and I'm good at it :)

So mocking the actual HTTP traffic recorded against the test instance would help that.

BTW, on the topic of off topic (pun intended) - how about setting up a gitter channel for such questions/ideas?

Anyway, I'm making some progress on learning espresso.

[ramack]

I have a piwigo-test installation on procreo.de/piwigo that I use for testing I think we can use that for CI and as backup I guess @plegall can also setup a piwigo server, so if we keep the base URL of teh test server configurable in the tests it should be easy to switch - or even use multiple ones with different piwigo server versions, php versions, piwigo configurations etc... And the current logout code was written by a student without any testing and I was not really happy during the review but still merged it due to other parts in that PR, so it can only improve :-)

About instant messaging: piwigo team has a channel on their mattermost server for the development of the iOS and Android apps, we could use the #Piwigo IRC channel on freenode or you could reach me via XMPP. For sure gitter is also cool, but I am not really keen on yet another communication channel, I hope you understand...

For espresso: would you like to create a new issue to add espresso as a testing framework? - Then I could assign it to you and we'd be sure that we avoid double work. (Not a real issue here, but I think in general it would be good practice to have a process of "issue - discussion - assignment - coding - RP - review - merge - celebrate")

And: did I get it right, that you want to work on this logout issue here?

[coolo]

Actually I only picked it as mean to TDD something real. The issue itself is small enough to concentrate the effort on testing it.