Slugify Collectives and Pages

[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

• [x] resolve updater issue
• [x] update previewer
• [x] add/fix tests

🏁 Checklist

• [x] Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
• [x] 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[bot]]

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

[Koc]

For some reason migrations stop work for me (even previous). Both autoloading and dependency injection resolving not works. Any idea how to fix that?

devcontainer@5e275b290d01:/var/www/html$ ./occ migrations:migrate collectives -v

In MigrationService.php line 399:
                                                                                         
  [Exception]                                                                            
  Database error when running migration 021200Date20240725154232 for app collectives     
  Migration step 'OCA\Collectives\Migration\Version021200Date20240725154232' is unknown  
                                                                                         

Exception trace:
  at /var/www/html/lib/private/DB/MigrationService.php:399
 OC\DB\MigrationService->migrate() at /var/www/html/core/Command/Db/Migrations/MigrateCommand.php:42
 OC\Core\Command\Db\Migrations\MigrateCommand->execute() at /var/www/html/3rdparty/symfony/console/Command/Command.php:298
 Symfony\Component\Console\Command\Command->run() at /var/www/html/3rdparty/symfony/console/Application.php:1040
 Symfony\Component\Console\Application->doRunCommand() at /var/www/html/3rdparty/symfony/console/Application.php:301
 Symfony\Component\Console\Application->doRun() at /var/www/html/3rdparty/symfony/console/Application.php:171
 Symfony\Component\Console\Application->run() at /var/www/html/lib/private/Console/Application.php:187
 OC\Console\Application->run() at /var/www/html/console.php:87
 require_once() at /var/www/html/occ:11

In MigrationService.php line 482:
                                                                                         
  [InvalidArgumentException]                                                             
  Migration step 'OCA\Collectives\Migration\Version021200Date20240725154232' is unknown  
                                                                                         

Exception trace:
  at /var/www/html/lib/private/DB/MigrationService.php:482
 OC\DB\MigrationService->createInstance() at /var/www/html/lib/private/DB/MigrationService.php:497
 OC\DB\MigrationService->executeStep() at /var/www/html/lib/private/DB/MigrationService.php:395
 OC\DB\MigrationService->migrate() at /var/www/html/core/Command/Db/Migrations/MigrateCommand.php:42
 OC\Core\Command\Db\Migrations\MigrateCommand->execute() at /var/www/html/3rdparty/symfony/console/Command/Command.php:298
 Symfony\Component\Console\Command\Command->run() at /var/www/html/3rdparty/symfony/console/Application.php:1040
 Symfony\Component\Console\Application->doRunCommand() at /var/www/html/3rdparty/symfony/console/Application.php:301
 Symfony\Component\Console\Application->doRun() at /var/www/html/3rdparty/symfony/console/Application.php:171
 Symfony\Component\Console\Application->run() at /var/www/html/lib/private/Console/Application.php:187
 OC\Console\Application->run() at /var/www/html/console.php:87
 require_once() at /var/www/html/occ:11

migrations:migrate <app> [<version>]

[Koc]

hey @mejo- !

It seems like ./occ migrations:migrate collectives works in a different way comparing to ./occ upgrade. Here some interesting scenario:

1. Truncate all collectives/pages
2. Create few collectives with few pages. No need to delete anything.
3. Run ./occ migrations:migrate collectives
4. Migrations migrated, slugs were successfully generated :heavy_check_mark:
5. Drop columns/migration records
6. Bump module version
7. Run ./occ upgrade :boom: . There an error that collectives folder /myCurrentUserName/files/Collectives/myCollectiveName not exists

It doesn't matter have I removed collectives/pages or not. The fact is the same code works fine with migrations:migrate but not works with upgrade. I've try to debug it but no luck.

That's why I've moved slugs generation into separate console command. So, after upgrade old pages/collectives would have same urls like before.

Details

string(29) "/admin/files/Collectives/aaaa"  <---- exception message with path + collective name

#0 /var/www/html/lib/private/Files/Node/Folder.php(107): OC\Files\Node\Root->get()
#1 /var/www/html/apps/collectives/lib/Service/PageService.php(100): OC\Files\Node\Folder->get() <--- why collective folder not available? Only 1 page was trashed, not whole collective
#2 /var/www/html/apps/collectives/lib/Service/PageService.php(498): OCA\Collectives\Service\PageService->getCollectiveFolder()
#3 /var/www/html/apps/collectives/lib/Migration/Version021500Date20240820000001.php(78): OCA\Collectives\Service\PageService->findAll()
#4 /var/www/html/lib/private/DB/MigrationService.php(522): OCA\Collectives\Migration\Version021500Date20240820000001->postSchemaChange()
#5 /var/www/html/lib/private/DB/MigrationService.php(395): OC\DB\MigrationService->executeStep()
#6 /var/www/html/lib/private/legacy/OC_App.php(694): OC\DB\MigrationService->migrate()
#7 /var/www/html/lib/private/Updater.php(325): OC_App::updateApp()
#8 /var/www/html/lib/private/Updater.php(236): OC\Updater->doAppUpgrade()
#9 /var/www/html/lib/private/Updater.php(100): OC\Updater->doUpgrade()
#10 /var/www/html/core/Command/Upgrade.php(190): OC\Updater->upgrade()
#11 /var/www/html/3rdparty/symfony/console/Command/Command.php(326): OC\Core\Command\Upgrade->execute()
#12 /var/www/html/3rdparty/symfony/console/Application.php(1078): Symfony\Component\Console\Command\Command->run()
#13 /var/www/html/3rdparty/symfony/console/Application.php(324): Symfony\Component\Console\Application->doRunCommand()
#14 /var/www/html/3rdparty/symfony/console/Application.php(175): Symfony\Component\Console\Application->doRun()
#15 /var/www/html/lib/private/Console/Application.php(187): Symfony\Component\Console\Application->run()
#16 /var/www/html/console.php(87): OC\Console\Application->run()
#17 /var/www/html/occ(11): require_once('...')
#18 {main}

[Koc]

I don't know why cypress is failing with 401: Unauthorized error :confused:

[juliusknorr]

Cypress failures were unrelated, should be fixed after a rebase to latest main by https://github.com/nextcloud/collectives/pull/1623

[Koc]

@juliusknorr I've already rebased few times :smile:. Mentioned PR fixes Behat tests but not Cypress.

[juliusknorr]

Ah sorry, seems I mixed up repos and have not pushed a fix for collectives yet. We'll need to adapt https://github.com/nextcloud/text/pull/6819 for this repo as well then

[Koc]

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

But now there is another error :man_shrugging: :

Could not download app contacts

[max-nextcloud]

@Koc I've finally found some time to look into this PR. Thanks a lot for starting it in the first place. I'd be happy to rebase it, resolve conflicts and take a look at cypress next week. Would this be okay or do you have local changes that you have not pushed yet? Don't want to step on your feet.

[Koc]

@max-nextcloud I've just rebased branch and resolve conflicts. Yeah, I will be very appreciated if you can help me with Cypress. I've completely stuck with it. Feel free to push into my branch

[max-nextcloud]

I'm prioritizing #1685 for now so we can start with a clean ci and rebase this on top of it. Sorry this is taking so long.

[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-]

@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 :construction:
• fix conflicts after rebase :heavy_check_mark:
• do manual testing :hourglass_flowing_sand:
• reopen new PR based on main repository instead of my fork :hourglass:

And after that I will disappear :smile: . 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 :face_with_peeking_eye:

[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 :heavy_check_mark:

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

   :mag: Preview

   

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

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

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

cc @mejo-