Fix #1670 - Add option to manually assign an URL to a container

[dannycolin]

Before submitting your pull request

• I agree to license my code under the MPL 2.0 license.
• I rebased my work on top of the main branch.
• I ran npm test and all tests passed.
• I added test coverages if relevant.

Description

Add an option in "manage site list" panel to manually assign an URL to a container

Remaining tasks

• Add a message when the list is empty
• Move hardcoded style values in popup.js to popup.css
• Move hardcoded strings to /locales
• ???
• Profit

Type of change

Select all that apply.

• Bug fix
• New feature
• Major change (fix or feature that would cause existing functionality to work differently than in the current version)

Tag issues related to this pull request:

• Manually add URL's to a container #1670

[achernyakevich-sc]

Looks like this block should be inside if (new URL(url).protocol == "https:" || new URL(url).protocol == "http:") { because if condition fail assignment is not required.

Though probably more correct would be to do not make any assumptions about protocol, but expect from user that if he manually add URL then it would be fully qualified URL containing protocol. So in general code flow could looks like (it is not a real code but pseudo-code):

let url = document.getElementById("edit-sites-add-assignment-info").value;

try {
    let urlObj = new URL(url);

    if (urlObj.protocol == "https:" || urlObj.protocol == "http:") {
        const userContextId = Logic.currentUserContextId();
        await Utils.setOrRemoveAssignment(
            false, urlObj.origin, userContextId, false
        );

        const assignments = await Logic.getAssignmentObjectByContainer(userContextId);
        this.showAssignedContainers(assignments);

        errorMessage.style.display = "none";
        assignmentInput.style.border = "1px solid var(--input-border-color)";
    }
} catch {
    errorMessage.style.display = "block";
    assignmentInput.style.border = "2px solid var(--input-destructive-border-color)";
}

[TriMoon]

@achernyakevich-sc
Why not use the below instead of your if clause? 😉

if (/^http(s)?::/.test(url)) {

The used regExp can easily be expanded/modified...

[achernyakevich-sc]

Why not use the below instead of your if clause? 😉

At first it is "pseudo-code" as I mentioned in the comment. :) I just used code that already exists and regrouped it to show more correct execution flow.

But I think RegExp is not necessary here and now. MAC works for HTTP and HTTPS protocols only (AFAIK). And now code is "easily readable". We don't need to spend effort to care about that will never happen. Use KISS approach - Keep It Simple Stupid.

P.S.> One of my England customers said years ago - if I have a car that can drive to China it does not mean that I need to travel to Beijing.

[TriMoon]

It must be personal preference then i guess, but i bet my code runs faster with less extra resource usage... 😛
All little bits help in the long run of any software..

---

"easily readable"

It is easy readable as can be for anyone that knows JavaScript well enough...

[dannycolin]

@achernyakevich-sc At first, I used exactly what you're proposing with a try...catch. However, I wanted the user to have the ability to omit http(s) similar to the previous PR for this feature. In this case, I had to check the URL validity and force one if it's missing. I feel like it makes it more intuitive for users that don't know that valid URLs need a protocol.

@TriMoon It's more readable as @achernyakevich-sc said. Also, the performane is negligeable in this case.

[TriMoon]

@dannycolin Anything is negligible on PC's these days with their power, when you look at each one on it's own, think about the bigger total 😉

But anyhow did you notice #2710 (comment) 🤔

[dannycolin]

This very specific case is fine. I'll try my best to look at your other comment during the weekend and make sur to report back on GitHub ;).

[achernyakevich-sc]

However, I wanted the user to have the ability to omit http(s) similar to the previous PR for this feature. In this case, I had to check the URL validity and force one if it's missing.

@dannycolin It radically changes how we need to implement it! Below I will try to summarize how it would be simpler (to my mind) for understanding by users and simpler for implementation. :)

I have made some research of MAC behaviour and didn't check the code. So I will make some assumptions that proposal below is based on (to lazy to check the code base). The main one - MAC records not a whole URL of site, not protocol+host name, but just site origin. It means that though MAC works for HTTP and HTTPS, but it will work the same for both URLs - http://example.com/ and https://example.com/. Conclusion - UI should not only provide possibility to omit protocol part, but should force to enter site origin without protocol and path name. Only site origin!

Currently saving is triggered by button click. As soon as user have made click then it should go the following way:

• take url string that leading http:// or https:// will be removed.
• get urlObject based on cleaned url and added explicit https://.
• as we explicitly added https:// we don't need checking for protocol.
• get site origin from urlObject and expose it to UI in input as cleaned up.

So user will see on UI exactly that will be saved as configuration and used for triggering container switching.

In this case code could looks like:

let urlInput = document.getElementById("edit-sites-add-assignment-info")
let url = urlInput.value.replace(/^http(s)?:\/\//, "");

try {
    let urlObject = new URL("https://" + url);
    urlInput.value = urlObject.origin;

    const userContextId = Logic.currentUserContextId();
    await Utils.setOrRemoveAssignment(
        false, urlObject.origin, userContextId, false
    );

    const assignments = await Logic.getAssignmentObjectByContainer(userContextId);
    this.showAssignedContainers(assignments);

    errorMessage.style.display = "none";
    assignmentInput.style.border = "1px solid var(--input-border-color)";
} catch {
    errorMessage.style.display = "block";
    assignmentInput.style.border = "2px solid var(--input-destructive-border-color)";
}

To make it even more explicit you could set placeholder text for the input as site origin and it will be visible before user enter anything there. :)

[achernyakevich-sc]

Anything is negligible on PC's these days with their power, when you look at each one on it's own, think about the bigger total

@TriMoon It is not a problem if something takes more time almost uncatchable for user but it as well takes electricity. I think you will agree that:

• any operation on PC cause electricity costs.
• wasting of electricity increases carbon footprints and produces a damage to nature.

Problem is that small destructive things happen many-many times! Firefox and MAC is popular. :)

For us it costs almost nothing to make piece of code more electricity-effective. But resulting effect to nature could be very valuable.

[dannycolin]

@achernyakevich-sc I agree. It makes sense to drop the protocol detection since we don't store it anyway. I'll refactor the patch to match your proposal. Thanks!

[TriMoon]

Looking forward for this work guys 👍

[TriMoon]

url.origin can be non-existant/undefined due to the check above...

[dannycolin]

What "check" are you referring to?

[TriMoon]

   if (new URL(url).protocol == "https:" || new URL(url).protocol == "http:") {
      url = new URL(url);
   } else {

As you can see you change url here to be an URL(), but before this check url was a string.
Thus you can't access the .origin member if it isn't a URL() yet 😉

Good thing my eyes were trained before we had IDE's that do type checking automatically 😸