Me: Decode entities for /me form inputs.

[oskosk]

Decode the HTML entities present in user settings REST API response.

See #625 and #625 concerns comment by @ebinnion.

The API takes care of encoding characters but React components are re-encoding those characters on render.

Regarding previously mentioned concerns, it seems the API does a good job filtering HTML tags.

Steps to currently recreate the issue fixed here:

1. Go to your profile.
2. Set a display name, description or Web Address with an ampersand character.
3. Reload the page (full browser reload).
4. Look at the display name input having the &amp there.

It seems there's also an issue after updating the description field and saving.

Before:

After:

[oskosk]

cc @ebinnion

[ebinnion]

Functionally, this seems to work well. But, I did notice that we are getting the following warnings:

Also, the more I think about it, we may not want/need to decode all fields. So, perhaps we should come up with a whitelist of fields? To start with, we could do all of the profile fields on the /me route.

[oskosk]

Great observations. I'll come back to this.

With this PR, I just had the intention of this topic being present here as it may have got lost on #625 comments given it's a closed PR now and had no related issue.

[nb]

Should the API be returning strings with HTML entities?

[oskosk]

FYI, these issues submitted by @v18 are related:

#1008 (Me: About Me should not escape an ampersand)
#977 (Me: About Me section strips html code, but Gravatar does not)

[oskosk]

@nb, by changing this behaviour on the API to it offering raw content, I can imagine all other usages of the API outside Calypso that are not aware of the change, would break (or behave unexpectedly).

In this case, encoded content is an issue because React is doing the re-encoding but I suppose that's not the case for other existing consumers relying on that content being encoded and not using React as renderer.

It may also be the case that the DB has the encoded content (when saved) and it's not just the API doing it on the fly. That, I can't be sure of.

Given that this behaviour is a known documented fact as you mentioned before, I think the solution would have to be handled by Calypso itself.

I'll try to come up with an alternative. (At least one, that doesn't show warnings on the console regarding null/empty values passed to formatting.decodeEntities() 😝 )

BTW, @ebinnion clarified me that his comments about maybe not wanting all field values automatically decoded was about boolean values for checkbox/radios, so I'll try to cope with that situations.

[nb]

@oskosk, OK, not changing the API makes sense in terms of backwards compatibility standpoint. Still, what would be the most general place we could fix this problem without breaking anything?

[oskosk]

@nb IMHO, the most general approach would be to recommend everyone doing a wpcom request, to do the decoding as soon as the API request returns, as React is prepared for handling unencoded content and the API is returning encoded content [1].

It seems other issues like this are being reported.

• People: Decode HTML entities for user inputted information #1086
• Me: About Me section strips html code, but Gravatar does not  #977
• Me: About Me should not escape an ampersand #1008

and it seems these other too assuming the messages come from the API.

• Purchases: HTML in error message #1046
• Signup: Users marked as spam see HTML in the error message #471

In the case of the profile, I think this would be the right place to decode every profile property:

https://github.com/Automattic/wp-calypso/blob/master/client/lib/user-settings/index.js#L63

A super-mega-general approach could be doing it at lib/wpcom level. But I can't assert yet if that wouldn't break stuff.

[1] I've updated this PR with a proposal for this alternative.

[ebinnion]

My biggest concern with this method is that it mutates the original data returned from the API. While I don't think it's explicitly called out as a no-no in the docs, it is something we should avoid. See - http://wpcalypso.wordpress.com/devdocs/docs/coding-guidelines/javascript.md#default-parameters

On another note, I think you could save yourself some work and make the code more readable by using a higher-order function such as mapValues. See - https://lodash.com/docs#mapValues

[oskosk]

Thanks @ebinnion. I understand that traverseAndDecodeEntity function mutates data. I declared that on its docs for it to be reliable. I also took in account that a straightforward alternative like

this.settings = data;
this.settings.display_name = decodeEntities ( data.display_name );
this.settings.user_URL = decodeEntities ( data.user_URL );
this.settings.description = decodeEntities ( data.description );

would be also mutating data.

IMHO, this case of a function mutating its argument is particular because this is an anonymous function declared and defined specifically for being passed control by the API request method. Thus it offers no interface and no chance for secondary effects because no others usages of this function are possible. Also, take in account this is the beginning of this.settings life cycle.

It seems to me, that unless a full mapValues() is done on the whole object, there's no alternative to mutating data.

I think that in order for this fix to go along I'll drop that traverseAndDecodeEntities and just do this:

this.settings = mapValues( data );
this.settings.display_name = data.display_name && decodeEntities ( data.display_name );
this.settings.user_URL = data.user_URL && decodeEntities ( data.user_URL );
this.settings.description =data.description && decodeEntities ( data.description );

If needed, a more general approach or utility function can be added as an independent PR. What do you think?

[ebinnion]

In this usage, I agree that there's likely not a downside to mutating data. But, you're also attempting to add a method to lib/formatting which is used throughout the project, so it is likely that the method would not be used in the same manner.

You could do something like the following which doesn't mutate data and doesn't require you to map the entire object.

let decodedValues = {
    display_name: decodeEntities( data.display_name )
    user_URL: decodeEntities( data.user_URL )
    description: decodeEntities( data.description )
};

this.settings = Object.assign( {}, data, decodedValues );

[oskosk]

That's way more efficient and readable. Thanks

[ebinnion]

One last point I'd like to make is that if you intend to add a method to lib/formatting, you should plan on writing tests for that. See - https://github.com/Automattic/wp-calypso/blob/master/shared/lib/formatting/test/test.js

[oskosk]

It seems there's also an issue after updating the description field and saving. I'll update the description

[ebinnion]

The reason why there is a decoding issue after updating settings is likely due to this line: https://github.com/Automattic/wp-calypso/blob/master/client/lib/user-settings/index.js#L89

So, what I would suggest is to factor out the decoding functionality above into a function, and then run that for fetching and saving settings.

[oskosk]

Nice!

[ebinnion]

There should be a blank newline above this comment.