bugfix/error_signing_redirect: Added error handling #221

toaderandrei · 2020-10-20T06:46:24Z

Added a retrofit exception that transforms all the retrofit rx calls
to the retrofit exception. Retrofit exception is of 3 types, http,
network and unexpected.
Added WebViewException, RetrofitException and EmptyResultsException.
WebViewException is called when the webview fails to load an url.
EmptyResultsException is thrown in case the data retrieved from both
database and network is null or empty.
Added a base fragment, namely, BaseViewModelFragment where we handle
the error received from the viewmodel by overriding onError method.
Added error handling when login fails.
Replaced all this from methods like viewModel.obj.observer(this, ...)
with viewLifecycleOwner.
Created a few utility methods in Utils.
Improvd Result class so now it supports success and error checks.
Added error codes for not authorized, unknown and not found.
Added some error messages to the strings.xml.
Added logs for unknown actions.
Added todos for not fixed errors.

Requirements for making a pull request

Thank you for contributing to our project!

Please fill out the template below to help the project maintainers review it as fast as possible and include your contribution to the project.

What does it fix?

Closes #213 and #43

Please mention the main changes this PR brings.

How has it been tested?

Please describe the tests that you ran to verify your changes.

catileptic · 2020-10-20T07:23:06Z

This PR is quite hefty in size. Thank you, @toaderandrei, for your work! And especially for listing the changes you made, individually, in the PR body. We're going to review this, but it will take a bit longer than our usual code reviews.

Let's also chat on Slack about the changes you've made. You've introduced new dependencies in the architecture and we gotta discuss whether this is a good way to move forward.

lukstbit

LGTM 👍

There is some dead code(RetrofitException.getErrorBodyAs() and Result.error are not being used anywhere) but that is just nitpicking.

lukstbit · 2020-10-27T15:59:51Z

app/src/main/java/ro/code4/monitorizarevot/ui/forms/FormsViewModel.kt


    init {
        getForms()
+        //todo this does not work as expected.


Are you referring to not handling exceptions when fetching the polling station text?

It is connected with the comments from below where there is a null value in an rxjava operator. When this method is called for the first time, and neither api nor the db return something, the null values do not behave as expected. In RxJava1 it was valid, however, in rxjavsa2 it is not.

DeKaN · 2020-10-28T14:33:11Z

app/src/main/java/ro/code4/monitorizarevot/helper/Result.kt

- * .:.:.:. Created by @henrikhorbovyi on 13/10/19 .:.:.:.
+
+ * Class that encapsulates successful result with a value of type [T] or
+ * a failure with a [Throwable] exception.
 */
 sealed class Result<out T> {


Was it an attempt to reintroduce kotlin.Result<T>?

It was there before, I just added some things. Changing it to Kotlin.Result might take a bit more effort. I analyzed that already.

DeKaN · 2020-10-28T15:01:27Z

app/src/main/java/ro/code4/monitorizarevot/helper/Utils.kt

+    val s = SpannableString(message)
+
+    //added a TextView
+    val tx1 = TextView(this)


I think it's better to not create views in code

I just moved it so we can reuse it. I plan to change it in future versions.

DeKaN · 2020-10-28T15:03:13Z

app/src/main/java/ro/code4/monitorizarevot/repositories/Repository.kt

-            })
+            .subscribeOn(Schedulers.io())
+            .flatMap {
+                return@flatMap syncAnswers(it)


Why do you use explicit return?

No special reason.

DeKaN · 2020-10-28T15:19:06Z

app/src/main/java/ro/code4/monitorizarevot/ui/guide/GuideFragment.kt

+                    var message: String? = description
+                    if (TextUtils.isEmpty(message)) {
+                        message = "Unknown Error"
+                    }


Suggested change

var message: String? = description

if (TextUtils.isEmpty(message)) {

message = "Unknown Error"

}

val message = description.takeUnless { it.isNullOrEmpty() } ?: "Unknown Error"

DeKaN · 2020-10-28T15:22:29Z

app/src/main/java/ro/code4/monitorizarevot/ui/guide/GuideFragment.kt

            }
        }
        viewModel.url().observe(viewLifecycleOwner, Observer {
            webView.loadUrl(it)
        })
    }

+    override fun onError(thr: Throwable) {
+        logE(TAG, "Error loading the page:" + thr.message)
+        var messageId: String = getString(R.string.error_generic_message)


why not val messageId = if (...) R.string.error_no_connection else R.string.error_generic_message ?
and then just logE(TAG, getString(messageId))

DeKaN · 2020-10-28T15:55:55Z

app/src/main/java/ro/code4/monitorizarevot/exceptions/RetrofitException.kt

+/**
+ * Exception that is retrieved from retrofit. It is of three types, http, network and unexpected.
+ */
+open class RetrofitException internal constructor(


Why it's open? there are no any classes extending it

DeKaN · 2020-10-28T15:59:13Z

app/src/main/java/ro/code4/monitorizarevot/exceptions/RetrofitException.kt

+     * @throws IOException if unable to convert the body to the specified `type`.
+     */
+    @Throws(IOException::class)
+    fun <T> getErrorBodyAs(type: Class<*>?): T? {


No usages. Mostly API has one type for any errors, but it can be useful if the app's backend uses different types.

Yeah, I added it for a reason. I know it is not used at the moment. I plan to take on other user stories that will depend on that.

we should keep this PR as clean as possible

DeKaN · 2020-10-28T16:03:01Z

app/src/main/java/ro/code4/monitorizarevot/repositories/Repository.kt

        val observableApi = apiInterface.getForms()

+        // todo fix this as it does not work as expected. nulls are not treated as return values.
+        //  instead it throws a NullPointerException.


RxJava2 doesn't allow null as a return value, it's well-known.

Well, that is the comment here for. However, nonetheless the nulls were somehow added to the code and it does not work as expected. Thus, the comment you see.

DeKaN · 2020-10-28T16:05:39Z

app/src/main/java/ro/code4/monitorizarevot/repositories/Repository.kt

        val observableApi = apiInterface.getForms()

+        // todo fix this as it does not work as expected. nulls are not treated as return values.
+        //  instead it throws a NullPointerException.
        return Observable.zip(
            observableDb.onErrorReturn { null },
            observableApi.onErrorReturn { null },


@aniri I think an issue should be created for it.

I plan to fix it in a future task. I tried, but the idea was to not explode this PR.

yep, I'll add a new issue for this after we finish merging this PR

DeKaN · 2020-10-28T16:07:28Z

app/src/main/java/ro/code4/monitorizarevot/ui/login/LoginActivity.kt

+                        processHttpException(exception, fallback)
+                    }
+                    RetrofitException.Kind.NETWORK -> {
+                        var messageId: String = getString(R.string.error_generic)


val messageId = if (isOnline) R.string.error_generic else R.string.login_no_internet

* Added a retrofit exception that transforms all the retrofit rx calls to the retrofit exception. Retrofit exception is of 3 types, http, network and unexpected. * Added WebViewException, RetrofitException and EmptyResultsException. WebViewException is called when the webview fails to load an url. EmptyResultsException is thrown in case the data retrieved from both database and network is null or empty. * Added a base fragment, namely, BaseViewModelFragment where we handle the error received from the viewmodel by overriding onError method. * Added error handling when login fails. * Replaced all this from methods like viewModel.obj.observer(this, ...) with viewLifecycleOwner. * Created a few utility methods in Utils. * Improvd Result class so now it supports success and error checks. * Added error codes for not authorized, unknown and not found. * Added some error messages to the strings.xml. * Added logs for unknown actions. * Added todos for not fixed errors.

* Processed code review comments.

* Fixed the error message not being parsed from the request in case an error 400 is received. In case it is null, we show the generic error message.

…nges * Fixed bug in onPause that was causing the disposable not to finish. * Reverted some unnecessary changes.

* Removed unused classes and dead code.

sonarqubecloud · 2020-11-08T12:59:33Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
3 Code Smells

No Coverage information
0.0% Duplication

toaderandrei requested a review from aniri as a code owner October 20, 2020 06:46

aniri added hacktoberfest-accepted in-code-review in-functional-testing labels Oct 22, 2020

lukstbit mentioned this pull request Oct 25, 2020

use appropriate lifecycle owner instead of "this" #223

Closed

lukstbit reviewed Oct 27, 2020

View reviewed changes

DeKaN reviewed Oct 28, 2020

View reviewed changes

toaderandrei force-pushed the bugfix/error_signing_redirect branch from 674262a to 1762af9 Compare November 2, 2020 07:19

aniri requested a review from AlexandraDaraban November 3, 2020 19:06

AndreiToader-stanescu-TomTom added 2 commits November 7, 2020 11:24

bugfix/error_signing_redirect: Fixed code review comments

1855137

* Processed code review comments.

toaderandrei force-pushed the bugfix/error_signing_redirect branch from 1762af9 to 1855137 Compare November 7, 2020 10:27

bugfix/error_signing_redirect: Fixed error log not parsed

e9727af

* Fixed the error message not being parsed from the request in case an error 400 is received. In case it is null, we show the generic error message.

aniri added the needs-updates label Nov 8, 2020

AndreiToader-stanescu-TomTom added 2 commits November 8, 2020 13:54

bugfix/error_signing_redirect: fixed bug and reverted unnecessary cha…

86eee37

…nges * Fixed bug in onPause that was causing the disposable not to finish. * Reverted some unnecessary changes.

bugfix/error_signing_redirect: Removed unused classes

39b0a96

* Removed unused classes and dead code.

aniri mentioned this pull request Nov 30, 2020

[Improvement] An empty state placeholder for the forms #100

Open

aniri added the postponed-until-after-elections label Nov 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix/error_signing_redirect: Added error handling #221

bugfix/error_signing_redirect: Added error handling #221

toaderandrei commented Oct 20, 2020 •

edited

Loading

catileptic commented Oct 20, 2020

lukstbit left a comment

lukstbit Oct 27, 2020

toaderandrei Nov 1, 2020

DeKaN Oct 28, 2020

toaderandrei Oct 28, 2020

DeKaN Oct 28, 2020

toaderandrei Oct 28, 2020

DeKaN Oct 28, 2020

toaderandrei Oct 28, 2020

DeKaN Oct 28, 2020

DeKaN Oct 28, 2020

DeKaN Oct 28, 2020

DeKaN Oct 28, 2020

toaderandrei Oct 28, 2020

aniri Oct 28, 2020

DeKaN Oct 28, 2020

toaderandrei Oct 28, 2020

DeKaN Oct 28, 2020

toaderandrei Oct 28, 2020 •

edited

Loading

aniri Oct 28, 2020

DeKaN Oct 28, 2020

sonarqubecloud bot commented Nov 8, 2020

bugfix/error_signing_redirect: Added error handling #221

Are you sure you want to change the base?

bugfix/error_signing_redirect: Added error handling #221

Conversation

toaderandrei commented Oct 20, 2020 • edited Loading

Requirements for making a pull request

What does it fix?

How has it been tested?

catileptic commented Oct 20, 2020

lukstbit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toaderandrei Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 8, 2020

toaderandrei commented Oct 20, 2020 •

edited

Loading

toaderandrei Oct 28, 2020 •

edited

Loading