Bare2share - Redirect bare domain to defined #shareRoot

[maphew]

Addresses #658
Adds Options >> Other >> Share Settings:

• Adds a checkbox to enable/disable bare domain redirect to /share
• "Show login in share theme" option

Requires a note that is a) shared, and b) defined as #shareRoot

[pano9000]

Hi,

thanks for the PR – just a couple of general comments and a tiny request to fix the i18n part.

apart from that:
Could you kindly run prettier on your modified files?
This will automatically format the files in accordance with the project's defaults.

You can do that via npx prettier --write ./path/to/your/file-or-folder

[maphew]

Thanks for review @pano9000, I appreciate it. I believe I've addressed what you pointed out.

A caution for anyone else following: it's not safe to run prettier globally on the repo, much more than a handful of files will be changed! 😆

[pano9000]

@maphew thanks for your changes, sorry for this, but there need to be some additional changes:
Since the option is now a boolean, you will need to also change all of your code to handle booleans, instead of strings (e.g. comparison like options.redirectBareDomain === "true" → can be dropped completely)

let me know, if you need any assistance with that or if there are any questions

[pano9000]

could you use boolean values here please :-)
(i.e. just drop the quotation marks :-))

[maphew]

done. When I did so intellisense complained about boolean not assignable to type string. Consulting LLM (Claude Sonnet) about this led to commit 15fc572. I don't know if this was the right thing.

[pano9000]

since getOption should return a boolean, you can drop the comparison entirely here.

[maphew]

sorry for this, but there need to be some additional changes

I'm perfectly fine with this. Java/typescript is whole new world for me and I appreciate the scrutiny.

[pano9000]

thanks for your changes and additional commits

I'll get back to you with more details on this evening after my work -> I might've overlooked something here (or the Type Definitions in the option_interface are wrong/weird):
i.e. when we take another example: "isPasswordSet" -> it is defined as boolean in OptionDefinitions interface
in the "OptionMap" type though it seems to define the types as Record<OptionNames, string>;

it seems like options are actually stored as strings, but can get retrieved as their "intended" type via the extra methods provided in options.ts
e.g. for booleans → you could use optionService.getOptionBool and you would get a boolean instead of a string

i have to admit, that is a tiny bit confusing and also it does not look like there is any consistency across the codebase on when what is used either! :-)

if you don't mind I'll lend you a hand though with that

[maphew]

yes please, for your help (and explanations) :-)
It is gratifying to hear it's not just me who finds the boolean/string not straightforward.

[pano9000]

@maphew sorry mate, I did not get to work on this directly today, but working on my other "friendly numbers" PR did make me grasp a better understanding of the optionsMap vs optionsService thing, that we were talking about in my comment above.

I'll provide you with some suggested improvements tomorrow morning before I head to work – then we can finally get this one ready for Elian to merge :-)

[pano9000]

so, I took a look and noticed that I mixed up the "option" service from backend and the "option" service from the frontend:
The backend one has the getBoolean method, frontend does not... Implementing it would be trivial, but we would still have issues with the options_init having "true" boolean values:
With the way the options currently work, I was wrong about using true "boolean" values inside options_init :-(
I think we should change that at some point, but that would require some bigger refactoring, that is out of scope for this PR.

long story short:
I think if we revert back to aab3595, and ignore my comments and the last two commits, we should be good.
You can use the following to revert

// switch to your branch
git switch bare2share2

// this creates a copy of the current branch, *just in case*
git switch -c bare2share2_backup

// go back to the "live" branch and do the reset
git switch bare2share2
git reset --hard aab35955bf6af74595a49e4737278709d8b907b8

// force push the changed branch (the little + in front of the branch name is important here)
git push +bare2share2

Sorry for the extra work and confusion I may have caused!

Afterwards there is only one change I would do in the TPL, to make it look and behave the same as the other options:

I'd change your current TPL with the one below

const TPL = `
<div class="options-section">
    <h4>${t("share.title")}</h4>

    <label class="tn-checkbox">
        <input class="form-check-input" type="checkbox" name="redirectBareDomain" value="true">
        ${t("share.redirect_bare_domain")}
    </label>
    <p class="form-text">${t("share.redirect_bare_domain_description")}</p>

    <label class="tn-checkbox">
        <input class="form-check-input" type="checkbox" name="showLoginInShareTheme" value="true">
        ${t("share.show_login_link")}
    </label>
    <p class="form-text">${t("share.show_login_link_description")}</p>
</div>`;

this will make it look better integrated with the rest of the options:

apart from that: I've tested it and can confirm that it works → we should document the requirement to have a "#shareRoot" tag set, because otherwise you are greeted with:

-- Thanks for your work here!

[maphew]

thank you for the detailed help. I've made the changes.

The + syntax did not work for me, yielding unknown repository messages. I couldn't find help pages that made sense to me (a generic internet search for git and + is essentially impossible 🙃 ) so I used --force instead.

I'm experimenting with:

Adding a "Check Share Root Status" button and status message area that appears only when the "Redirect bare domain" option is checked

The status check:
Runs automatically when the page loads if the redirect option is enabled
Runs when the redirect option is toggled on
Can be manually triggered by clicking the "Check Share Root Status" button
Shows a success message in green with the note title if a #shareRoot note is found
Shows an error message in red if no #shareRoot note is found

but don't have it working yet. I'll do that as a separate pull request since I'm not all sure if I can succeed.

[pano9000]

thank you!
to me that looks good now, but we will need @eliandoran approval of course :-)

changes done

[maphew]

the validation worked out! And the settings are new self-documented

[eliandoran]

@maphew , I've changed a bit the layout to match @pano9000 's since I think it looks great.

Another important point... instead of simply redirecting to /share, wouldn't it make more sense to also rewrite all addresses such as /share/STi3RCMhUvG6 to /STi3RCMhUvG6?

[maphew]

Thanks @eliandoran, I agree on layout changes. As for the redirect change, sure! If you think it works. I thought there might be conflict determining if secured and open were on same level (my understanding of routing is very limited).

There is a UX gain by knowing that any url starting with /share (or whatever the path is) is public but I don't have a firm opinion on that. (If anything my default is the reverse: everything except /mine or /edit or ... would be public.). Bottom line, I'm happy to use whatever you think fits best.

[pano9000]

I tend to agree with @maphew on that we maybe should keep /share for now?

I mean potentially this could still be changed in the future as an option – but I would see it as a separate PR anyways.

[maphew]

I tend to agree with @maphew on that we maybe should keep /share for now?

I mean potentially this could still be changed in the future as an option – but I would see it as a separate PR anyways.

I'm going to take another run at making the target url configurable. #667 included it initially, but the branch was too messed up, and I hadn't considered a target of /.