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

SEO: Title format editor won't save "remove custom formatting" #10003

Closed
oandregal opened this issue Dec 13, 2016 · 11 comments · Fixed by Automattic/jetpack#7044
Closed

SEO: Title format editor won't save "remove custom formatting" #10003

oandregal opened this issue Dec 13, 2016 · 11 comments · Fixed by Automattic/jetpack#7044
Assignees
Labels
[Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Feature] Site Settings All other general site settings. [Type] Bug
Milestone

Comments

@oandregal
Copy link
Contributor

Steps to reproduce as per #user-report

  • I clicked Settings then chose the SEO tab and entered Site Title + Tag Line + Post Title.
  • Then I saved and I checked the result.
  • I decided to remove these settings so I clicked on the Site Title, Tag Line and Post Title and they disappeared and I Saved.
  • But when I go back to SEO again later the old settings are still there. I can't remove them.

The user also mentioned this workaround: "I removed the tags and then entered 2 spaces and saved. It seems to save the 2 spaces.".

I could reproduce the issue and it happens for all sections (front page, posts, etc).

@oandregal oandregal added [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Feature] Site Settings All other general site settings. [Type] Bug labels Dec 13, 2016
@oandregal oandregal added this to the Advanced SEO milestone Dec 13, 2016
@oandregal
Copy link
Contributor Author

refs 8620-chat

@dmsnell
Copy link
Contributor

dmsnell commented Dec 13, 2016

Thanks for reporting this @nosolosw - I suspect that I know exactly what's happening… the absence of settings updates are considered as no-requests to change the data. If the field is blank, it probably thinks we're not sending any changes.

@vindl did we ever settle on a way to "reset to default" and clear the fields?

@dmsnell dmsnell self-assigned this Dec 13, 2016
@vindl
Copy link
Member

vindl commented Dec 13, 2016

@dmsnell The current way to reset to default is to set a corresponding title format to empty string.

@roundhill roundhill assigned roundhill and unassigned dmsnell Jan 12, 2017
@roundhill
Copy link
Contributor

I dug into this a bit, and I think there's a few things going wrong here:

  • It doesn't appear that the advanced_seo_title_formats data is getting sent to the API at all when we send an empty array like: advanced_seo_title_formats.front_page = []
  • Even if I set it to an empty string, the api returns Invalid SEO title format.

@vindl maybe you can take a look?

@vindl
Copy link
Member

vindl commented Jan 13, 2017

@dan I now noticed that my previous comment was not entirely true. In order to reset the particular title to default value, we should be passing an empty array. For example:

{
    "advanced_seo_title_formats": {
        "pages": []
    }
}

Sending advanced_seo_title_formats.front_page = [] will have no effect, since only those format types that are explicitly passed with request will be considered for update (like pages in the example above). We did this on purpose in order to avoid having to replay existing/unchanged values of other format types with every request. Does this help?

@roundhill
Copy link
Contributor

Does this help?

Yes, thanks! I need to figure out why the empty array that we set in Calypso never makes it to the api call.

@dmsnell dmsnell changed the title SEO Settings not SEO: Title format editor won't save the change "remove all custom formatting" Jan 13, 2017
@dmsnell dmsnell changed the title SEO: Title format editor won't save the change "remove all custom formatting" SEO: Title format editor won't save "remove custom formatting" Jan 13, 2017
@roundhill
Copy link
Contributor

I've traced this all the way down in the request chain to where we send the postMessage to the wpcom iframe. It appears to be stripping out the empty array when the title field is empty:

screen shot 2017-01-13 at 1 18 14 pm

Notice how the advanced_seo_title_formats object isn't there at all! I wonder if the array is being collapsed or something?

I guess a workaround could be to send an empty string instead of the empty array, and adjust the API to account for that.

@dmsnell @vindl

@roundhill
Copy link
Contributor

@vindl I forgot about this issue. Could we adjust the API to check for an empty string and then just save an empty array?

@vindl
Copy link
Member

vindl commented Apr 20, 2017

@roundhill I'll have to investigate that. It could turn out to be tricky because that change would also affect Jetpack sites, and the UI could stop working on previous Jetpack versions (because of format validation in API). There is also a risk of introducing bugs on sites that have already saved option values in existing format.
It would be best if we could limit the changes required to fix this to Calypso only.

vindl added a commit to Automattic/jetpack that referenced this issue Apr 24, 2017
Previously, only arrays were considered as valid title formats, and
in case of deletion we needed to provide empty array. Now, we are also
allowing empty string to be passed in that case instead. This is needed
to resolve the bug that prevented title deletion in Calypso.

GitHub issue: Automattic/wp-calypso#10003
@vindl
Copy link
Member

vindl commented Apr 24, 2017

@roundhill I created an update for API side that will allow empty strings to be accepted, and modified the Calypso side to send them instead of empty arrays. That fixed the issue for me.

jeherve pushed a commit to Automattic/jetpack that referenced this issue Apr 24, 2017
* SEO Tools: Allow empty string to be passed as title format

Previously, only arrays were considered as valid title formats, and
in case of deletion we needed to provide empty array. Now, we are also
allowing empty string to be passed in that case instead. This is needed
to resolve the bug that prevented title deletion in Calypso.

GitHub issue: Automattic/wp-calypso#10003

* yoda conditional
@vindl
Copy link
Member

vindl commented Apr 25, 2017

Still requires #13327 and D5298-code to be deployed, in addition to Automattic/jetpack#7044.

@vindl vindl reopened this Apr 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Feature] Site Settings All other general site settings. [Type] Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants