fix(repository): fix repo view when repos return 404 #726

dhamanutd · 2018-02-14T04:29:35Z

instead of blank view, now will show not found message when error 404

BREAKING CHANGE: fix repository view when the repos 404 not found

Question	Response
Version?	v1.4.1
Devices tested?	OnePlus One
Bug fix?	yes
New feature?	yes/no
Includes tests?	no
All Tests pass?	yes
Related ticket?	#690

Screenshots

Before	After

Description

Change action reducer getRepository to return Promise reject when response status code is not 200 OK
Make notFoundMsg props in the repository screen and component to take string from github api when return 404.
Change repository reducer in GET_REPOSITORY.ERROR to return initialState instead of recent state.
- In purpose to make repository screen not showing any info from latest state, if there are error 404.
Create logic to show not found messages, hide owner section, and disabled view code button if error 404.

coveralls · 2018-02-14T04:36:18Z

Coverage increased (+0.08%) to 44.863% when pulling 7f23a51 on dhamanutd:repo-screen-problem-when-repo-404 into 3b92054 on gitpoint:master.

chinesedfan

@dhamanutd Great new feature! Waiting for your feedbacks about my comments.

chinesedfan · 2018-02-14T07:40:19Z

src/repository/screens/repository.screen.js

@@ -49,6 +49,7 @@ const mapStateToProps = state => ({
  isPendingTopics: state.repository.isPendingTopics,
  isPendingSubscribe: state.repository.isPendingSubscribe,
  topics: state.repository.topics,
+  notFoundMsg: state.repository.error.message,


Personally, I prefer errorMessage or error directly. Then we can handle other types of errors and other information about errors in the future.

chinesedfan · 2018-02-14T07:41:25Z

src/repository/screens/repository.screen.js

              underlayColor={colors.greyLight}
+              disabled={(notFoundMsg && true) || false}


disabled={!!notFoundMsg} will be simpler.

chinesedfan · 2018-02-14T07:50:06Z

src/components/repository-profile.component.js

@@ -12,6 +12,7 @@ type Props = {
  loading: boolean,
  subscribed: boolean,
  locale: string,
+  notFoundMsg: string,


I think notFoundMsg is an inner state, which will not be customized from outside frequently. So we can add a new prop called error, hasError, or simply determinate it by repository.name.

chinesedfan · 2018-02-14T07:55:17Z

src/components/repository-profile.component.js

@@ -133,7 +135,7 @@ export const RepositoryProfile = ({
        )}

      <Text style={[styles.languageInfoTitle]}>
-        {repository.language || ' '}
+        {notFoundMsg || repository.language || ' '}


Try to add a new field in all files of locale/languages, say repository.unknowLanguage or something else you think better. Then rewrite as,

repository.language || translate('repository.unknowLanguage', locale)

@chinesedfan can i use google translate for another translation?

@dhamanutd No, please just leave them in English. Other contributors will pick them up and translate.

Nice feature. Just an observation, the key has a typo, should be unknownLanguage.

chinesedfan · 2018-02-14T07:58:11Z

src/components/repository-profile.component.js

+          (notFoundMsg && 'stop') ||
+          (repository.fork && 'repo-forked') ||
+          'repo'
+        }


As we forbid nested ternary expressions, please extract as a function to make it more readable.

chinesedfan · 2018-02-14T07:58:52Z

src/components/repository-profile.component.js

        type="octicon"
        size={45}
        color={colors.greyLight}
      />

-      <Text style={styles.title}>{repository.name || ' '}</Text>
+      <Text style={styles.title}>{notFoundMsg || repository.name || ' '}</Text>


Similar here.

is that mean, you want make "Unkown" or "Not Found" as default, right?

My point is that the current "Not Found" is a message returned by Github API. Setting it as the value of Text directly will make the usage of this Text unclear. We'd better,

declare a corresponding string constant to represent special repository name, as I said before

or, create another certain UI component (maybe like a Toast) to show it

@chinesedfan if want to use Toast, i will install library react-native-simple-toast. What do you think? does it needed? Because toast from react native didn't support iOS

Sure, I don't mind to introduce a new library as long as it is useful. And we can also wait for some feedbacks from other reviewers, like @machour.

chinesedfan · 2018-02-14T08:16:11Z

src/api/index.js

-    return response.json();
+    return response.ok
+      ? response.json()
+      : response.json().then(err => Promise.reject(err));


In fact, response.json().then(err => Promise.reject(err)) is the same with response.json(). You handle the error in then and throw out identically. Please correct me if I misunderstood your thinking.

@chinesedfan
this because when the response status is 4xx it always handled by .then(), that's why i give return Promise.reject to make it handled by .catch(). But if you think there better way to distinct beetween 200 OK and 4xx Error, let me know.

Another suggestion from me is to implement Axios and Redux-Saga to the app for better handling failures.

@dhamanutd I see. Then you can modify getRepository to call v3.call, and check response.status. Please refer to checkReadMe.

instead of blank view, now will show not found message when error 404 BREAKING CHANGE: fix repository view when the repos 404 not found fix gitpoint#690

simpler ternary and change nested ternary to a function instead, delete props that not needed anymore.

repository.main.unknowLanguage and repository.main.notFoundRepo

give logical promise in the getRepository action reducer and add new state.repository.hasRepoExist

to make Toast also supported in iOS.

dhamanutd · 2018-02-15T07:07:04Z

@chinesedfan i did some of you requested to changes. you can review it now.

chinesedfan

@dhamanutd Thanks for your fast update! We are almost there.

chinesedfan · 2018-02-15T16:20:23Z

src/repository/repository.action.js

+          json = response.json().then(err => Promise.reject(err));
+        }
+
+        return json;


How about writing in one line, return response.status === 200 ? response.json() : Promise.reject()?

chinesedfan · 2018-02-15T16:24:09Z

src/repository/screens/repository.screen.js

@@ -39,6 +40,7 @@ const mapStateToProps = state => ({
  starred: state.repository.starred,
  forked: state.repository.forked,
  subscribed: state.repository.subscribed,
+  hasRepoExist: state.repository.hasRepoExist,


Shall we keep hasRepoExist and error together? Or either is enough?

Actually i want to remove error. But as you said before, you said maybe it will be used in future.

So, will me to delete that?

I prefer to keep error, because error is needed for Toast, while hasRepoExist can be replaced by !error. What do you think about?

@chinesedfan i think hasRepoExist is only to set state if repo exist or not, but error is to get value whatever it is not only about repo exist or not. I think both are used.

Yes, there are tricky differences between them. It requires us to handle these 2 fields more carefully.

chinesedfan · 2018-02-15T16:25:44Z

src/locale/languages/en.js

@@ -177,6 +177,8 @@ export const en = {
  },
  repository: {
    main: {
+      notFoundRepo: 'Repository is not found',
+      unknowLanguage: 'Unknown',


unknownLanguage as @arthurdenner suggested? Sorry I am not a native English speaker.

edit typo in translation simpler conditional to ternary, also edit fetchRepoInfo() promise to .finally() instead of .then()

chinesedfan · 2018-02-17T09:53:31Z

src/repository/repository.reducer.js

        error: action.payload,
-        isPendingRepository: false,


I just noticed that this line was removed. Do you want to keep pending state when an error occurs?

@chinesedfan it's because we take ...initialState that already had isPendingRepository state as false.

chinesedfan · 2018-02-17T09:56:52Z

src/repository/repository.reducer.js

@@ -62,19 +63,21 @@ export const repositoryReducer = (state = initialState, action = {}) => {
      return {
        ...state,
        issues: [],
+        error: '',


If you persist to keep 2 fields, I suggest to clear hasRepoExist when pending and error. If repo1 set it to be true, then repo2 will get wrong value during pending or error.

@chinesedfan in current changes, i've tried to test in real world. I don't face any problem when openning from one repo to another.

Yes, as you reset with initialState, the error state is cleared. And the pending state is too fast to be noticed.

Understand, okay i will set hasRepoExist when pending too

chinesedfan · 2018-02-17T09:59:49Z

src/repository/repository.action.js

+      .then(response => {
+        return response.status === 200
+          ? response.json()
+          : response.json().then(err => Promise.reject(err));


response.json() will never throw an error, no matter response.status === 200 or not, right? So I think Promise.reject() is enough. We don't need to call response.json() again.

@chinesedfan if we just only using Promise.reject(), we will not pass any value from that. It will just call .catch() but didn't pass any data you get from API response. In this case we will not get the "Not Found" messages from github API.

That's why first i need to get json() data from API, then i pass that data in the Promise.reject(err).

I make both if true (equal 200) or false (not equal 200) will send same json formatted data.

@dhamanutd Oh, hope I understand your idea this time. Do you mean response.json().then((json) => response.status === 200 ? json : Promise.reject(json))?

@chinesedfan yes, that's what i'm intended to

Good. Can you update it? Then I think there are no problems from my side. Thanks for your patient revisions!

@chinesedfan done

housseindjirdeh

This is great, thank you @dhamanutd :)

Just wanted to ask - how does the Toast show on Android? Can check it as well if we merge this in

housseindjirdeh · 2018-03-01T02:55:31Z

.all-contributorsrc

@@ -740,6 +740,15 @@
      "contributions": [
        "code"
      ]
+    },
+    {
+      "login": "dhamanutd",


🎉 🚀 🚀

chinesedfan requested changes Feb 14, 2018

View reviewed changes

dhamanutd added 3 commits February 14, 2018 18:59

fix(repository): fix repo view when repos return 404

03ed2ae

instead of blank view, now will show not found message when error 404 BREAKING CHANGE: fix repository view when the repos 404 not found fix gitpoint#690

refactor(repository): edit ternary and props

aa39189

simpler ternary and change nested ternary to a function instead, delete props that not needed anymore.

feat(locale): add new translation in repository.main section

440d091

repository.main.unknowLanguage and repository.main.notFoundRepo

dhamanutd force-pushed the repo-screen-problem-when-repo-404 branch from a7520f5 to 440d091 Compare February 14, 2018 12:28

dhamanutd added 2 commits February 15, 2018 13:51

refactor(repository): revert changes i made in api

71fb1f2

give logical promise in the getRepository action reducer and add new state.repository.hasRepoExist

feat(package.json): yarn add react-native-simple-toast

e0e5fc8

to make Toast also supported in iOS.

chinesedfan requested changes Feb 15, 2018

View reviewed changes

refactor(repository, locale): edit typo, if logic, promises

068d25e

edit typo in translation simpler conditional to ternary, also edit fetchRepoInfo() promise to .finally() instead of .then()

chinesedfan requested changes Feb 17, 2018

View reviewed changes

dhamanutd added 2 commits February 19, 2018 18:07

refactor(repository): add hasRepoExist in PENDING

3c67ad2

refactor(repository): prettier code, reset some state when pending

2e2d6a8

chinesedfan approved these changes Feb 20, 2018

View reviewed changes

Merge branch 'master' into repo-screen-problem-when-repo-404

7f23a51

housseindjirdeh approved these changes Mar 1, 2018

View reviewed changes

.all-contributorsrc

@@ -740,6 +740,15 @@

"contributions": [

"code"

]

},

{

"login": "dhamanutd",

Copy link

Member

housseindjirdeh Mar 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🚀 🚀

housseindjirdeh approved these changes Mar 1, 2018

View reviewed changes

housseindjirdeh merged commit 86dd2f7 into gitpoint:master Mar 1, 2018

machour mentioned this pull request Apr 22, 2018

refactor(repository-screen): switch to GraphQL #775

Merged

		underlayColor={colors.greyLight}
		disabled={(notFoundMsg && true) \|\| false}

fix(repository): fix repo view when repos return 404 #726

fix(repository): fix repo view when repos return 404 #726

Uh oh!

Conversation

dhamanutd commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Description

Uh oh!

coveralls commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinesedfan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhamanutd Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhamanutd Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhamanutd Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhamanutd commented Feb 15, 2018

Uh oh!

chinesedfan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhamanutd commented Feb 14, 2018 •

edited

Loading

coveralls commented Feb 14, 2018 •

edited

Loading

dhamanutd Feb 14, 2018 •

edited

Loading

dhamanutd Feb 15, 2018 •

edited

Loading

dhamanutd Feb 14, 2018 •

edited

Loading

dhamanutd Feb 18, 2018 •

edited

Loading