ether
allow option to make pad names case-insensitive
fixes #3844
Adds Option to:
lowerCasePadIds = true // default is false
I hesitate to merge this because it makes it impossible to access existing pads that have upper-case letters. What do you think about checking if the pad exists with the name as-is, and if not then redirect the user to the lower-case name?
Also, is the default Unicode case conversion algorithm sufficient? Or should this use .toLocaleLowerCase()?
I hesitate to merge this because it makes it impossible to access existing pads that have upper-case letters. What do you think about checking if the pad exists with the name as-is, and if not then redirect the user to the lower-case name?
I could implement such a check if you want, but I would argue against it. As it would introduce inconsistency [1] from the user and admin perspective. Because whoever choose to use lowercase only should be aware that turning it on won't be backwards compatible anyways. (This is also why I made it configurable) I will add a comment highlighting this.
Also, is the default Unicode case conversion algorithm sufficient? Or should this use
.toLocaleLowerCase()?
The whole code base does not use toLocaleLowerCase, but I am happy to change it, as it should not have any drawbacks.
[1]: Scenario: Two pads that were created before (EtHeRpaD and etherpad). Which one should we choose? Also one of them will be hidden forever. The only solution to it is migrating it before.
Do the Tests normally fail? Or should I add anything to make them work?
Scenario: Two pads that were created before (
EtHeRpaDandetherpad). Which one should we choose?
The idea is that if the user visits EtHeRpaD then they will get EtHeRpaD because it already exists. If they use any different capitalization, they will be redirected to etherpad.
Also one of them will be hidden forever. The only solution to it is migrating it before.
With your change as-is EtHeRpaD is hidden forever. With my suggested proposal users will still be able to visit EtHeRpaD as long as they capitalize it exactly.
In addition to hiding existing pads with upper-case letters, I'm worried about finding and adjusting all of the other code paths that might not hit the new .toLowerCase() call. For example, without additional changes it would still be possible for the createPad HTTP API to create a pad with upper-case letters. Plugins would also be able to create pads with upper-case letters. Keeping Etherpad case-sensitive but redirecting to the lower-case name when creating a new pad avoids unintended issues.
Do the Tests normally fail? Or should I add anything to make them work?
The tests are failing because settings.json.template is simply renamed to settings.json before the tests run. The template has "caseInsensitivePadNames": true, so any tests with upper-case letters will fail. Change it to false and it should be OK.
Is there a way to writing tests with specific settings turned on? (e.g. the caseInsensitivePadeNames)?
I rebased your PR onto latest develop and squashed the commits together.
Is there a way to writing tests with specific settings turned on? (e.g. the caseInsensitivePadeNames)?
Yes; see here for an example. In order for test changes to have an effect you'll need to change:
const {caseInsensitivePadNames} = require('../utils/Settings');
// ...
if (caseInsensitivePadNames) {
to:
const settings = require('../utils/Settings');
// ...
if (settings.caseInsensitivePadNames) {
Also: I ran the front-end tests locally (http://localhost:9001/tests/frontend/) with the new option turned on and some of the timeslider tests failed. Can you look into that? (The front-end tests aren't run in a GitHub pull request workflow because the automated tests require Sauce Labs credentials.)
The failing tests mostly have to do with these lines: (note e.g. FRONTED_TESTS_OxmGelclDdwtqszpderu)
https://github.com/ether/etherpad-lite/blob/64757d1636299c743840ef9d1f5a8d507cb385f1/src/tests/frontend/helper.js#L85-L100
as with the setting on they are converted to lowercase. I could add toLowerCase() to them.
@rhansen Any updates on this PR? Hit me up if something is still missing.
Nice that the bot added the stale label, but maybe somebody can review this PR? @rhansen thanks!
@DanielHabenicht I just rebased your commits to the latest branch and tested the feature. It's working as expected. Older pads are even accessible if I enable the option. Newer pads on the other hand are relying on this restriction and test123 and TEST123 are redirected to the same pad. Thanks for the contribution. I'll include it in the next release as it is an optional feature that even if activated doesn't break any logic.
