Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 1 addition & 1 deletion apps/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ def deploy_resources(self, request, queryset):

for instance in queryset:
instance.set_k8s_values()
instance.url = get_URI(instance.k8s_values)
instance.url = get_URI(instance)
instance.save(update_fields=["k8s_values", "url"])

deploy_resource.delay(instance.serialize())
Expand Down
26 changes: 7 additions & 19 deletions apps/forms/custom.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
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 @@ -19,20 +18,20 @@ 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")
default_url_subpath = forms.CharField(max_length=255, required=False, label="Custom default url subpath")
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
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")

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


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(
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."
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
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')

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

self.fields["default_url_subpath"].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"
" (<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."
)
)
Expand Down Expand Up @@ -61,7 +60,7 @@ def _setup_form_helper(self):
AccordionGroup(
"Advanced settings",
PrependedText(
"custom_default_url",
"default_url_subpath",
"Subdomain/",
template="apps/partials/srv_prepend_input_group_custom_app.html",
),
Expand All @@ -72,17 +71,6 @@ def _setup_form_helper(self):
)
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 @@ -120,7 +108,7 @@ class Meta:
"port",
"image",
"tags",
"custom_default_url",
"default_url_subpath",
]
labels = {
"note_on_linkonly_privacy": "Reason for choosing the link only option",
Expand Down
6 changes: 3 additions & 3 deletions apps/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,9 @@ def get_URI(instance):
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)
if hasattr(instance, "default_url_subpath") and instance.default_url_subpath != "":
URI = URI + "/" + instance.default_url_subpath
logger.info("Modified URI by adding default url subpath for the custom app: " + URI)
return URI


Expand Down

This file was deleted.

23 changes: 23 additions & 0 deletions apps/migrations/0018_customappinstance_default_url_subpath.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Generated by Django 5.1.1 on 2024-11-19 13:27

import apps.models.app_types.custom.custom
from django.db import migrations, models


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

operations = [
migrations.AddField(
model_name="customappinstance",
name="default_url_subpath",
field=models.CharField(
blank=True,
default="",
max_length=255,
validators=[apps.models.app_types.custom.custom.validate_default_url_subpath],
),
),
]
28 changes: 27 additions & 1 deletion apps/models/app_types/custom/custom.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,40 @@
import regex as re
from django.core.exceptions import ValidationError
Comment on lines +1 to +2
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 django.db import models

from apps.models import AppInstanceManager, BaseAppInstance

from .base import AbstractCustomAppInstance


def validate_default_url_subpath(candidate):
"""
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"
" ( - ), 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, candidate):
raise ValidationError(error_message)


class CustomAppInstanceManager(AppInstanceManager):
model_type = "customappinstance"


class CustomAppInstance(AbstractCustomAppInstance, BaseAppInstance):
custom_default_url = models.CharField(max_length=255, default="", blank=True)
default_url_subpath = models.CharField(
validators=[validate_default_url_subpath], max_length=255, default="", blank=True
)
objects = CustomAppInstanceManager()
49 changes: 48 additions & 1 deletion apps/tests/test_forms.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import pytest
from django.contrib.auth import get_user_model
from django.core.exceptions import ValidationError
from django.template import Context, Template
from django.test import TestCase

from apps.forms import CustomAppForm
from apps.models import Apps, AppStatus, Subdomain, VolumeInstance
from apps.models.app_types.custom.custom import validate_default_url_subpath
from projects.models import Flavor, Project

User = get_user_model()
Expand Down Expand Up @@ -44,7 +47,7 @@ def setUp(self):
"port": 8000,
"image": "ghcr.io/scilifelabdatacentre/image:tag",
"tags": ["tag1", "tag2", "tag3"],
"custom_default_url": "valid-custom_default_url/",
"default_url_subpath": "valid-default_url_subpath/",
}

def test_form_valid_data(self):
Expand Down Expand Up @@ -193,3 +196,47 @@ def test_form_rendering(self):
self.assertIn('value="private"', rendered_form)
self.assertIn('value="link"', rendered_form)
self.assertIn('value="public"', rendered_form)


invalid_default_url_subpath_list = [
"invalid space",
'invalid_"_double_quote',
"invalid_<_less_than_sign",
"invalid_\\_backslash",
"invalid_|_pipe",
"invalid_^_caret",
"invalid_{_left_curly_brace",
"invalid_?_question_mark",
]

valid_default_url_subpath_list = [
"valid_ÄÄ_unicode_charecters",
"valid_aa/_forward_slash",
"valid_____underscore",
"_aa/bb/c_format",
"valid_-_hiphen",
"_ad-frt/fgh_cd_",
"ÅÄaad1234",
]


@pytest.mark.parametrize("valid_default_url_subpath", valid_default_url_subpath_list)
def test_valid_default_url_subpath(valid_default_url_subpath):
valid_check = True
try:
validate_default_url_subpath(valid_default_url_subpath)
except ValidationError:
valid_check = False

assert valid_check


@pytest.mark.parametrize("invalid_default_url_subpath", invalid_default_url_subpath_list)
def test_invalid_default_url_subpath(invalid_default_url_subpath):
valid_check = True
try:
validate_default_url_subpath(invalid_default_url_subpath)
except ValidationError:
valid_check = False

assert not valid_check
46 changes: 0 additions & 46 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

This file was deleted.

79 changes: 42 additions & 37 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
@@ -1,41 +1,46 @@
{% 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);
<head>
<script>
document.addEventListener("DOMContentLoaded", function () {
const subdomainInput = document.getElementById("id_subdomain");
const customDefaultUrlFormModifiedUrlText = document.getElementById("id_original_default_url_subpath_form");
const customDefaultUrlFormModifiedHelpTextUrl = document.getElementById("id_custom_default_url_subpath_form_help_text");

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

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

updateUrlWithSubdomain();

subdomainInput.addEventListener("blur", updateUrlWithSubdomain);
});
</script>
</head>

<body>
<div>
<div class="input-group">
{% if crispy_prepended_text %}
<span class="input-group-text" id="id_original_default_url_subpath_form"></span>
{% endif %}
{{ field }}
</div>
{% if field.field.bottom_help_text %}
<small class="form-text text-muted">{{ field.field.bottom_help_text }}</small>
{% endif %}

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