SS-1176 Users are able to set a custom default start URL for their apps #249

anondo1969 · 2024-10-29T15:07:54Z

Description

Source: https://scilifelab.atlassian.net/browse/SS-1176

when we deploy an app it show app-subdomain.serve.scilifelab.se but if user wants the base page to be app-subdomain.serve.scilifelab.se/docs then that should be configurable. There is a field in the database for each app, called url. That’s the field we need to be able to modify with the extra setting.
This should be an advanced setting field for Custom app (similar to how site_dir is an advanced field for specifically Shiny apps now).

Checklist

If you're unsure about any of the items below, don't hesitate to ask. We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

This pull request is against develop branch (not applicable for hotfixes)
I have included a link to the issue on GitHub or JIRA (if any)
I have included migration files (if there are changes to the model classes)
I have included, reviewed and executed tests (unit and end2end) to complement my changes
I have updated the related documentation (if necessary)
I have added a reviewer for this pull request
I have added myself as an author for this pull request
In the case I have modified settings.py, then I have also updated the studio-settings-configmap.yaml file in serve-charts

Further comments

Anything else you think we should know before merging your code!

… change to input form, and added validation for the url

…est class

…t-start-URL-for-their-apps

churnikov · 2024-11-12T09:50:22Z

apps/types_/url_additional_path_validation.py

I'm not sure, that this class belongs here, tbh

Like, it's a module for custom types, but this a validator, not a new type if it makes sence

And you could use this docs to make it a part of initialization of a CharField on a apps/forms/custom.py:22

I also agree that this is a validator rather than a new type and therefore does not belong in _types. Perhaps there was inspiration from the SubdomainCandidateName class but in that case it is logically a complete class with its own behavior and properties that we may continue to extend in the future.

Rewrote it in the apps/forms/custom.py

churnikov · 2024-11-12T09:55:38Z

apps/types_/url_additional_path_validation.py

+    __name = None
+
+    def __init__(self, name: str):
+        self.__name = name


I don't see a reason to not do it like this:)

Suggested change

__name = None

def __init__(self, name: str):

self.__name = name

def __init__(self, name: str):

self._name = name

Removed it.

churnikov · 2024-11-12T09:58:34Z

apps/types_/url_additional_path_validation.py

+        error_message = (
+            "Your custom default URL is not valid, please correct it. "
+            "It must be 1-53 characters long."
+            " It can contain only Unicode letters, digits, hyphens"


So we are allowing unicode characters in the subpath?

Yes. We had a discussion and agreed on keeping unicode characters.

churnikov · 2024-11-12T10:29:44Z

templates/apps/partials/srv_prepend_input_group_custom_app.html

I have a few question about this file:

Why didn't you include the templates/apps/partials/srv_prepend_append_input_group.html?

I don't really like the <script> to be placed here. On the other hand, it sort of logical for it to be here, considering all the design choices that we've made. I just don't like two things: that it potentially won't work if it'll be used anywhere else, other than an app with an input for subdomain and second thing is that scripts should be grouped together in the header and not scattered.

I think because of the script I created a new file. I placed the script in the header section. Should I try to use the templates/apps/partials/srv_prepend_append_input_group.html for this?

churnikov · 2024-11-12T10:50:48Z

portal/views.py

    app_orms = (app_model for app_model in APP_REGISTRY.iter_orm_models() if issubclass(app_model, SocialMixin))

    for app_orm in app_orms:
-        logger.info("Processing: %s", app_orm)


Is there a reason to remove this?

churnikov · 2024-11-12T11:01:24Z

apps/tests/test_forms.py

            "port": 8000,
            "image": "ghcr.io/scilifelabdatacentre/image:tag",
            "tags": ["tag1", "tag2", "tag3"],
+            "custom_default_url": "valid-custom_default_url/",


I'd call it a default_url_subpath, but I'm not insisting. Just that it's not a full URL, that's bothering me a little.

Rename this field to default_url_subpath all together.

alfredeen

I do not see any unit tests of the URL sub path validation.

… into SS-1176-Ability-for-users-to-set-a-default-start-URL-for-their-apps

Copilot

Copilot reviewed 5 out of 8 changed files in this pull request and generated 1 suggestion.

Files not reviewed (3)

templates/apps/partials/srv_prepend_input_group_custom_app.html: Language not supported
portal/views.py: Evaluated as low risk
apps/tests/test_forms.py: Evaluated as low risk

Copilot · 2024-11-18T15:46:44Z

apps/helpers.py

    URI = URI.strip("/")
+    if hasattr(instance, "custom_default_url") and instance.custom_default_url != "":
+        URI = URI + "/" + instance.custom_default_url
+        logger.info("Modified URI by adding custom default url for the custom app: " + URI)


String concatenation using the '+' operator is not recommended. Use formatted strings or the logger's formatting capabilities instead.

Suggested change

logger.info("Modified URI by adding custom default url for the custom app: " + URI)

logger.info("Modified URI by adding custom default url for the custom app: %s", URI)

anondo1969 · 2024-11-19T13:45:07Z

I do not see any unit tests of the URL sub path validation.

Wrote two (pass/fail) unit tests for this.

churnikov

We are close:)

I think after this I'd approve

churnikov · 2024-11-19T15:14:02Z

apps/models/app_types/custom/custom.py

+import regex as re
+from django.core.exceptions import ValidationError


Suggested change

import regex as re

from django.core.exceptions import ValidationError

import regex as re

from django.core.exceptions import ValidationError

This should be caught by isort I think

Thank you. Addressed it.

churnikov · 2024-11-19T15:22:46Z

templates/apps/partials/srv_prepend_input_group_custom_app.html

Following this conversation https://github.com/ScilifelabDataCentre/serve/pull/249/files/3527fd079d58deac18bd5cd9461d242c37dbd8c7..83f995417664557d998e06b577565d4e008756c1#r1837858395

So I'd say, it would be best to also {%include %} the already existing markup for the prepend append group and not copy it.

Thank you. Worked on it.

hamzaimran08

I would also suggest changing the file templates/apps/partials/srv_prepend_append_input_group.html with the following. The prepend text should be bold in my opinion

- <span class="input-group-text">{{ crispy_prepended_text }}</span>
+ <span class="input-group-text fw-bold">{{ crispy_prepended_text }}</span>

hamzaimran08 · 2024-11-19T14:40:20Z

apps/forms/custom.py

 from django import forms
+from django.core.exceptions import ValidationError
+from django.utils.safestring import mark_safe



Suggested change

from django.urls import reverse

Thank you. Addressed it.

hamzaimran08 · 2024-11-19T14:42:15Z

apps/forms/custom.py

    image = forms.CharField(max_length=255, required=True)
    path = forms.CharField(max_length=255, required=False)

+    default_url_subpath = forms.CharField(max_length=255, required=False, label="Custom default url subpath")


Suggested change

default_url_subpath = forms.CharField(max_length=255, required=False, label="Custom default url subpath")

default_url_subpath = forms.CharField(max_length=255, required=False, label="Custom URL subpath")

Thanks. Changed it.

hamzaimran08 · 2024-11-19T14:42:29Z

apps/forms/custom.py

        self.fields["volume"].initial = None

+        self.fields["default_url_subpath"].widget.attrs.update({"class": "textinput form-control"})
+        self.fields["default_url_subpath"].help_text = "Specify a non-default start URL if your app requires that."


Suggested change

self.fields["default_url_subpath"].help_text = "Specify a non-default start URL if your app requires that."

self.fields["default_url_subpath"].help_text = "Specify a non-default start URL if your app requires that."

apps_url = reverse('portal:apps')

Thanks. Modified it.

hamzaimran08 · 2024-11-19T14:50:30Z

apps/forms/custom.py

+                "We will display this URL for your app in our app catalogue."
+                " Keep in mind that when your app does not have anything on the root URL"
+                " (<span id='id_custom_default_url_subpath_form_help_text'></span>), if a user manually"
+                " navigates to the root URL, they will see an empty page there."


Suggested change

"We will display this URL for your app in our app catalogue."

" Keep in mind that when your app does not have anything on the root URL"

" (), if a user manually"

" navigates to the root URL, they will see an empty page there."

f"Note: This changes the URL connected to the Open button for an app"

f" on the Serve <a href='{apps_url}'>Apps & Models</a> page."

I think this text needs to be shorter. We can add a link to documentation and have longer text and examples of how to use this in a separate part of the docs in my opinion. This could be a separate task.

Thanks. Edited it.

hamzaimran08 · 2024-11-19T15:00:36Z

apps/forms/custom.py

+                        "Subdomain/",
+                        template="apps/partials/srv_prepend_input_group_custom_app.html",


Suggested change

"Subdomain/",

template="apps/partials/srv_prepend_input_group_custom_app.html",

mark_safe("Subdomain/"),

template="apps/partials/srv_prepend_append_input_group.html",

I think one way to this this is to not use a separate template but use apps/partials/srv_prepend_input_group_custom_app.html, add an id to the Prepend input group element using span so that it is accessible through the javascript id match that you use. I have also tried to shorten the id, you can ofcourse choose another name.

Then I suggest moving parts of your javascript to the templates/apps/create_base.html file. The changes can be the following:

Load get_setting

{% load get_setting %}

In the script section make the following changes

+ const subdomainInput = document.getElementById("id_subdomain"); + const customDefaultUrlFormModifiedUrlText = document.getElementById("id_custom_default_url_prepend"); + function updateUrlWithSubdomain() { + const subdomainValue = subdomainInput.value.trim(); + const displayValue = subdomainValue || "subdomain_name"; + customDefaultUrlFormModifiedUrlText.innerHTML = `${displayValue}.{% get_setting 'DOMAIN' %}/`; + } // User is "finished typing," do something function doneTyping () { let subdomain_input = $("#id_subdomain").val(); if (subdomain_input.length == 0) { //console.log("Empty subdomain input"); clearSubdomainValidation(); // Empty text field. Enable the submit button and clear any validation styles. $("#submit-id-submit").prop('disabled', false); } else { //console.log(`Checking subdomain validity for: ${subdomain_input}`); // Run check if there is text in the input field // Convert the input subdomain to lowercase for the validation. // OK because the input form and the server does the same. checkSubdomainValidity(subdomain_input.toLowerCase()); } + updateUrlWithSubdomain(); } ........ ........ ........ htmx.onLoad(function(content) { runScript(); + updateUrlWithSubdomain(); })

We already have an eventlistener listening to keystrokes on the subdomain input box. So we can use that one to update the URL for your input prepend text.

Thanks a lot for this suggestion. It helped me a lot to understand the existing scripts.

… into SS-1176-Ability-for-users-to-set-a-default-start-URL-for-their-apps update.

alfredeen

LGTM. Thanks for adding the unit tests. I will let others review the recent changes as well before approving.

anondo1969 · 2024-11-20T15:05:06Z

LGTM. Thanks for adding the unit tests. I will let others review the recent changes as well before approving.

Thank you so much.

hamzaimran08

So, I think just this small change left. We need to catch the console error in case the element does not exist. Otherwise LGTM. Good work.

hamzaimran08 · 2024-11-20T17:03:16Z

templates/apps/create_base.html

+        const subdomainValue = subdomainInput.value.trim();
+        const displayValue = subdomainValue || "subdomain_name";
+        customDefaultUrlFormModifiedUrlText.innerHTML = `${displayValue}.{% get_setting 'DOMAIN' %}/</b>`;


Suggested change

const subdomainValue = subdomainInput.value.trim();

const displayValue = subdomainValue || "subdomain_name";

customDefaultUrlFormModifiedUrlText.innerHTML = `${displayValue}.{% get_setting 'DOMAIN' %}/`;

try {

const subdomainValue = subdomainInput.value.trim();

const displayValue = subdomainValue || "subdomain_name";

customDefaultUrlFormModifiedUrlText.innerHTML = `${displayValue}.{% get_setting 'DOMAIN' %}/`;

} catch (error) {

console.log("No custom url subpath option in this app type.");

}

Thanks a lot. With it, it passed all the e2e test cases.

…ect names

… into SS-1176-Ability-for-users-to-set-a-default-start-URL-for-their-apps update

hamzaimran08

LGTM. Good work.

anondo1969 · 2024-11-21T15:01:01Z

LGTM. Good work.

Thanks a lot.

anondo1969 and others added 10 commits October 29, 2024 15:52

showing advance settings in custom app creation

4994f10

SS-1176 showing advance settings in custom app creation

27b763b

SS-1176 saving the default custom url to the database

b5e8313

SS-1176 modified saving custom default url to database, added dynamic…

636ff9c

… change to input form, and added validation for the url

SS-1176 allowing forward slash at the end of the custom default url

1425fdb

SS-1176 adding the new custom_default_url field to the CustomAppFormT…

90f2b98

…est class

SS-1176 allowing empty string to be valid

06b74f8

SS-1176 included migration file

42c1e9f

Merge branch 'develop' into SS-1176-Ability-for-users-to-set-a-defaul…

b5cf2ed

…t-start-URL-for-their-apps

remove unnecessary log message

758939e

akochari changed the title ~~Ss 1176 ability for users to set a default start url for their apps~~ Users are able to set a custom default start URL for their apps Nov 1, 2024

akochari changed the title ~~Users are able to set a custom default start URL for their apps~~ SS-1176 Users are able to set a custom default start URL for their apps Nov 1, 2024

akochari and others added 2 commits November 1, 2024 11:19

change the field names and explanations

60002fc

SS-1176 separated the url validation part to ensure re-usability

b3860ab

anondo1969 self-assigned this Nov 1, 2024

anondo1969 requested a review from hamzaimran08 November 1, 2024 11:37

anondo1969 marked this pull request as ready for review November 1, 2024 11:38

anondo1969 added 6 commits November 1, 2024 13:33

SS-1176 removed custom_default_url from the redeployment_fields

ad3feae

SS-1176 used Django URLValidator for the url validation

dc37d17

SS-1176 edit url validation error message

7b0ec61

SS-1176 fix pre-commit issues

8d560bd

SS-1176 alllowing unicode letter for url validation

9ce5d5b

SS-1176 remove url tests

3527fd0

churnikov reviewed Nov 12, 2024

View reviewed changes

alfredeen reviewed Nov 13, 2024

View reviewed changes

Merge branch 'develop' of https://github.com/ScilifelabDataCentre/serve…

6c4b717

… into SS-1176-Ability-for-users-to-set-a-default-start-URL-for-their-apps

hamzaimran08 requested a review from Copilot November 18, 2024 15:44

Copilot AI reviewed Nov 18, 2024

View reviewed changes

anondo1969 added 2 commits November 19, 2024 14:24

SS-1176 modified the code acccording to the review

cde08ad

SS-1176 adding new migration file

83f9954

anondo1969 requested review from alfredeen and churnikov November 19, 2024 13:45

churnikov reviewed Nov 19, 2024

View reviewed changes

hamzaimran08 reviewed Nov 19, 2024

View reviewed changes

anondo1969 added 2 commits November 20, 2024 15:15

Merge branch 'develop' of https://github.com/ScilifelabDataCentre/serve…

47476ce

… into SS-1176-Ability-for-users-to-set-a-default-start-URL-for-their-apps update.

SS-1176 finishing modifications according to the reviews

a9b049b

anondo1969 requested a review from hamzaimran08 November 20, 2024 14:38

alfredeen reviewed Nov 20, 2024

View reviewed changes

hamzaimran08 reviewed Nov 20, 2024

View reviewed changes

anondo1969 added 3 commits November 21, 2024 11:44

SS-1176 catching console error and fixing e2e test for duplicate proj…

bbcb968

…ect names

Merge branch 'develop' of https://github.com/ScilifelabDataCentre/serve…

a329f08

… into SS-1176-Ability-for-users-to-set-a-default-start-URL-for-their-apps update

SS-1176 removing comment from e2e test

7d84b18

anondo1969 requested a review from hamzaimran08 November 21, 2024 12:33

hamzaimran08 approved these changes Nov 21, 2024

View reviewed changes

anondo1969 merged commit 8163d0d into develop Nov 21, 2024
3 checks passed

anondo1969 deleted the SS-1176-Ability-for-users-to-set-a-default-start-URL-for-their-apps branch November 21, 2024 15:02

	logger.info("Modified URI by adding custom default url for the custom app: " + URI)
	logger.info("Modified URI by adding custom default url for the custom app: %s", URI)

		import regex as re
		from django.core.exceptions import ValidationError

	default_url_subpath = forms.CharField(max_length=255, required=False, label="Custom default url subpath")
	default_url_subpath = forms.CharField(max_length=255, required=False, label="Custom URL subpath")

	self.fields["default_url_subpath"].help_text = "Specify a non-default start URL if your app requires that."
	self.fields["default_url_subpath"].help_text = "Specify a non-default start URL if your app requires that."
	apps_url = reverse('portal:apps')

		"Subdomain/",
		template="apps/partials/srv_prepend_input_group_custom_app.html",

SS-1176 Users are able to set a custom default start URL for their apps #249

SS-1176 Users are able to set a custom default start URL for their apps #249

Uh oh!

Conversation

anondo1969 commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Further comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfredeen Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfredeen left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

anondo1969 commented Nov 19, 2024

Uh oh!

churnikov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamzaimran08 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hamzaimran08 Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

anondo1969 commented Oct 29, 2024 •

edited

Loading

alfredeen Nov 13, 2024 •

edited

Loading

hamzaimran08 Nov 19, 2024 •

edited

Loading