Make sure all reports are included when fetching all reports #600
Conversation
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
Looks good, but still not 100% sure why/if we need this
Also, one thing that is bugging me is that if we look at the code here we're setting the value of the report (which would now include chat reports):
https://github.com/Expensify/ReactNativeChat/blob/dc4ecb810d228c8c61467680051eba68e6ff425e/src/lib/actions/Report.js#L315-L318
But we've already done this for the chat reports when fetching them on their own here:
https://github.com/Expensify/ReactNativeChat/blob/dc4ecb810d228c8c61467680051eba68e6ff425e/src/lib/actions/Report.js#L150-L159
I haven't identified a problem with that per se, but since this is not happening synchronously anymore I wonder if we could get some weird data or missing reportName from this change? The order of operations is a little unclear. At the very least, we are setting report data in Ion twice whenever fetching. Again, not sure if this is a problem just creates "another thing to think about".
| // Merge the data into Ion | ||
| Ion.merge(`${IONKEYS.COLLECTION.REPORT}${report.reportID}`, newReport); | ||
| }); | ||
| return {reports: _.indexBy(fetchedReports, 'reportID')}; |
There was a problem hiding this comment.
I think maybe fetchedReports is already an object indexed by reportID.
| promiseAllSettled(reportFetchPromises) | ||
| .then((data) => { | ||
| fetchedReports = _.compact(_.map(data, (promiseResult) => { | ||
| fetchedReports = _.compact(_.reduce(data, (finalArray, promiseResult) => { |
There was a problem hiding this comment.
Probably don't need compact if we're changing this to reduce
That's a great point. I hadn't considered that these reports are being double-merged into Ion. Perhaps in this case then, it's better to do what I mentioned in your PR and to refactor |
|
OK, I'm just going to close this for now and look forward to the changes in your PR. |
Possible fix for #597 to ensure we have all the reports