Fix forEach error
#10194
Merged
Fix forEach error
#10194
+227
−155
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…mary without properties to reproduce error.
|
Build files for 93b4390 have been deleted. |
|
Size Change: +121 B (+0.01%) Total Size: 2 MB
ℹ️ View Unchanged
|
aaemnnosttv
commented
Feb 7, 2025
| @@ -73,7 +70,7 @@ const fetchGetAccountSummariesStore = createFetchStore( { | |||
| ...state, | |||
| accountSummaries: [ | |||
| ...( state.accountSummaries || [] ), | |||
| ...( response.accountSummaries || [] ), | |||
| ...populateAccountSummaries( response.accountSummaries || [] ), | |||
There was a problem hiding this comment.
This fixes a potential bug where findMatchedProperty could be invoked after account summaries had been received, but before they had been transformed/populated with their IDs which happened in an action once all AS's had been received. This resulted in all propertyIDs being undefined and obviously caused problems with proper matching. That wasn't really necessary, as only the sorting needs to be done at the end.
| } ); | ||
| } ); | ||
| summaries.forEach( | ||
| ( { _id: accountID, propertySummaries = [] } ) => { |
There was a problem hiding this comment.
This fallback to the empty array was the main initial fix as the key isn't always defined in the accountSummary if there are no properties. This is no longer the case however as populateAccountSummaries will ensure every AS has propertySummaries defined.
| @@ -118,119 +122,176 @@ describe( 'getAccountDefaults', () => { | |||
| ).toHaveProperty( ENHANCED_MEASUREMENT_ENABLED, true ); | |||
| } ); | |||
| } ); | |||
| } ); | |||
There was a problem hiding this comment.
Previously the added populate* describe blocks were nested under getAccountDefaults. This fixes the grouping as well. Toggle "hide whitespace" (?w=1) to make reviewing this part easier.
aaemnnosttv
commented
Feb 10, 2025
| { _id: '1122334455' }, | ||
| { _id: '1122334456' }, | ||
| { _id: '1122334457' }, | ||
| { property: 'properties/1122334455' }, |
There was a problem hiding this comment.
This is necessary because of the populating of ID that we do in the reducer callback for receiveGetAccountSummaries which will otherwise overwrite the pre-populated ID field here with undefined since the source property is missing. This was the only instance where this was needed so I opted to update the test instead of the populate utilities. It might be a good idea to preserve an existing/pre-populated ID in the future.
tofumatt
approved these changes
Feb 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Addresses issue:
Important
This PR now targets develop
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist