Settings: Uploading Publisher Logo deletes existing logos

[BrittanyIRL]

Something happened and now when you upload a new publisher logo it removes all other logos except for the default logo.

To replicate to go dashboard/settings and try to upload a new logo, you'll see other logos disappear. No delete requests are going through, it just seems like it's overriding?

Needs investigation.

[BrittanyIRL]

Digging into this a bit. Media upload is working as expected for single and bulk upload on dashboard settings.

When an upload occurs we're seeing the request for the media ids to become publisher logos, the ids are getting to the path to post to settings just fine:

/web-stories/v1/settings/?web_stories_publisher_logos=2992&web_stories_publisher_logos=3046&web_stories_publisher_logos=3041&web_stories_publisher_logos=3040&web_stories_publisher_logos=3042&web_stories_publisher_logos=3044&web_stories_publisher_logos=3045&web_stories_publisher_logos=3043

But then the actual request url only queries 1 of the ids within its params, like so: http://localhost:8899/wp-json/web-stories/v1/settings/?web_stories_publisher_logos=3047&_locale=user

It seems as though something is getting reduced in the api?

@spacedmonkey or @swissspidy did anything change recently that rings a bell about how this might be happening?

[swissspidy]

Not to my knowledge. But why doesn‘t this pass a comma separated list of IDs or use arrayweb_stories_publisher_logos[]= format? Did the way the URL is constructed change in the dashboard?

[BrittanyIRL]

the front end dashboard for this hasn't changed for 8 months 🙃 i'll dig a little more into it later tonight. it's something in the wpAdapter in the dashboard it seems since the query's right.

[swissspidy]

Where is the URL being stitched together then? Perhaps caused by an update of that query-string package?

[BrittanyIRL]

So we get to apiFetch in the wpAdapter of the dashboard and the query string is stitched together like so: "/web-stories/v1/settings/?web_stories_publisher_logos=10&web_stories_publisher_logos=50&web_stories_publisher_logos=46&web_stories_publisher_logos=48&web_stories_publisher_logos=47&web_stories_publisher_logos=49" for the path of the post which is normal and expected but then the actual query done as witnessed by the network tab is http://localhost:8899/wp-json/web-stories/v1/settings/?web_stories_publisher_logos=49&_locale=user so it's just recalling the last instance now. This would have to be occurring in the apiFetch.

From what I gather there's not been an update of the query string package that would cause this as their documentation says that they still support multiple instances of the same key: https://www.npmjs.com/package/query-string

It just really seems like the query is put together and then only the last instance is getting handled.

[swissspidy]

It just really seems like the query is put together and then only the last instance is getting handled.

Indeed somewhere in the apiFetch package this is happening. Although I didn't dig deep into the code to find out where this is happening.

I do have to say though that the /web-stories/v1/settings/?web_stories_publisher_logos=10&web_stories_publisher_logos=50&web_stories_publisher_logos=46&web_stories_publisher_logos=48&web_stories_publisher_logos=47&web_stories_publisher_logos=49 format is a little bit unexpected to me, but I know it's not uncommon.

My personal preference would have been a format like /web-stories/v1/settings/?web_stories_publisher_logos=10&web_stories_publisher_logos=50&web_stories_publisher_logos=46&web_stories_publisher_logos=48&web_stories_publisher_logos=47&web_stories_publisher_logos=49 or /web-stories/v1/settings/?web_stories_publisher_logos=10,50,46,48,47,49.

This works in my testing:

        if (publisherLogoIds) {
          query.web_stories_publisher_logos = [
-            ...new Set([...state.publisherLogoIds, ...publisherLogoIds]),
+            [...new Set([...state.publisherLogoIds, ...publisherLogoIds])].join(
+              ','
+            ),
          ];
        }

Since the Upload logo button is of input[type=file], it should be possible to create an e2e test for uploading publisher logos. Let's do that to prevent regressions.

[BrittanyIRL]

This works in my testing:

It sure is, ugh. I was looking at the issue from the query and didn't think about just changing it so that it wouldn't be an issue of multiple instances of the same key. PR is up now. Thanks for the help, @swissspidy

Ticket for e2e test: https://github.com/google/web-stories-wp/issues/7224

[csossi]

Verified in QA