Fix 2246: Setting or changing a products perma-link causes hard refresh

[prinzdezibel]

Resolves #2246
Impact: minor
Type: bugfix

Issue

Changing or creating the permalink of a product did trigger a page reload. Also this action was not reactive, meaning that other connected clients did not receive the new URL after publishing.

Solution/Changes

• Don't trigger route change from callback value of meteor call revisions/publish, because

1. it's changing server state that gets delivered to connected clients asynchronously. It's not possible to rely on the publications already being synced to the client side when the method returns. This led to race conditions when trying to change the route client side, therefore it was done the hard way by setting window.location.href. Tried to observe permalink changes client side, but that was unreliable as well, probably because tracker runs may re-instantiate observed cursors at any time.

2. it's not reactive (other connected clients won't get noticed about the route change), when the permalink has changed

-> INSTEAD we observe changes to handles server side and let the client know it has changed through a new property changedHandleWas, which will be published like any other property (gets persisted to db, but if this would be a problem we could eliminate that through usage of Products.observe instead of Products.observeChanges)

• Don't use window.location.href for route changes any more. Router.go() is used instead.

• Changes to the Product publication which seems wrong regarding updating product.__revision
  Don't publish the whole product object to the client. The code changed is only about the property __revisions on the product collection. Therefore it's wrong to push the whole object to the client. If done anyway, it will push product data to the client (e.g. handle, title) that is going to be updated soon after due to meteor method revision/publish, which results in outdated data on the client (which in turn results in template "not found" being displayed)

• Also I think it's wrong to do database queries to decide what should be pushed to the client, e.g.

  const revisions = Revisions.find({	
        "documentId": id,	
         "workflow.status": {	
            $nin: [	
                "revision/published"	
            ]	
          }	
        }).fetch();	
	
     fields.__revisions = revisions;	
     this.changed("Products", id, fields);

doesn't seem correct. To know what needs to be synced with the client, I think the source of truth needs to be the merge box (session collection view). Using db queries will not work reliably, as it seems.

Breaking changes

none expected

Testing

1. As an admin create a new product
2. Change permalink of the product
3. Publish changes
4. See updates to content and URL without page refresh
5. URL for connected clients should update as well

[prinzdezibel]

I believe because server side publish functions are not reactive, the observer will not push changes to the client, if Products.findOne(preSelector) changes. I've tried to get rid of that cascading database query, but realized that it's not possible to ditch query cascade completely. It's still needed for peeking into product to get associated shop. This is important for permission checks. Instead I've relaxed the second selector from

selector.$or = [
    { _id },
    { ancestors: _id }];

to

selector.$or = [
    { _id },
    { ancestors: _id },
    { handle: productIdOrHandle } ];

so that changes in subscription parameter productIdOrHandle get observed and pushed to client.

[prinzdezibel]

I have overhauled the Product and Products publications significantly. I think they are very buggy in its current state. I think it's not possible to reliably solve the task at hand (the client side redirect when the permalink changes) without overhauling the Product and Products publication.

These are the main culprits we need to look out for when testing this change (as far as I know)

• Revision control (incl. rollbacks)
• I know about the products documents that go to the Revision collection. But other documents like images can be revisioned too. I don't know how these non-product revisions are treated. For instance, I've tested the undo feature. The product document got rolled-back to its older version, which looked ok to me. Interestingly, related product images were not rolled-back. But I've observed the same behaviour in master.
• Adding new products from PDP or Product grid should always work properly. All intermediate values (until revision is published) should be shown correctly in admin-view.
• Changing the permalink handle should work (albeit the field get sometimes reset the first time, see below)
• In-depth test of marketplace feature.

Open issues (happen on master as well)

• The first time the handle (permalink) is changed, the field value gets reset. This happens only on first edit.
• When changing title AND permahandle, a server side error may occur
  Exception while invoking method 'products/updateProductField' RangeError: Maximum call stack size exceeded

[brent-hoover]

@prinzdezibel I feel like overhauling the two main product publications is way out-of-scope for this ticket, before you do any more work in that avenue let's talk.

[prinzdezibel]

@zenweasel Not planning any more work on that, as it looks good to me. But would be great if somebody could have a look into it. Would be a sad is we need to ditch that. I think it's an improvement, really (assuming I have not overseen something important - which I can't rule out of course).

[brent-hoover]

@prinzdezibel Looks like you have conflicts here

[jshimko]

@prinzdezibel There's a lot going on here with a bunch of things I'm not very familiar with. Can you please resolve this merge conflict since you're more familiar with these proposed code changes? Once you've done that, I'll confirm this is all doing what it's supposed to.

[prinzdezibel]

@jshimko I think this is ready for a review from you. After it has passed this first stage, I'll also assign Spencer as second reviewer. I've seen some errors, but these are happening on master as well (listed in open issues above). Therefore I would propose to ignore them until this is approved, because maybe even more things come up during testing. I'm going to file all those issues later as new, separate issues.

For testing, you'd need to pull in two other PRs from Brent:

• (fix: 3846) Set localstorage even when no Meteor.user exists #3856
• (fix 3851) Properly record product revisions #3857

[prinzdezibel]

@jshimko Tests are passing. Spencer was suggesting that you reviewing it should be enough. The reason I wanted him as second peer reviewer was because Aaron put him on the reviewer's list a couple of days ago. Anyway, we can always ask another person to look into it, if we think it helps..

[aaronjudd]

@prinzdezibel can you review and fix up the conflicts one more time for 1.9.0? 👍 @spencern we've discussed but since the catalog updates are pending, I think this should be reasonable to merge - we can pair on a review if necessary for 1.9...