make slug editable

[junnotantra]

Started from my question on #308 about my needs to edit slug, I make some changes myself to achieve that. First time contributing here. Honestly I'm not that familiar about the design of this repo. I've tried to make changes as unobtrusive as possible, but let me know if the changes is not acceptable.

Actually there are several concerns when I'm working on this:

1. I'm not sure whether slug is should to be editable or not
2. Currently on the editor slug data is sent twice, from a hidden field and a disabled text field. Not sure if it's intended or not, but I'm using both value to check if slug is being edited by user
3. value.Get("field_name") only return the first value of the specified field, I use value["field_name"][1] to get the second value of a field. Not sure if it's deemed not neat

Let me know what you guys think :)

[nilslice]

Hi @junnotantra - apologies for the delay in responding to your PR. First, thank you for taking the time to implement this! I've been focused on other projects and have not had time to dive in to much Ponzu work, so I'll have to let another team member look at this before we give the sign-off on merging.

I'm not fully convinced that the slug should be editable after the content has been stored, but I'm willing to hear out other's thoughts on the subject.

@olliephillips & @ponzu-cms/team -- anyone have time to dig in on this?

[olliephillips]

I can't get into this myself @nilslice & @junnotantra ATM, but I think the case for slug being editable depends on whether that URL is likely to be used in a way visible to search engines, in which case you'd want to ensure it was free of errors but also, best reflected the content linked to.

But, in this scenario, since it would be a permalink, once indexed, would you want to be able to change it, and end up needing redirections to sort Google out - I don't know TBH.

With a backend/server Ponzu client, there's a strong case for being able to reference a piece of content semantically, rather than on ID, in which case, you'd want the slug to evolve as the content evolved.

I'm totally on the fence, so that's probably not very helpful.

[olliephillips]

Hey @junnotantra I've checked out your PR and built a new ponzu project with it. Slug editing works fine for me.

The only suggestion I'd make relates to the UX - default behaviour with Ponzu content item allows saving of nil values, i.e a song with no title. However, if you try to wipe the slug and save, you get a 404 - the result of an error return I expect - and the slug is not set to nil.

Maybe an alternative, which might avoid the 404, could be to reimplement the routine which creates a slug when one not provided on a new content item? That might be better than seeing a 404 in the editor. Or perhaps some JS validation on the slug field which prevents save operation? But these are only suggestions and provided for your consideration. It is unlikely someone would try to save an empty slug.

I can see no problems in the code myself, and on balance, I think this is a nice feature to have.

[junnotantra]

Hi @olliephillips Thank you for the feedback. Sure it's a bad UX when user get 404 after deleting the slug. I will look into it again and make necessary changes to the PR. Maybe I'll do it within this week.

[junnotantra]

Hi @olliephillips , I've updated this PR with validation on frontend and backend side. Please help take a look. Thanks :clinking_glasses:

[olliephillips]

Hey @junnotantra, I've fetched the changes, but there appears to be small problem. For me at least, the dialog alert stating slug cannot be empty, fires on all attempts to save a content item, except the initial save when the item is frist created, and this occurs regardless of whether the slug is blanked or not. I haven't dug into it, but if you need more info/help just ask.

[junnotantra]

Sorry for the buggy PR last time. :pray: I've updated the PR again to fix the frontend validation.

[olliephillips]

Great, thanks @junnotantra, that's working as I'd expect now👍

@nilslice is there more discussion to be had on this feature? I think this is useful for those that need/want it, and it there's no impact on the UX for those that don't.

[nilslice]

Thanks, @junnotantra!

@olliephillips - I agree, I think it's a nice feature and something others have asked for in the past - so I say let's go for it!

The only additional testing that should be done is the API call to get content by slug & ensure that once a slug is changed the API no longer returns data by the old slug selector and only does for the new one.

Can that be verified by one of you prior to merge?

Also, it might be worth checking the full-text search by enabling indexing on a content type, and then doing a search for the initial slug, changing it, and then ensuring the old slug does not still lead a search to find the content any longer, but that a search for the new slug does locate it.

Let me know if any clarification is needed there! Thanks all for the work on this.

[junnotantra]

You're welcome @nilslice

I've already tested the API call to get content by slug. Using the old and new slug already return desired response. Even though sometimes need to hard reload the browser to make the old slug return 'not found'

For the full-text search, I haven't try this feature yet actually. I'll try exploring this feature if I have time this weekend. Maybe @olliephillips can help me with this also. :smile:

[olliephillips]

I'd checked the new slug was available over the API, but not that the old one was not, but looks like you have @junnotantra. I'm going to be a bit busy this week so might not be that helpful I'm afraid but I'll jump in when I can.

@nilslice is there a reason the update on item might not hit the full-text search, is there an intermediate cache or index constructed for purpose search (which would make total sense)? Hands up - it's not a feature I've used either😀

[olliephillips]

@junnotantra just checking-in. Did you get to test the full-text search yourself?

[olliephillips]

I've checked full text search out. It works fine. A content item which appears in full text search reflects the new slug, and a search conducted on the new slug returns the correct content item.

@nilslice I did notice one minor funny, unrelated to this PR, in that, once you build with content item overriding the IndexContent method, it's not until a new content item is created (or existing saved) that the index returns anything over the API.

At least that is how it seemed over a couple of tests.

Going to merge this into dev.

[junnotantra]

Hi @olliephillips thank you for helping with the testing and merging the PR. Sorry for not being responsive, I got some emergency matter to take care for the last few days :pray: