Skip to content
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
4994f10
showing advance settings in custom app creation
anondo1969 Oct 29, 2024
27b763b
SS-1176 showing advance settings in custom app creation
anondo1969 Oct 29, 2024
b5e8313
SS-1176 saving the default custom url to the database
anondo1969 Oct 30, 2024
636ff9c
SS-1176 modified saving custom default url to database, added dynamic…
anondo1969 Oct 31, 2024
1425fdb
SS-1176 allowing forward slash at the end of the custom default url
anondo1969 Oct 31, 2024
90f2b98
SS-1176 adding the new custom_default_url field to the CustomAppFormT…
anondo1969 Oct 31, 2024
06b74f8
SS-1176 allowing empty string to be valid
anondo1969 Oct 31, 2024
42c1e9f
SS-1176 included migration file
anondo1969 Oct 31, 2024
b5cf2ed
Merge branch 'develop' into SS-1176-Ability-for-users-to-set-a-defaul…
akochari Nov 1, 2024
758939e
remove unnecessary log message
akochari Nov 1, 2024
60002fc
change the field names and explanations
akochari Nov 1, 2024
b3860ab
SS-1176 separated the url validation part to ensure re-usability
anondo1969 Nov 1, 2024
ad3feae
SS-1176 removed custom_default_url from the redeployment_fields
anondo1969 Nov 1, 2024
dc37d17
SS-1176 used Django URLValidator for the url validation
anondo1969 Nov 4, 2024
7b0ec61
SS-1176 edit url validation error message
anondo1969 Nov 6, 2024
8d560bd
SS-1176 fix pre-commit issues
anondo1969 Nov 6, 2024
9ce5d5b
SS-1176 alllowing unicode letter for url validation
anondo1969 Nov 11, 2024
3527fd0
SS-1176 remove url tests
anondo1969 Nov 11, 2024
6c4b717
Merge branch 'develop' of https://github.com/ScilifelabDataCentre/ser…
anondo1969 Nov 18, 2024
cde08ad
SS-1176 modified the code acccording to the review
anondo1969 Nov 19, 2024
83f9954
SS-1176 adding new migration file
anondo1969 Nov 19, 2024
47476ce
Merge branch 'develop' of https://github.com/ScilifelabDataCentre/ser…
anondo1969 Nov 20, 2024
a9b049b
SS-1176 finishing modifications according to the reviews
anondo1969 Nov 20, 2024
bbcb968
SS-1176 catching console error and fixing e2e test for duplicate proj…
anondo1969 Nov 21, 2024
a329f08
Merge branch 'develop' of https://github.com/ScilifelabDataCentre/ser…
anondo1969 Nov 21, 2024
7d84b18
SS-1176 removing comment from e2e test
anondo1969 Nov 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions apps/forms/custom.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
from crispy_forms.bootstrap import Accordion, AccordionGroup, PrependedText
from crispy_forms.layout import HTML, Div, Field, Layout, MultiField
from django import forms
from django.core.exceptions import ValidationError
from django.utils.safestring import mark_safe

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from django.urls import reverse

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Addressed it.

from apps.forms.base import AppBaseForm
from apps.forms.field.common import SRVCommonDivField
from apps.models import CustomAppInstance, VolumeInstance
from apps.types_.url_additional_path_validation import UrlAdditionalPathValidation
from projects.models import Flavor

__all__ = ["CustomAppForm"]
Expand All @@ -15,11 +19,24 @@ class CustomAppForm(AppBaseForm):
image = forms.CharField(max_length=255, required=True)
path = forms.CharField(max_length=255, required=False)

custom_default_url = forms.CharField(max_length=255, required=False, label="Custom default start URL")

def _setup_form_fields(self):
# Handle Volume field
super()._setup_form_fields()
self.fields["volume"].initial = None

self.fields["custom_default_url"].widget.attrs.update({"class": "textinput form-control"})
self.fields["custom_default_url"].help_text = "Specify a non-default start URL if your app requires that."
self.fields["custom_default_url"].bottom_help_text = mark_safe(
(
"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_form_help_text'></span>), if a user manually"
" navigates to the root URL, they will see an empty page there."
)
)

def _setup_form_helper(self):
super()._setup_form_helper()

Expand All @@ -40,10 +57,32 @@ def _setup_form_helper(self):
),
SRVCommonDivField("port", placeholder="8000"),
SRVCommonDivField("image"),
Accordion(
AccordionGroup(
"Advanced settings",
PrependedText(
"custom_default_url",
"Subdomain/",
template="apps/partials/srv_prepend_input_group_custom_app.html",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Subdomain/",
template="apps/partials/srv_prepend_input_group_custom_app.html",
mark_safe("<span id='id_custom_default_url_prepend'>Subdomain/</span>"),
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' %}/</b>`;
+    }

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

),
active=False,
),
),
css_class="card-body",
)
self.helper.layout = Layout(body, self.footer)

def clean_custom_default_url(self):
cleaned_data = super().clean()

custom_url = cleaned_data.get("custom_default_url", None)
custom_url_candidate = UrlAdditionalPathValidation(custom_url)
try:
custom_url_candidate.validate_candidate()
return custom_url
except ValidationError as e:
self.add_error("custom_default_url", e)

def clean_path(self):
cleaned_data = super().clean()

Expand Down Expand Up @@ -81,6 +120,7 @@ class Meta:
"port",
"image",
"tags",
"custom_default_url",
]
labels = {
"note_on_linkonly_privacy": "Reason for choosing the link only option",
Expand Down
20 changes: 16 additions & 4 deletions apps/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,12 +221,15 @@ def update_status_time(status_object, status_ts, event_msg=None):
status_object.save(update_fields=["time", "info"])


def get_URI(values):
def get_URI(instance):
values = instance.k8s_values
# Subdomain is empty if app is already deleted
subdomain = values["subdomain"] if "subdomain" in values else ""
URI = f"https://{subdomain}.{values['global']['domain']}"

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)
Copy link

Copilot AI Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copilot uses AI. Check for mistakes.
return URI


Expand Down Expand Up @@ -261,7 +264,16 @@ def create_instance_from_form(form, project, app_slug, app_id=None):
do_deploy = True
else:
# Only re-deploy existing apps if one of the following fields was changed:
redeployment_fields = ["subdomain", "volume", "path", "flavor", "port", "image", "access", "shiny_site_dir"]
redeployment_fields = [
"subdomain",
"volume",
"path",
"flavor",
"port",
"image",
"access",
"shiny_site_dir",
]
logger.debug(f"An existing app has changed. The changed form fields: {form.changed_data}")

# Because not all forms contain all fields, we check if the supposedly changed field
Expand Down Expand Up @@ -357,5 +369,5 @@ def save_instance_and_related_data(instance, form):
instance.save()
form.save_m2m()
instance.set_k8s_values()
instance.url = get_URI(instance.k8s_values)
instance.url = get_URI(instance)
instance.save(update_fields=["k8s_values", "url"])
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Generated by Django 5.1.1 on 2024-10-29 14:36

from django.db import migrations, models


class Migration(migrations.Migration):
dependencies = [
("apps", "0017_alter_streamlitinstance_port"),
]

operations = [
migrations.AddField(
model_name="customappinstance",
name="custom_default_url",
field=models.CharField(blank=True, default="", max_length=255),
),
]
3 changes: 3 additions & 0 deletions apps/models/app_types/custom/custom.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from django.db import models

from apps.models import AppInstanceManager, BaseAppInstance

from .base import AbstractCustomAppInstance
Expand All @@ -8,4 +10,5 @@ class CustomAppInstanceManager(AppInstanceManager):


class CustomAppInstance(AbstractCustomAppInstance, BaseAppInstance):
custom_default_url = models.CharField(max_length=255, default="", blank=True)
objects = CustomAppInstanceManager()
1 change: 1 addition & 0 deletions apps/tests/test_forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ def setUp(self):
"port": 8000,
"image": "ghcr.io/scilifelabdatacentre/image:tag",
"tags": ["tag1", "tag2", "tag3"],
"custom_default_url": "valid-custom_default_url/",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this field to default_url_subpath all together.

}

def test_form_valid_data(self):
Expand Down
46 changes: 46 additions & 0 deletions apps/types_/url_additional_path_validation.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@alfredeen alfredeen Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrote it in the apps/forms/custom.py

Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import regex as re
from django.core.exceptions import ValidationError


class UrlAdditionalPathValidation:
"""
Validating a custom url path addition to the existing URL.
The validation limits to that additional part only.
For example, if the original URL is: https://xyz.serve-dev.scilifelab.se,
and we want to add a path so that it becomes https://xyz.serve-dev.scilifelab.se/new-path,
then it will only validate 'new-path' part.
"""

__name = None

def __init__(self, name: str):
self.__name = name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it.


def is_valid(self) -> bool:
"""Determines if the proposed path addition is valid."""
try:
self.validate_candidate()
return True
except ValidationError:
return False

def validate_candidate(self):
"""
Validates a custom default url path addition.
The RegexValidator will raise a ValidationError if the input does not match the regular expression.
It is up to the caller to handle the raised exception if desired.
"""
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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we are allowing unicode characters in the subpath?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

" ( - ), forward slashes ( / ), and underscores ( _ )."
" It cannot start or end with a hyphen ( - ) and "
"cannot start with a forward slash ( / )."
" It cannot contain consecutive forward slashes ( // )."
)

pattern = r"^(?!-)(?!/)(?!.*//)[\p{Letter}\p{Mark}0-9-/_]{1,53}(?<!-)$|^$"

if not re.match(pattern, self.__name):
raise ValidationError(error_message)
1 change: 0 additions & 1 deletion portal/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ def get_unique_apps(queryset, app_ids_to_exclude):
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to remove this?

filters = ~Q(app_status__status="Deleted") & Q(access="public")
if collection:
filters &= Q(collections__slug=collection)
Expand Down
41 changes: 41 additions & 0 deletions templates/apps/partials/srv_prepend_input_group_custom_app.html
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Worked on it.

Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
{% include "apps/partials/div_label_with_help_toggle.html" with help_message=field.help_text %}
{% load get_setting %}
<div>
<div class="input-group">
{% if crispy_prepended_text %}
<span class="input-group-text" id="id_original_url_custom_default_url_form"></span>
{% endif %}
{{ field }}
</div>
{% if field.field.bottom_help_text %}
<small class="form-text text-muted">{{ field.field.bottom_help_text }}</small>
{% endif %}

{% if field.errors %}
{% for error in field.errors %}
<div class="client-validation-feedback client-validation-invalid">
{{ error }}
</div>
{% endfor %}
{% endif %}
</div>

<script>

const subdomainInput = document.getElementById("id_subdomain");
const customDefaultUrlFormModifiedUrlText = document.getElementById("id_original_url_custom_default_url_form");
const customDefaultUrlFormModifiedHelpTextUrl = document.getElementById("id_custom_default_url_form_help_text");

function updateUrlWithSubdomain() {
const subdomainValue = subdomainInput.value.trim();
const displayValue = subdomainValue || "subdomain_name";

customDefaultUrlFormModifiedUrlText.innerHTML = `https://${displayValue}.{% get_setting 'DOMAIN' %}/`;
customDefaultUrlFormModifiedHelpTextUrl.innerHTML = `https://${displayValue}.{% get_setting 'DOMAIN' %}/</b>`;
}

updateUrlWithSubdomain();

subdomainInput.addEventListener("blur", updateUrlWithSubdomain);

</script>
Loading