Bug fix #1504 #1506

ashishkumar468 · 2018-05-10T11:05:04Z

Description (required)

Fixes #{#1504}

{Subscribed to onError method in NearByActivity to handle exceptions at the source}

Tests performed (required)

Tested on {API level 21 & google x_86}, with {build variant, ProdDebug}.

Screenshots showing what changed (optional)

NA

codecov-io · 2018-05-10T11:32:55Z

Codecov Report

Merging #1506 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #1506      +/-   ##
=========================================
- Coverage    3.14%   3.14%   -0.01%     
=========================================
  Files         136     136              
  Lines        7381    7387       +6     
  Branches      714     714              
=========================================
  Hits          232     232              
- Misses       7134    7140       +6     
  Partials       15      15

Impacted Files	Coverage Δ
...a/fr/free/nrw/commons/nearby/NearbyController.java	`0% <ø> (ø)`	⬆️
.../java/fr/free/nrw/commons/nearby/NearbyPlaces.java	`0% <ø> (ø)`	⬆️
...ava/fr/free/nrw/commons/nearby/NearbyActivity.java	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ab6a21...36a4b7d. Read the comment docs.

maskaravivek

Looks good to me! IMO this should fix the bug. The issue was that exceptions were not being handled properly, thus leading to a crash.

Needs to be tested once on a real device.

sivaraam · 2018-05-10T16:15:53Z

I tried building an app which includes this fix and noted that I get an error snackbar. BTW, the snackbar has info that possibly the user shouldn't be seeing. Maybe we should be using a generic message such as "Error while trying to load nearby places."

ashishkumar468 · 2018-05-10T16:35:59Z

@sivaraam Thanks for the update, will fix it it and update

ashishkumar468 · 2018-05-10T16:43:19Z

Hi @sivaraam I have made a fix for the same. I am not able to reproduce the same exception on my device. Can you please verify it now.

sivaraam · 2018-05-10T17:29:51Z

app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java

+                                    if (throwable instanceof UnknownHostException || throwable instanceof ConnectException) {
+                                        showErrorMessage(getString(R.string.slow_internet));
+                                    } else {
+                                        showErrorMessage(throwable.getMessage());


May be we should replace this with showErrorMessage(R.string.error_loading_nearby) with error_loading_nearby as string for "Error while loading Nearby locations that need images" or something similar. (I guess conveying the fact "... Nearby locations that need images" in the error would be informative part of the error though not related to the actual error that has occured ;-))

We could put throwable.getMessage() where it's appropriate, the log. This way the exception isn't visible to the user.

Alternatively, we could try and find the possible exceptions that happen and provide a more user-friendly message for each exception. This should improve the user experience. Anyway, showing the exception message to the user would in no way be helpful to the non-technical users.

sivaraam · 2018-05-10T17:34:36Z

@ashishkumar468 I could check that but I don't think showing the message "You seem to have slow internet connection" would be appropriate in this case. That's because there was no internet connection at all. To be more precise technically the java.net.ConnectException seems to be thrown "typically when the connection was refused remotely (e.g., no process is listening on the remote address/port).". So, I don't think it's appropriate to show the slow internet error in this case. I've mentioned what I actually have in mind as an inline comment.

ashishkumar468 · 2018-05-11T07:40:06Z

@sivaraam True, Lets put a generalised message, like you said, "Error while loading nearby locations". Later on we can figure out what all errors are possible for this scenario and may be include specific messages for such scenarios. Is everybody okay with this ?

maskaravivek · 2018-05-11T08:18:40Z

Sounds good to me. Having a generic message and the app not crashing would be great. @ashishkumar468 can you go ahead and make the changes.

ashishkumar468 · 2018-05-11T08:38:48Z

@maskaravivek Okay, will make the necessary changes and update

…s-app#1504]

sivaraam

A few fine points.

sivaraam · 2018-05-11T13:50:52Z

app/src/main/res/values/strings.xml

@@ -282,7 +282,6 @@

  <string name="share_app_title">Share App</string>
  <string name="share_coordinates_not_present">Coordinates were not specified during image selection</string>
-  <string name="something_went_wrong">Something went wrong</string>
-  <string name="slow_internet">You seem to have slow internet connection</string>
+  <string name="error_fetching_nearby_places">Error fetching nearby places.</string>


How about changing the message to "Error fetching nearby places that need images" thus mentioning what Nearby should actually be used for, as stated in my comment previously)? What Nearby should be used for isn't mentioned in a lot of places. So, it might be helpful to the user who see the error message.

IMO, If it's not mentioned at other places then just mentioning it in the error messages won't be that useful. After #706 this should be a bit more clear. :)

sivaraam · 2018-05-11T13:52:29Z

app/src/main/java/fr/free/nrw/commons/nearby/NearbyActivity.java

-                                        showErrorMessage(throwable.getMessage());
-                                    }
+                                    Timber.d(throwable);
+                                    showErrorMessage(getString(R.string.error_fetching_nearby_places));


How about adding a TODO: give meaningful error messages for each specific exception that might occur here thus documenting the fact that this has to re-visited. It might even kindle people who accidentally see the TODO to work on it 😉

@sivaraam Am going ahead and merging this PR. Feel free to create a new issue for the above and fix it if possible. :)

maskaravivek · 2018-05-12T11:19:40Z

Tested on a real device. The app didn't crash for me. Going ahead and merging it.

sivaraam · 2018-05-12T23:27:51Z

The app didn't test for me.

I think you actually meant The app didn't crash for me., right?

maskaravivek · 2018-05-13T08:21:20Z

@sivaraam Yes. Typo!

* Bug fix #1504 * Filtered messages with ConnectException [issue #1504] * A generalised message for exceptions in Nearby Activity [issue #1504]

…rby (#1495) * Localisation updates from https://translatewiki.net. * Integrate API for displaying featured images (#1456) * Integrate API for displaying featured images * Add pagination and refactor code so that it can be reused for category images * Add license info to the images * Fix author view * Remove unused values * Fix minor issues with featured images * Fix null license url issue * Remove some log lines * Fix back navigation issue * fix tests * fix test inits * Gracefully handling various error situations * Added java docs * Update pull_request_template.md (#1476) * Update pull_request_template.md * Remove Javadocs mention * Added required/optional notes * resolves #1464 : MediaDataExtractor is making inefficient (redundant) server calls (#1496) * Open map of place where picture was taken (#1360) * Intent to map added * Merge conflicts resolved * Added the functionality to hide FAB incase of null coordinate * Merge Conflict resolved * Improve pr quality * Improve Quality * Added nested FAB animations * Nested FAB implemented * Improve Quality * Added up arrow * Javadocs Added * Add nearby tutorial (#1467) * Add dependency for MaterialShowcase * Add actionview class to get a reference to material showcase * Create a NearbyMaterialShowcaseSequence class * Apply sequence steps * Add first three steps of nearby showcase * Add sequence id constants to make sure they will be displayed only once * Add last step of sequence to explain plus fab * Create an object to prevent customize all sequences every time * Fix typo * Code cleanup * Add strings to strings.xml * Code cleanup * Revert irrelevant change * Revert irrelevant change * Remove showcaseview for recenter button * Use single showcaseView instead of sequence * Add single showcase view insted of sequence to be able to edit text style * Make sure it will be displayed only once * Cleanup * Update strings * Change dismiss text style * CONTRIBUTING: fix formatting of the gist of the guidelines (#1453) * CONTRIBUTING: fix formatting of the gist of the guidelines First level headings for a gist seems to be overkill. So, replace first level headings with an ordered-list which sounds more meaningful. * CONTRIBUTING: specify clearly that 'blame' is a feature of "Git" The contributing file specifies about the ability to know who wrote something without the need of @author javadoc tags but incorrectly attributes the feature to GitHub. Correctly attribute the feature to where it belongs, Git, and specify the name of the feature to help users easily take advantage of it. * Feature/switch to butterknife (#1494) * Implemented butterknife in MediaDetailFragment [issue #1491] * Implemented butterknife in MediaDetailPagerFragment [[issue #1491]] * post merge upstream master wip [[issue #1491]] * Localisation updates from https://translatewiki.net. * Bug fix #1504 (#1506) * Bug fix #1504 * Filtered messages with ConnectException [issue #1504] * A generalised message for exceptions in Nearby Activity [issue #1504] * Localisation updates from https://translatewiki.net. * Fix security exception crash while accessing network location provider (#1498) * Fix security exception crash while accessing network location provider * Added java docs * Localisation updates from https://translatewiki.net. * Log P18 edits to wikidata corresponding wikidata entity on uploading a nearby image * Added java docs * Fix test build * Refresh nearby * Refresh nearby list on successful edit * Java docs * Make authenticated wikidata edits * Updated toast message to show entity name that was edited

Bug fix commons-app#1504

6b043db

maskaravivek approved these changes May 10, 2018

View reviewed changes

Filtered messages with ConnectException [issue commons-app#1504]

20d3c37

commons-app deleted a comment May 10, 2018

sivaraam requested changes May 10, 2018

View reviewed changes

A generalised message for exceptions in Nearby Activity [issue common…

36a4b7d

…s-app#1504]

commons-app deleted a comment May 11, 2018

sivaraam requested changes May 11, 2018

View reviewed changes

maskaravivek merged commit b6e4fb2 into commons-app:master May 12, 2018

maskaravivek pushed a commit that referenced this pull request May 18, 2018

Bug fix #1504 (#1506)

8bdc4f6

* Bug fix #1504 * Filtered messages with ConnectException [issue #1504] * A generalised message for exceptions in Nearby Activity [issue #1504]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fix #1504 #1506

Bug fix #1504 #1506

ashishkumar468 commented May 10, 2018

codecov-io commented May 10, 2018 •

edited

Loading

maskaravivek left a comment

sivaraam commented May 10, 2018 •

edited

Loading

ashishkumar468 commented May 10, 2018

ashishkumar468 commented May 10, 2018

sivaraam May 10, 2018

sivaraam commented May 10, 2018

ashishkumar468 commented May 11, 2018

maskaravivek commented May 11, 2018

ashishkumar468 commented May 11, 2018

sivaraam left a comment

sivaraam May 11, 2018

maskaravivek May 12, 2018

sivaraam May 11, 2018

maskaravivek May 12, 2018

maskaravivek commented May 12, 2018 •

edited

Loading

sivaraam commented May 12, 2018

maskaravivek commented May 13, 2018

Bug fix #1504 #1506

Bug fix #1504 #1506

Conversation

ashishkumar468 commented May 10, 2018

Description (required)

Tests performed (required)

Screenshots showing what changed (optional)

codecov-io commented May 10, 2018 • edited Loading

Codecov Report

maskaravivek left a comment

Choose a reason for hiding this comment

sivaraam commented May 10, 2018 • edited Loading

ashishkumar468 commented May 10, 2018

ashishkumar468 commented May 10, 2018

sivaraam May 10, 2018

Choose a reason for hiding this comment

sivaraam commented May 10, 2018

ashishkumar468 commented May 11, 2018

maskaravivek commented May 11, 2018

ashishkumar468 commented May 11, 2018

sivaraam left a comment

Choose a reason for hiding this comment

sivaraam May 11, 2018

Choose a reason for hiding this comment

maskaravivek May 12, 2018

Choose a reason for hiding this comment

sivaraam May 11, 2018

Choose a reason for hiding this comment

maskaravivek May 12, 2018

Choose a reason for hiding this comment

maskaravivek commented May 12, 2018 • edited Loading

sivaraam commented May 12, 2018

maskaravivek commented May 13, 2018

codecov-io commented May 10, 2018 •

edited

Loading

sivaraam commented May 10, 2018 •

edited

Loading

maskaravivek commented May 12, 2018 •

edited

Loading