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

use single quotes for attributes that contain JSON data #11

Closed
wants to merge 1 commit into from

Conversation

ETLaurent
Copy link
Contributor

Summary

Summarize the changes briefly, including which issue/ticket this resolves. If it closes an existing Github issue, include "Closes #[issue number]"

What are the specific steps to test this change?

For example:

  1. Run the website and log in as an admin
  2. Open a piece manager modal and select several pieces
  3. Click the "Archive" button on the top left of the manager and confirm that it should proceed
  4. Check that all pieces have been archived properly

What kind of change does this PR introduce?

(Check at least one)

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

Make sure the PR fulfills these requirements:

  • It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
  • The changelog is updated
  • Related documentation has been updated
  • Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

@linear
Copy link

linear bot commented Feb 13, 2023

PRO-3664 Absolution returning improper HTML

After A3 upgrade, we experienced a lot of issues when rendering the CMS blog page through a Career Site. We narrowed down the issue to the Apostrophe's Absolution utility (https://github.com/apostrophecms/absolution) and noticed that the Absolution utility is returning improper HTML where it is using double quotes to surround an attribute whose value is JSON (e.g., data-apos) when it should have left the attribute's value surrounded by single quotes.

For full context using BMS in staging environment as an example, we are performing the following in our RMS Node layer for Career Sites:

  1. Make a request to CMS to fetch the HTML content. In the HTML tag with blog-wrapper class, notice that the data-apos attribute is surrounded by single quotes. HTML is valid.

    curl --location --request GET 'https://bms.staging.cms.talentplatform.us/blog'

  2. Update links in HTML replacing CMS URLs with RMS URLs using normal JS string replacement. HTML content is still valid at this point.

  3. Update the HTML to make relative paths absolute via Absolution utility. The returned HTML from absolution becomes improper because the data-apos attribute value gets wrapped by double quotes when it should have been left with single quotes.

  4. When the Career Site tries to serve the CMS blog page, a parsing error is thrown in the console (see screenshot parsing_error.png). If you inspect the HTML, you'll notice that data-apos attribute is not set properly because of the double quotes. Basically, data-apos is set to "{" and everything after that is ignored (see bad-data-apos-value.png). A parsing error happens in the "public-module-bundle.js" where it's trying to set the "window.apos" with "data-apos". This causes a lot features on the blog page to become broken due to "window.apos" not being setup properly.

    See staging BMS blog page via Career Site: https://bms.staging.jibeapply.com/blog

@@ -30,9 +30,10 @@ var absolution = module.exports = function(input, base, options) {
_.forEach(attribs, function(value, a) {
result += ' ' + a;
if (value.length) {
const quote = value.startsWith('{"') ? `'` : `"`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

input: `<div data-test1='{"foo":"bar"}'>Test</div>`
attribs: { 'data-test': '{"foo":"bar"}' }
value: `{"foo":"bar"}`

I only found the startWith('{"') solution, but there is probably a better way, only I can't find it...

@ETLaurent
Copy link
Contributor Author

See #12

@ETLaurent ETLaurent closed this Feb 13, 2023
@ETLaurent ETLaurent deleted the pro-3664-fix-quotes branch February 13, 2023 16:01
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

1 participant