Slugify Collectives and Pages (fork-based)

[Koc]

📝 Summary

This is a very beginning of my PR to slugify urls for Collectives and Pages. For now urls are too long and hard to understand if using cyrylic in this entities.

Here an example of some of them: http://localhost/index.php/apps/collectives/%D0%A3%D0%BA%D1%80%D0%B0%D1%97%D0%BD%D1%81%D1%8C%D0%BA%D1%96%20%D1%84%D1%83%D1%82%D0%B1%D0%BE%D0%BB%D1%96%D1%81%D1%82%D0%B8/%D0%AF%D1%80%D0%BC%D0%BE%D0%BB%D0%B5%D0%BD%D0%BA%D0%BE%20%D0%90%D0%BD%D0%B4%D1%80%D1%96%D0%B9%20%D0%9C%D0%B8%D0%BA%D0%BE%D0%BB%D0%B0%D0%B9%D0%BE%D0%B2%D0%B8%D1%87?fileId=187. Just compare it with slugified version: http://localhost/index-php/apps/collectives/ukrayinski-futbolisti-1/page-1-yurchenko-andriy-mykolayovich. Easy to read, 3 times shorter.

🖼️ Video

Screencast.from.2024-09-01.17-18-46.webm

🚧 TODO

• resolve updater issue
• update previewer
• add/fix tests

🏁 Checklist

• Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
• Sign-off message is added to all commits
• Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
• Documentation (README or documentation) has been updated or is not required

[mejo-]

Hey @Koc, thanks for looking into this. Definitely a good idea to slugify collectives and page titles for cleaner URLs.

Without looking into all the details, I wonder if we could implement this in a way that doesn't need the sluggified string to be persisted in the database.

Why not make calculate the slug of titles/names on demand in the backend and add them as attribute to the collectives and pages in the API responses? Then the frontend could always check for name/title match first and for slug match second. What do you think about this approach?

[Koc]

@mejo- we have a lot of pages in collectives, so I would like to reduce amount of runtime operations to speedup backend endpoints.

[Koc]

I've achieved some progress. Now it more or less works, we have even redirects if slugs have been renamed. You can observe how it works in the video that attached to PR description

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

[Koc]

@mejo- there is one more reason to persist slug in the database: we can make it editable in upcoming PRs. Sometimes page titles can be long, so we can give possibility to our users shorten them

[max-nextcloud]

I rebased on top of current master to resolve conflicts.

[juliusknorr]

@mejo- Just came to my mind in the call with @max-nextcloud - This could also be a building block for removing the unique constraint on the collective name

[max-nextcloud]

I briefly looked at the failing cypress tests and I think they point to an actual issue:

If one already has pages with links to other pages those links won't work after the migration - or will they?

update: Just checked and there are only new routes added - so the old urls should still work. It's only if the old url also happens to match a new route that problems might occure... so if my preexisting collective is called 'collective-1' I would end up on the route for a collective with slug 'collective' and id '1' instead of the old route for the collective with the name collective-1

[max-nextcloud]

@mejo- Just came to my mind in the call with @max-nextcloud - This could also be a building block for removing the unique constraint on the collective name

Currently we are limiting the collective name to not match the name of an existing team. So that would still be in effect even if the url schema changes.

[max-nextcloud]

Proposal: no collective id in urls

I propose to only use the collective name or the slug in the url. We could use one endpoint with a single parameter :collective. If we find a collective with a matchings slug we render it. If there's not collective with that slug we look for a collective with that name.

During the migration we generate slugs for all existing collectives, starting with collectives whose names could be a slug. We set their slugs to match the name. Then we generate slugs for the remaining collectives. If a slug has already been used we append a number (slug-1) similarly to how we handle file name conflicts.

This way we ensure that old urls still point to the same collective.

After the migration when rendering links ourselves we always use the slug. But we cannot update existing links so we also allow accessing collectives by their names.

When changing a slug or creating a new collective we follow the same principle. We can then later remove the restriction to require destinct names as the slugs may already have conflicts for distinct names such as 'hello' and 'hellö'.

update: Talked through this with @juliusknorr and @mejo- and the current approach seems fine. Maybe we can add a fallback to also check for collectives with a name: slug-id if we cannot find a collective with a matching id.
One benefit of including the ids is that the collective / page can still be identified after a rename.

[Koc]

@max-nextcloud

One benefit of including the ids is that the collective / page can still be identified after a rename.

Yeah, it was a main idea of having it as part of the url.

I did the suash & rebase with conflicts solving. But we still have failed Cypress tests which I not able to fix

[max-nextcloud]

Thanks for the rebase, @Koc
@mejo- will take another look at the migrations and I will take a stab at fixing the cypress tests.

[max-nextcloud]

I'm currently looking into the page-links cypress test. I noticed two things:

Use of the old page format

When clicking a page on the sidebar it opens the old subroute for me:
http://nextcloud.local/index.php/apps/collectives/test-leaf-22/New%20page?fileId=1806

Old paths are not converted to new paths

The cypress test clicks on a number of links in a document that still have the old format. I think that's a good thing to still have around. When visiting such a link the url stays in the old format leading to a failing assertion.

I'm not sure - but I think we should rewrite urls to have the new format on visiting the old format. For one this will hopefully make the test pass... and it will also use the shorter nicer urls more often.

[mejo-]

@Koc I don't understand why the collective slug contains the collectiveId and the page slug doesn't contain the pageId.

Looking at the frontend router changes it seems to me that the slug is expected to be separate from the IDs, which makes sense to me. So maybe generateCollectiveSlug() should not add -${collectiveId} either, right?

[Koc]

no, we need ids for both pages and collectives. This is needed because collective/page can be renamed but we want to make it possible content using old URLs.

At the same time I can't remember why we can't store fileId inside slug on BE side (PR was opened almost year ago). I guess it's because of routing stuff - we need to distinguish old and new routes

[mejo-]

I have to admit that I don't fully understand the purpose of this code. When is this expected to happen? Is the idea to redirect to the slugified URL when the old URL is opened?

[Koc]

yeah, exactly. The only one thing: it stopped work after one of latest rebases. I'm investigating it

[mejo-]

I think it's confusing to use <slug>-<id> for the collective URL part and page-<id>-<slug> for the page URL part. Why not use <slug>-<id> for the page as well?

[Koc]

This was added as a prefix for some edge cases when generated slug/old file path contains digits and dash at the beginning

[mejo-]

@juliusknorr I've opened PR with backport: #1625.

But now there is another error 🤷‍♂️ :

Could not download app contacts

@Koc seems like the CI job runs into a rate limit of the app store. I wonder whether this is because the base branch for this PR lives in your Github realm and not in nextcloud. Given that you're member of the nextcloud organisation, would you mind opening the PR again as a branch of nextcloud/collectives, instead of Koc/nextcloud-collectives? I can imagine that this helps with the failing Cypress tests.

[mejo-]

Dear @Koc, do you plan to continue working on this PR in the near future? Or would you prefer us to take over work on it? Both options are perfectly fine for us and there's no urgency or need to rush. I just wanted to check in with you.

[Koc]

hey @mejo- !

I will try to do next things:

• fix/answer to your code review comments 🚧
• fix conflicts after rebase ✔️
• do manual testing ⏳
• reopen new PR based on main repository instead of my fork ⌛

And after that I will disappear 😄 . I don't know how to fix Cypress tests

[mejo-]

I will try to do next things:

Thanks so much @Koc, much appreciated! Let me know when you want us to take over or when you have questions. Don't mind the upgrade app test failures, they're unrelated and will be fixed after the next app release.

Regarding creating the slugs initially, I think the best would be to implement a live-migration repair step that is scheduled anytime after the the upgrade and triggered by cron. See https://docs.nextcloud.com/server/latest/developer_manual/digging_deeper/repair.html#repair-step-types

[mejo-]

fix conflicts after rebase ✔️

Sorry @Koc, I just merged another larger PR that created rebase/merge conflicts for this PR one more time. They should be easy to fix this time though. Maybe you can open a new PR based on a branch in this repo soon, then I can do the manual rebase myself next time 🫣

[Koc]

So, here we go. I did rebase one more time and fixed few things.
Here my manual test scenario:

1. switch to the current main branch

2. install module ./occ app:enable circles text collectives

3. create 2 collectives with some nested pages

4. switch to my branch

5. run ./occ upgrade

6. open collectives and pages by legacy urls -> it stil works, navigation is possible if slugs not generated ✔️

7. run ./occ maintenance:repair -> slugs generated and present in database for all collectives/pages ✔️

   🔍 Preview

   

8. refresh page -> redirected to new slugified url, validated for both collective index/pages ✔️

9. rename collective (actually circle) -> slug changes, we've got redirected to the new url after change ✔️

Here reopened PR #1802 created from original Nextcloud repo instead of my own fork. Please don't abandon it 🙏 , I spent really too much time on such functionality

cc @mejo-