(fix): set slugified title as permalink #5264

mpaktiti · 2019-07-03T16:32:48Z

Resolves #4735
Impact: major
Type: bugfix

Issue

When you create a product, the permalink (handle) is set to a random ID instead of the slugified version of the title. This is happening because:

the code initially sets the handle to a random ID (in the meanwhile, the title is null)
when the title is set the handle is not updated because the condition doc.handle === doc._id is never true (the handle is set to a random ID, not the doc ID)

Steps to reproduce:

Create a new product
Observe that the handle is not a slugified version of the title
Update the title -> the handle is not updated

Solution

I removed the doc.handle === doc._id condition. This way when the user sets a Title for the product, the handle is updated. I also removed the initial population with a random ID because I see no use for it.

Breaking changes

None

Testing

Log in the Operator UI and create a product
Both the Title and the Handle are null
Set a Title for the product and tab to another field
Notice how the Handle is updated to the title's slugified version
Keep changing the Title and notice how the Handle keeps getting updated each time

zenweasel · 2019-07-04T02:43:35Z

@mpaktiti I think setting the handle temporarily is to handle the case where you would navigate out of the PDP and then click back again and the product you were editing would disappear because products are only accessible by handle so this newly created product would never be accessible. Can you double check that you haven't introduced a new bug around that scenario?

mpaktiti · 2019-07-04T12:45:01Z

@zenweasel Steps followed:

Go to Products
Click Create product
Fill no info (both title and handle are null)
Click on Orders
Click back on Products -> I can see the product I created earlier (2nd row in the screenshot below)

Let me know if I didn't get the steps right.

zenweasel · 2019-07-04T20:32:55Z

Are you able to click back into that product? If so I think we're OK

mpaktiti · 2019-07-05T13:02:31Z

@zenweasel Yes I can step back and edit it. The URL uses the product ID. For example, for an untitled product I created the URL is http://localhost:3000/operator/products/eMnJA25oWMGYP55zn and I can see in Mongo that eMnJA25oWMGYP55zn is the product's _id.

zenweasel · 2019-07-08T01:49:00Z

@mpaktiti You need to sign your commits when committing to the open source projects. Click on "details" next to the DCO check on how to rectify

zenweasel · 2019-07-08T05:19:42Z

@manueldelreal can you give this a quick test please?

Signed-off-by: Maria Paktiti <maria@reactioncommerce.com>

mikemurray · 2019-07-08T17:56:46Z

imports/plugins/core/catalog/server/methods/catalog.js

@@ -816,7 +814,7 @@ Meteor.methods({
        // Should be a call similar to the line below.
        [field]: createHandle(Reaction.getSlug(value), _id) // handle should be unique
      };
-    } else if (field === "title" && doc.handle === doc._id) {


The purpose of this is to ensure that permalink isn't updated every time someone changes a title. With the removal of the ID being the default permalink, this is definitely less useful; however, there should be new check added for the existence of the permalink and block updates if it's set.

The permalink changing every time the title changes will cause broken links. If someone wants to break their permalink, they can do so from the field itself, and make whatever redirects they want in their storefront if necessary.

What I'd like to see from this is:

The permalink should be set the first time automatically based on the title.

The permalink doesn't update automatically with the title change if it's already set.

The permalink is manually updateable through the field, which it is now.

Hey @mikemurray , thanks for the feedback. One clarification about the first bullet point.

When we create a product the title is null. I'll have the permalink updated automatically only the first time the title is set by the user. Until then it will be null (and after the first update it will not be updated automatically again). Sounds good or should we populate it initially with a default value?

The permalink could be blank until the first update of the title. Products can't be published with a null title. Not sure about a null permalink, but the null title will make sure the permalink it set.

is this thread resolved? asking for a friend that needs to test this.

@manueldelreal No it's not! I'll ping you once it's ready for testing.

I changed this locally and this is the behavior I see:

Go to Products and create a new product. The title is not set yet and therefore the permalink is empty as well.

Set a value for title -> permalink is updated automatically

Update the title -> permalink is not updated automatically

Update the permalink manually -> update successful

However, if I delete the permalink manually and try to Publish, the system lets me do it. Should I add the same validation we have for Title and not allow them to publish if the permalink is null?
@mikemurray @zenweasel

Actually the title validation is not on Publish, it doesn't let you delete it to start with:

Add the same for permalink?

I think it's fine to make the permalink required since it no-longer defaults to the product id.

@mikemurray Can you please review the changes I just pushed? Behaviour is the same as I described in my previous comment, plus if you try to delete the permalink you get this error:

manueldelreal · 2019-07-09T18:52:13Z

@zenweasel @mpaktiti waiting on the conversation thread between @mikemurray and @mpaktiti to be resolved before testing this one with its final form.

cc @zenweasel

Signed-off-by: Maria Paktiti <maria.paktiti@gmail.com>

mikemurray

LGTM 👍

manueldelreal

Signed-off-by: Mike Murray <hellomikemurray@gmail.com>

mpaktiti requested a review from mikemurray July 3, 2019 16:32

zenweasel requested a review from manueldelreal July 4, 2019 02:35

(fix): set slugified title as permalink

0419c60

Signed-off-by: Maria Paktiti <maria@reactioncommerce.com>

mpaktiti force-pushed the fix-4735-mpaktiti-catalog branch from c0319fb to 0419c60 Compare July 8, 2019 10:53

mikemurray requested changes Jul 8, 2019

View reviewed changes

mpaktiti and others added 4 commits July 18, 2019 11:45

(fix) make permalink required

fd60180

Signed-off-by: Maria Paktiti <maria.paktiti@gmail.com>

(fix) auto-update permalink only if null

7040dda

Signed-off-by: Maria Paktiti <maria.paktiti@gmail.com>

(fix) set permalink label for required handle

f65aca8

Signed-off-by: Maria Paktiti <maria.paktiti@gmail.com>

Merge branch 'develop' into fix-4735-mpaktiti-catalog

383e7c1

mikemurray approved these changes Jul 22, 2019

View reviewed changes

manueldelreal approved these changes Jul 23, 2019

View reviewed changes

mikemurray added 3 commits July 26, 2019 10:14

Merge branch 'develop' into fix-4735-mpaktiti-catalog

07b5238

Merge branch 'develop' into fix-4735-mpaktiti-catalog

f48a554

refactor: disable jsdoc for functions thay live inside meteor methods

7037276

Signed-off-by: Mike Murray <hellomikemurray@gmail.com>

mikemurray merged commit 7662bab into develop Jul 26, 2019

mikemurray deleted the fix-4735-mpaktiti-catalog branch July 26, 2019 23:39

kieckhafer mentioned this pull request Aug 2, 2019

release v2.1.0 #5409

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(fix): set slugified title as permalink #5264

(fix): set slugified title as permalink #5264

mpaktiti commented Jul 3, 2019

zenweasel commented Jul 4, 2019

mpaktiti commented Jul 4, 2019

zenweasel commented Jul 4, 2019

mpaktiti commented Jul 5, 2019

zenweasel commented Jul 8, 2019

zenweasel commented Jul 8, 2019

mikemurray Jul 8, 2019

mpaktiti Jul 9, 2019

mikemurray Jul 9, 2019

manueldelreal Jul 11, 2019

mpaktiti Jul 11, 2019

mpaktiti Jul 15, 2019

mpaktiti Jul 15, 2019 •

edited

mikemurray Jul 16, 2019

mpaktiti Jul 18, 2019

manueldelreal commented Jul 9, 2019

mikemurray left a comment

manueldelreal left a comment

(fix): set slugified title as permalink #5264

(fix): set slugified title as permalink #5264

Conversation

mpaktiti commented Jul 3, 2019

Issue

Solution

Breaking changes

Testing

zenweasel commented Jul 4, 2019

mpaktiti commented Jul 4, 2019

zenweasel commented Jul 4, 2019

mpaktiti commented Jul 5, 2019

zenweasel commented Jul 8, 2019

zenweasel commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpaktiti Jul 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manueldelreal commented Jul 9, 2019

mikemurray left a comment

Choose a reason for hiding this comment

manueldelreal left a comment

Choose a reason for hiding this comment

mpaktiti Jul 15, 2019 •

edited