Fixed constant display of error message when Social tab without Token #1849
Conversation
Pull reviewers statsStats of the last 30 days for popstellar:
|
There was a problem hiding this comment.
Overall, I don't think that your solution solves the issue in the right way (but also that solves the issue at all, I tested the code and it seems that the message keeps spamming).
If we want the message of the poptoken missing appearing only once, the right approach would be an ad-hoc check done in the fragment creation, which maybe sets a boolean constant that tells whether the user has a valid pop token. Then, all the current methods in the social media view model which catch and log a key exception must be kept to check errors (but simply made silent in case of no token). This is obviously a much longer fix to do and I'm also happy with a more provisional solution, as long as it works though.
I suggest you test this with 2 emulator instances so you can actually simulate this behaviour.
| } catch (e: InvalidPoPTokenException) { | ||
| // InvalidPoPTokenException means the user did not attend any roll calls and had no token. | ||
| // This is not an issue worth displaying the error for every single chirps. | ||
| // This error will already show once in isOwner. | ||
| return false | ||
| } catch (e: KeyException) { | ||
| logAndShow(getApplication(), TAG, e, R.string.error_retrieve_own_token) | ||
| false |
There was a problem hiding this comment.
To simplify a bit the code clarity you can exploit the fact that InvalidPoPTokenException is a subclass of KeyException, so you can use the catch only once for the most generic KeyException and use an if to logAndShow
| } catch (e: InvalidPoPTokenException) { | |
| // InvalidPoPTokenException means the user did not attend any roll calls and had no token. | |
| // This is not an issue worth displaying the error for every single chirps. | |
| // This error will already show once in isOwner. | |
| return false | |
| } catch (e: KeyException) { | |
| logAndShow(getApplication(), TAG, e, R.string.error_retrieve_own_token) | |
| false | |
| } catch (e: KeyException) { | |
| if (e !is InvalidPoPTokenException) { | |
| logAndShow(getApplication(), TAG, e, R.string.error_retrieve_own_token) | |
| } | |
| false |
|
I've made some updates to the token check implementation. Here's a quick rundown:
Now, when opening the Social tab, we are not even getting a single toast error, but we are only getting it when trying to send a chirps while not having the token, I think it makes more sense. Finally, the error should trigger if there is an issue with the retrieval of the token, other than it being uninitialized. This should handle the spamming issue more gracefully by centralizing the token validation and making our error handling smarter. Let me know if this works well on your end too. |
Tests are failing because it hasn't been formatted, so remember to format and then either squash the commit or push it as is |
|
|
…error-msg' into work-fe2-maxime-constant-social-error-msg # Conflicts: # fe2-android/app/src/test/framework/robolectric/java/com/github/dedis/popstellar/testutils/UITestUtils.kt # fe2-android/app/src/test/ui/robolectric/com/github/dedis/popstellar/ui/lao/socialmedia/ChirpListAdapterTest.kt # fe2-android/app/src/test/ui/robolectric/com/github/dedis/popstellar/ui/lao/socialmedia/SocialMediaHomeFragmentTest.kt
…error-msg' into work-fe2-maxime-constant-social-error-msg
| @@ -60,6 +61,8 @@ constructor( | |||
| private val mNumberCharsLeft = MutableLiveData<Int>() | |||
| val bottomNavigationTab = MutableLiveData(SocialMediaTab.HOME) | |||
|
|
|||
| private val _hasValidToken = MutableLiveData<Boolean>() | |||
There was a problem hiding this comment.
How come you decided to use this name convention here (I mean underscore to start the variable name)
There was a problem hiding this comment.
It is supposed to be the convention for mutable and private data. Copilot chose the name for me, and it felt normal since I've already come across this kind of naming. But i recon it can be misleading and there should be no issue not following this dark and unknown convention
| @@ -109,7 +112,7 @@ constructor( | |||
| val addChirp = AddChirp(text, parentId, timestamp) | |||
|
|
|||
| return Single.fromCallable { validPoPToken } | |||
| .doOnSuccess { token: PoPToken -> | |||
| .doOnSuccess { token: PoPToken? -> | |||
There was a problem hiding this comment.
Why did you change that? Technically the token cannot be null there: if a KeyException error is thrown and then become null, the doOnSuccess won't be triggered anyway (as it won't be a success)
There was a problem hiding this comment.
.... It helped raising the coverage 😅 (since the tests are having trouble creating valid data, some of them just could not get passed this without the "?"
| .subscribeOn(schedulerProvider.io()) | ||
| .observeOn(schedulerProvider.mainThread()) | ||
| .subscribe( | ||
| { _ -> _hasValidToken.postValue(true) }, |
There was a problem hiding this comment.
You can avoid the _ -> here
| @@ -287,7 +293,9 @@ constructor( | |||
| val toSearch = token.publicKey.encoded | |||
| senders.contains(toSearch) | |||
| } catch (e: KeyException) { | |||
| logAndShow(getApplication(), TAG, e, R.string.error_retrieve_own_token) | |||
| if (_hasValidToken.value == true) { | |||
There was a problem hiding this comment.
Can't you remove the "== true" in the if ?
There was a problem hiding this comment.
I am not sure I can since the value is Boolean? (could be null), but I'll check it out while addressing the other comments :thum
There was a problem hiding this comment.
In fact, I could not. I tried setting up the value to false in an early stage but did not change it, I guess it's really just a detail
|
|
|
|
|
addressing issue #1848
Disabled error toast when the error is
InvalidPoPTokenException, otherwise it would just pop multiple times for every single chirps. The same error is displayed once on opening the Social Tab inisOwnerso the user is still aware that no token is available.