Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add missing prop comments when merging global configs on upgrade #1004

Merged
merged 3 commits into from
Nov 15, 2021

Conversation

oshi97
Copy link
Contributor

@oshi97 oshi97 commented Nov 15, 2021

Fixes an issue where new prop comments would not be added to a user's
global config on upgrade. This issue occurred due to how comment-json
stores comments in CommentToken arrays. Each CommentToken[] is associated
with a symbol determined by the closest json property, such as Symbol.for('before:sessionTracking).
During a comment-json assign, CommentToken arrays associated with the same symbol
are not magically merged with no duplicates. Instead, the higher priority json will override the other.

This was fixed by re-adding in any missing comments.
No guarantees are made about the exact locations of re-added comments, to avoid
risk associated with a larger rework. I re-added them to the front for now, because
that works the best for the specific issue pointed out ("token" missing").

J=SLAP-1694
TEST=manual,auto

jambo upgrade, saw token added to my global config

Fixes an issue where new prop comments would not be added to a user's
global config on upgrade. This issue occurred due to how comment-json
stores comments in CommentToken arrays, under a symbol determined by the
closest json property, such as `Symbol.for('before:sessionTracking)`
When a comment-json assign is done on two CommentJSONValue-s that both
have comments under the same symbol, the json with higher priority will override
the other, as opposed to magically merging the arrays together with no duplicates.

This was fixed by re-adding in any missing comments.
No guarantees are made about the exact locations of re-added comments, to avoid
risk associated with a larger rework. I re-added them to the front for now, because
that works the best for the specific issue pointed out ("token" missing").

J=SLAP-1694
TEST=manual,auto

jambo upgrade, saw token added to my global config
@coveralls
Copy link

coveralls commented Nov 15, 2021

Coverage Status

Coverage remained the same at 9.19% when pulling e5aad38 on dev/token-on-upgrade into dd3db33 on release/v1.26.

@oshi97 oshi97 merged commit ed5f811 into release/v1.26 Nov 15, 2021
@oshi97 oshi97 deleted the dev/token-on-upgrade branch November 15, 2021 20:24
@nmanu1 nmanu1 mentioned this pull request Nov 16, 2021
nmanu1 added a commit that referenced this pull request Nov 16, 2021
## Version 1.26.0
### Features
- Add support for the auth token, with a new global_config `token` field  (#976)
- Add support for the visitor, which can be set or changed at runtime (#970)
- Add support for generic thumbs up/down feedback buttons to all cards (#973, #1005)
- Handle page and template names with spaces (#988)

### Changes
- Update styling of navigation ‘More’ menu (#989)
- Update hover color of search bar buttons (#1000)
- Internal repo changes (automate WCAG testing (#987))

### Bugfixes
- Fix console error which would appear on google maps (#955)
- Fix extra query when loading in iframe integrations (#986)
- Fix bug that displayed links that didn’t exist (#990)
- Allow microphone access for voice search in iframe (#991)
- Fix issue with custom search icon size (#992)
- Fix bug with new prop comments not being added (#1004)
- Fix distance alignment on location cards (#1006)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants