[Feature] Display of username and phone-number + [Refactoring] HTTP-requests, Json-mapping and error-handling

[loewenzahm]

Desription: After a successfull login, personal information (i.e. username and phone-number) are displayed above the list for bikes.

Steps: Extract information from user's json-object; Store info in shared preferences; Enhance/Update view of the MainActivity

Edit: issue #16

[cybre-finn]

Neat! Unfortunately, this is a larger chunk of code that cannot be reviewed so easily. So I'll leave this here for later, when I can try this on an Android phone. (or @lgehr could do this as well) Feel free to push me if I'm taking to long. I tend to procrastinate reviews in this repo. ^^

[loewenzahm]

By the way, the feature requires not only to be logged in but to log in. That means that for already logged in users, the MainActivity will show "unknown" as name and phone number. But that changes immediately after they log out and in again.

That is because the personal information are only requested in the LoginActivity and then stored in the SharedPreferences. The MainActivity then only uses the values stored in the SharedPreferences.

In a future version we could add a function in the MainActivity that checks if the loginKey is stored in the SharedPreferences for situations in which the personal data is missing. In this case the personal information could be requested via the api in the MainActivity as well. That would solve the situation for already logged in users (i.e. users that have a loginKey but no personal info in the SharedPreferences).

[loewenzahm]

@h0chi & @lgehr : I refactored the LoginActivity (HTTP-requests with Volley + Json-mapping with Jackson + Error displaying using Snackbar). We/I could do that for the other activities as well if you are okay with the way it is done now. Feel free to ask any questions or if you would like to talk about the changes.

[lgehr]

Thanks for the PR. 🎉🙏 I did not have time to test everything in detail but I have some notes:

For 50de8c7: I do like the feature. Although I don't really like that the save the userinformation locally. I would rather have it be requested every time. (Since the scree_name can change?!, I did not know that nextbike has usernames...). I think also this feature should tell the user the information the server has of them rather then what the client assumes the server has.
This would also fix the "bug" that the user has to logout and in to get the information.

For 3c7fa68: Volley seems to be a great choice to implement new features quickly. For error handling we could send show_errors=true with our requests. Then the server will reply with an error-code which volley can use (Although I do not know what volley makes of it). In the future we could also display the error-msg from the server. See nextbike-api
If we use a JSON serializing library I think it would make sense to also serialize our request and not only the replies with it. But only de-serializing the replies is maybe better then nothing?! :D

These are just my 2 cents. I think they should not stand in the way of merging something because I did not find the time to do any great contribution to this project.

Kind Regards!

[loewenzahm]

I really appreciate your feedback and ideas - thanks! :pray:

And I totally agree on your suggestion to access the user's data not only on login but every time the app is used - can't understand myself why I implemented it the other way. I will fix that in the next days. :)

And yes, further error handling should definitely be something to be considered in the future... :smile:

[loewenzahm]

@lgehr I tried to change the way (or better the situation in which) we access the user's name and phone number as discussed above. Unfortunately we can't access this information with another API call than the one we use to authenticate the user. The reason for that is that the API calls for getting user information seem not to work with our API key. I tried getting user information with both, the official API's method (getUserInfo) as well as with the endpoint listed in h0chi's reverse engineered API (api/v1.1/getUserDetails.json). For both approaches I get error code="17" message="access denied for API key". Is it possible that our API key has limited access concerning that data, @h0chi ? (Other API calls work as usual, by the way.)

For now I would suggest that we keep it that way (despite you have an idea to solve that problem) and maybe merge the PR? :)

[lgehr]

Hey. Sorry I had to close the PR to force push a new commit which rewrote the git history. Reason is that the nextbike people asked us to not have the apikey we used before in our history.

[lgehr]

I think we get the merge errors now because of the forced history rewrite... @loewenzahm Could you look into this?

[lgehr]

Hey @meierjan could we get access to getUserInfo with our apikey? Thanks!

[meierjan]

Done. Might take an hour for the cache for invalidate. :) 

[loewenzahm]

I think we get the merge errors now because of the forced history rewrite... @loewenzahm Could you look into this?

Done. :)

[loewenzahm]

For 50de8c7: I do like the feature. Although I don't really like that the save the userinformation locally. I would rather have it be requested every time. (Since the scree_name can change?!, I did not know that nextbike has usernames...). I think also this feature should tell the user the information the server has of them rather then what the client assumes the server has. This would also fix the "bug" that the user has to logout and in to get the information.

@lgehr I addressed that concern with 881f95d2b9665c3ba7c51d11d69b231fb6cb3899 . Now these information are requested from within the MainActivity every time they are needed instead of being stored locally. Thanks for setting the base for this solution by asking for the enhancement of our api key. :smile:

@meierjan Thank you as well for giving us access to the relevant data :pray:

[loewenzahm]

@lgehr @h0chi Do you think you could merge this or are there other things I must change first? :)

[lgehr]

Sorry for the really late response.

I have problem applying the patch to my local repo. I also find it rather strange that this PR has 69 commits, because of the history rewrite. I would like for this PR to be just the one commit is really provides. (It hopefully will be easier to apply it locally then.)

Maybe you could make a new PR from a new branch which just cherry-picks the topcommit. Thanks!

[cybre-finn]

Any idea what happened here? B/c at least from the github GUI the commits (the two I checked) including the initial have the same hashes...

[cybre-finn]

I looked into it and it looks like @loewenzahm somehow managed to have a copy of every commit (as in reapplied?) in their git history. This breaks the commit graph of @loewenzahm's repo and should not be committed into our master.

Possible solutions are:

• @loewenzahm could try to somehow cherry-pick the top commit which contains the relevant feature as @lgehr said.
• We could try to squash and merge this PR (however, I would prefer to do this locally or to another branch)
• @loewenzahm could delete their repo, close this PR, fork again and manually apply and commit their change again, then open another PR.

I could fix and merge this locally, but I do not know if the feature is mature.