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

NickAkhmetov/Fix publications page crashing when supporting json data is completely missing #3673

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

NickAkhmetov
Copy link
Collaborator

Summary

This PR adjusts the condition for whether to display the visualizations section of the publication page so that the .length property is not accidentally accessed from a missing/undefined vignettes list.

Design Documentation/Original Tickets

Commit 5e5f263 introduced this issue while attempting to fix handling of empty vignette lists in present json files.

Testing

Manually tested locally with all publications on dev and test environments to confirm no crash occurs.

Screenshots/Video

N/A

Checklist

  • Code follows the project's coding standards
    • Lint checks pass locally
    • New CHANGELOG-your-feature-name-here.md is present in the root directory, describing the change(s) in full sentences.
  • Unit tests covering the new feature have been added
  • All existing tests pass
  • Any relevant documentation in JIRA/Confluence has been updated to reflect the new feature
  • Any new functionalities have appropriate analytics functionalities added

Additional Notes

Please specify any additional information or context relevant to this PR.

@@ -30,7 +30,7 @@ function Publication({ publication, vignette_json }) {
const shouldDisplaySection = {
summary: true,
data: true,
visualizations: vignette_json?.vignettes.length > 0 && Boolean(Object.keys(vignette_json).length),
visualizations: vignette_json?.vignettes?.length > 0 && Boolean(Object.keys(vignette_json).length),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
visualizations: vignette_json?.vignettes?.length > 0 && Boolean(Object.keys(vignette_json).length),
visualizations: vignette_json?.vignettes?.length > 0,

Do we need the second condition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double check with this change in place to make sure there are no unexpected regressions

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, looks good/seems to work as expected with the second clause removed 👍🏻

Copy link
Collaborator

@john-conroy john-conroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@NickAkhmetov NickAkhmetov merged commit 039f86b into main Jan 23, 2025
8 checks passed
@NickAkhmetov NickAkhmetov deleted the nickakhmetov/fix-publications-regression branch January 23, 2025 16:20
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.

2 participants