fix(issue): Make sure the right repository is fetched

[chinesedfan]

Fixed #315 and #318.

In fact, #318 is fixed partially by #317, as getContributorsByDispatch is not called correctly. But the main problem is that the right repository is not always fetched.

If visit issue.screen from:

• notifications, then issueURLParam has value, while issueParam is undefined (Wrong issue/pr permissions when visit from notifications tab #315)
• home events, then issueParam has value, while issueURLParam is undefined
  • in addition, for special home events, i.e. close pr, issueParam.repository_url is undefined (Incorrect contributors when trying to @ in issues #318)

So I use props.issue.repository_url to check, which belongs to the fetched issue.

[lex111]

@chinesedfan can as earlier receive variable issue (144 line)? Then you just do not need this line.

[chinesedfan]

@lex111 No, we should read after those 2 promises, getIssueFromUrlByDispatch and getIssueCommentsByDispatch, are fulfilled. Or this.props.issue may refer to the previous wrong issue.

That's what I mean "the fetched issue". Sorry for my poor English pr description.

[housseindjirdeh]

Nope that shouldn't be the case :)

In the top of the file, we're mapping parts of our state object into our props. So issue isn't a fixed object parameter here, but will always reflect what's happening with state. If getIssueFromUrlByDispatch and/or getIssueCommentsByDispatch update state.issue where state is our Redux container - our issue props will always reflect this. So we should be good with leaving it as part of the destructuring in line 144 😃

Please let me know to clarify this if it's still not making any sense to you mate

[chinesedfan]

Right. this.props.issue will always be updated by redux, but in line 144, issue is just a temporary local variable destructed from this.props, which will never be updated.

[housseindjirdeh]

Oh I'm sorry, you're right here. It gets modified in the promise call and our current local variable isn't really going to reflect this.

Cheers thank you mate, and I'm sorry for causing any confusion 😞

[housseindjirdeh]

Thanks a million for this and appreciate you catching exactly what was causing the error @chinesedfan 🎉

Left one comment per @lex111's suggestion, otherwise this is solid :)

[housseindjirdeh]

Nope that shouldn't be the case :)

In the top of the file, we're mapping parts of our state object into our props. So issue isn't a fixed object parameter here, but will always reflect what's happening with state. If getIssueFromUrlByDispatch and/or getIssueCommentsByDispatch update state.issue where state is our Redux container - our issue props will always reflect this. So we should be good with leaving it as part of the destructuring in line 144 😃

Please let me know to clarify this if it's still not making any sense to you mate

[housseindjirdeh]

So this was the culprit. It looks like issueParam, unlike props.issue, is actually obtained from the navigation.params so it's not being updated when those calls within Promise.all are fired

[chinesedfan]

Suppose we have visited repository A, and try to visit another issue/pr of repository B. props.repository will refer to A at first. issueParam.repository_url will refer to B.

The basic logic is correct here. But issueParam.repository_url does not always have value, which may be caused by issueParam itself or issueParam.repository_url as my PR description says.

[chinesedfan]

@housseindjirdeh Any feedbacks? :)

[housseindjirdeh]

Thank you so much @chinesedfan, and apologies for not getting to this sooner!