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...

[TriMoon]

Looking forward for this work guys 👍

[TriMoon]

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

Better choice...

[TriMoon]

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