Fix duplicate for all collections

[rchlfryn]

Description

Duplicating a collection fails because the slug isn't unique, and Payload's unique: true validator doesn't work correctly in multi-tenant environments. We added custom validation to ensureUniqueSlug to handle this. It looks like Payload is working on a solution so we can remove our workaround eventually and use unique: true again. payloadcms/payload#14938

Until then, we added getIndexedTitleAndSlug when making duplicatePageToTenant #322 which will increment the slug and title based on the number of documents found in the current tenant.. I checked payload's docs and they handle duplication like this:

By Default, unique and required text fields Payload will append "- Copy" to the original document value.

I think we should do the same rather than increment and hopefully they will fix the unique prop issue.

Related Issues

Fixes #827

Key Changes

Adds - Copy to title and -copy to slugs on duplication

[github-actions]

Preview deployment: https://fix-uniq-slug.preview.avy-fx.org

[busbyk]

I don't think it's a good idea to skip this check since we still don't want duplicate slug values. We just want to append something to the slug value like we do in the src/collections/Pages/endpoints/duplicatePageToTenant.ts server action.

For example, if we skip checking that the slug is unique on duplicate and then the user doesn't manually change the slug, we'll have two documents with the same slug...

What about using the beforeDuplicate field hook to use getIndexedTitleAndSlug like you suggested in this PR description (or similar logic).

[rchlfryn]

For example, if we skip checking that the slug is unique on duplicate and then the user doesn't manually change the slug, we'll have two documents with the same slug...

This will not happen. The user will will be forced to update the slug upon saving the newly duplicated page since the validation will catch this. The problem is the creation is failing because of the validation. I should have explained this in the description. I updated it.

What about using the beforeDuplicate field hook to use getIndexedTitleAndSlug like you suggested in this PR description (or similar logic).

Let me try this. I think it will work.. Good callout

[rchlfryn]

@busbyk Completely rewrote the description for this PR so take a look.

[busbyk]

I like the - Copy for title and -copy for slug behavior. It's nice to mimic Payload's behavior here.

I did have one thought though: If you try to copy the same page twice you still get a "The following field is invalid: slug" error on duplicate which is pretty unhelpful to the user. They have to infer that they can't duplicate it because there's another slug with the same value.

We have a custom error message in ensureUniqueSlug but since the duplicate action doesn't submit the form, it just makes an API call, the ValidationError that we send doesn't get displayed next to the custom slug field because Payload's FieldError component only shows when the form has been submitted.

We'd ideally like it to show this (which currently shows if you save with a duplicate slug bug not when using the duplicate action in the case where there's already a -copy version):

So we could change our ValidationError to an:

    throw new APIError(
      `A ${collection.labels.singular} with the slug "${value}" already exists. Slug must be unique${collectionHasTenantField ? ' per avalanche center' : ''}.`,
      400,
    )

Which would only show the error message in the toast. This would then be the same for save and for duplicate but we'd just have a slightly different behavior than the norm. That's probably the preferred option here.

Alternatively, we could combine this -copy appending with the indexing logic from the removed getIndexedTitleAndSlug so that duplicate will always work even if a user has already copied the document before.

What are your thoughts on this?

[rchlfryn]

@busbyk I went ahead an changed the validation error to an API error. I think it is the easiest and best solution. I did double check to see how payload handles this with the unique prop enabled (without the multi-tenant bug), and they just share the generic The following field is invalid toast.

[busbyk]

@busbyk I went ahead an changed the validation error to an API error. I think it is the easiest and best solution. I did double check to see how payload handles this with the unique prop enabled (without the multi-tenant bug), and they just share the generic The following field is invalid toast.

I like it. Seems like the best way to provide the right message to the user in a consistent way.

[busbyk]

Looks great!