Include linked policy name with report summary name

[arosiclair]

Problem
Per this Google Doc convo, some report pages including the ReportSettingsPage need the name of the linked policy, but this data is not included with report data provided by the API so it needs to be searched on the client.

Solution
Include linked policy name with report summary data and remove logic for looking up the policy name in ReportSettingsPage.

[iwiznia]

Include linked policy name with report summary data and remove logic for looking up the policy name in ReportSettingsPage.

Are we sure we want to do that? We would be sending the same data over and over again (once per report) and allegedly, any policy you have access to, you should have the summary loaded in newDot, so we could display the correct name by looking it up in the policy summary data.

[luacmartins]

I agree with @iwiznia. Can we just look up the policy name without sending a request to the server? We should have this data loaded in the client already!

[arosiclair]

Are we sure we want to do that? We would be sending the same data over and over again

I agree this would denormalize our data, but I guess one could argue having to lookup the policy name is "data massaging". I don't feel too strongly either way. @tgolen thoughts?

[tgolen]

one could argue having to lookup the policy name is "data massaging".

Exactly this, yes. It is the definition of data massaging which is against everything we've been working towards with this new API.

We would be sending the same data over and over again (once per report)

Is there a problem with this?

[luacmartins]

one could argue having to lookup the policy name is "data massaging".

Hmm I'm not suggesting doing this in the API call. We already have the data since we fetched the policies in OpenApp.

Maybe I'm missing something, but why wouldn't ReportSettingsPage connect to Onyx, e.g. policy_123 and get the policyName for that report's policy, e.g. this.props.policy[policyName]? That would prevent massaging and duplicating data, as well as data being out of sync.

[iwiznia]

Exactly this, yes. It is the definition of data massaging which is against everything we've been working towards with this new API.

Why would it be massaging the data? It is just reading it from a different source, not modifying it.

Is there a problem with this?

Not per se, but why would we return data we already know we have?

[tgolen]

Right now, this page requires two separate API calls to get all the data needed for display. That is not 1:1, that is 1:2 and I think that's wrong.

I would feel a little bit better if the Onyx connection was only connecting to the single policyID key that it needs to get the name, but currently, it's connecting to the entire collection of policies, and looping through all of them to find the right policy ID.

This sounds similar to what @luacmartins is proposing, but it still breaks the 1:1 rule.

[luacmartins]

This sounds similar to what @luacmartins is proposing, but it still breaks the 1:1 rule.

Not sure why it would break 1:1, since the policies are fetched on OpenApp and we don't need to trigger an additional API call for it.

I would feel a little bit better if the Onyx connection was only connecting to the single policyID key that it needs to get the name, but currently, it's connecting to the entire collection of policies, and looping through all of them to find the right policy ID.

We would need to connect to the entire collection, but we don't need to loop through all of it to find the right policy, we could just access it directly by this.props.policies[this.props.report.policyID]

[iwiznia]

Not sure why it would break 1:1, since the policies are fetched on OpenApp and we don't need to trigger an additional API call for it.

Exactly. There must be tons of places where we assume the data we loaded during OpenApp is there and load additional data for a specific page, no?

[luacmartins]

Yea, we will either fetch additional data like we do in the InitialSettingsPage (we fetch the user wallet balance, which is not returned in OpenApp) or we update it when switching chats for example.

[tgolen]

We would need to connect to the entire collection

Why do you need to connect to the entire collection? You should be able to connect to the collection's policy key directly, can't you?

key: (props) => `${ONYXKEYS.COLLECTION.POLICY}${props.report.policyID}`,

Regarding 1:1; If you are on the workspace settings page, and you refresh the page, you would count 2 API requests for one action, no? How is that not 1:2?

[luacmartins]

Why do you need to connect to the entire collection? You should be able to connect to the collection's policy key directly, can't you?

I don't think we get passed report as a prop. We seem to get the reportID from route.params.reportID and then connect to the report key. Would calling key: props => ... still work in that case?

Regarding 1:1; If you are on the workspace settings page, and you refresh the page, you would count 2 API requests for one action, no? How is that not 1:2?

Well that happens right now. Refresh your app with an open report and notice a call to both OpenApp and OpenReport. Not sure if this is something that we can avoid entirely, since the second call is triggered by the page you are on, so the action is more like refresh page and open the report I was on.

[tgolen]

then connect to the report key

Well, once this happens, the report is in the props that are passed to key: props => ...

Maybe we don't have logic built to handle this, and I honestly don't know what would happen, but it's worth trying (and supporting):

key: (props) => props.report ? `${ONYXKEYS.COLLECTION.POLICY}${props.report.policyID}` : null,

and if the key returns null, then we just ignore it.

[luacmartins]

Maybe we don't have logic built to handle this, and I honestly don't know what would happen, but it's worth trying (and supporting):

Yea, it doesn't seem like we support that yet. These are the props available in this call:

One thing that I noticed is that policy is already available, because we pass it from withFullPolicy and connecting to it again seems useless.

[arosiclair]

Maybe we don't have logic built to handle this, and I honestly don't know what would happen, but it's worth trying (and supporting)

I can give this a try.

EDIT: doesn't work unfortunately. I think report has to be passed down from the parent component

One thing that I noticed is that policy is already available, because we pass it from withFullPolicy and connecting to it again seems useless.

withFullPolicy is added incorrectly here. It connects the policy based on the policy ID in the route however there is none for this page so it doesn't work. We'll be removing it from this page with the GetFullPolicy refactor soon.

[arosiclair]

One solution could be to update withFullPolicy to also pass the policy based on the report ID in the route. This is more or less just moving this logic from one place to another, but at least it would be abstracted.

Otherwise we'd have to update our Auth command for report summaries to join on nameValuePairs to include the policy name.

[tgolen]

EDIT: doesn't work unfortunately. I think report has to be passed down from the parent component

I'd be interested to see what you tried, because I still think this would be possible to get it to work, but it would require a change to withOnyx().

we'd have to update our Auth command for report summaries to join on nameValuePairs to include the policy name.

I think this is really what we should be doing in the first place. This is the precedent and the best practice. Anything else is just a hack, and a complex one of that. If we keep trying to do this on the front-end, then that basically cements this rule in place:

To show the policy name that a report belongs to, it requires having a FULL list of policies updated on the client.

[iwiznia]

To show the policy name that a report belongs to, it requires having a FULL list of policies updated on the client.

This is not correct, it just requires the policy summaries, which are loaded on first load and always there.

[tgolen]

Well, I did not specify what policy objects I was referring to. But my concern still stands:

it requests having a FULL list of policy summaries updated on the client

[iwiznia]

Yes, which we always will have.

[arosiclair]

I've got around to drafting SQL query changes for GetChats command in Auth that should get us the policyName added to the response for reports. Something to the effect of this in query at the bottom

SELECT 
    ...
    JSON_EXTRACT(nvps.value, '$.name') policyName
FROM
    ...
    LEFT JOIN nameValuePairs nvps ON nvps.name = 'expensify_policy' || JSON_EXTRACT(rnvps.rNVPs, '$.expensify_policyID');

However, it sounds like we're divided on whether a join across datasets on the frontend (even a simple one like what we've talked about) qualifies as data massaging so I'm gonna start a discussion in #N7 on it to hopefully get some consensus.

[iwiznia]

I still think we should not be sending this data back and should get it from the already loaded policy summaries that we know will always be loaded

[arosiclair]

After our discussion on data massaging here it sounds like this specific issue isn't too big of a problem so I'll close this out. However, it would be nice to have some functionality in Onyx to clean up this behavior for joining keys so I created a follow up issue for that here.