servala/servala-portal
3
0
Fork 0
Code Issues 21 Pull requests 5 Projects 2 Releases Packages 2 Activity Actions

Include link to support in error message #149

Merged
tobru merged 7 commits from error-with-help into main 2025-07-11 14:54:01 +00:00
Conversation 5 Commits 7 Files changed 6 +182 -30
tobru commented 2025-07-07 11:38:50 +00:00
Owner
Copy link

Closes #142

Closes #142
rixx was assigned by tobru 2025-07-07 11:38:50 +00:00
tobru added 2 commits 2025-07-07 11:38:51 +00:00
do not dismiss message when error message
All checks were successful
Tests / test (push) Successful in 25s
Details
5fa3d32c57
tobru commented 2025-07-07 11:39:36 +00:00
Author
Owner
Copy link

I'm not entirely happy with the implementation, but this is what I could come up with. Maybe there is a better / simpler way to do that?

I'm not entirely happy with the implementation, but this is what I could come up with. Maybe there is a better / simpler way to do that?
rixx reviewed 2025-07-07 11:59:27 +00:00
rixx left a comment
Member
Copy link

LGTM – can be merged as-is, just some suggestions for possibly better code structure.

LGTM – can be merged as-is, just some suggestions for possibly better code structure.
src/servala/core/models/service.py Outdated
@ -857,0 +879,4 @@
"""
support_url = organization.urls.support
# Combine the main message with the support message template
full_message = _("{message} {support_message}").format(
rixx commented 2025-07-07 11:54:01 +00:00
Member
Copy link

The full message doesn’t need to be an i18n string – better off as just as f"{message} {support_message}"

The full message doesn’t need to be an i18n string – better off as just as `f"{message} {support_message}"`
tobru marked this conversation as resolved
src/servala/frontend/views/mixins.py Outdated
@ -4,3 +6,4 @@
from servala.core.models import Organization
# Import the support message template from the service model
rixx commented 2025-07-07 11:54:45 +00:00
Member
Copy link

I don't think comments like this add much to the code, FWIW (of course up to you if you want to keep it).

I don't think comments like this add much to the code, FWIW (of course up to you if you want to keep it).
tobru marked this conversation as resolved
src/servala/frontend/views/mixins.py Outdated
@ -91,2 +96,4 @@
def has_permission(self):
return self.has_organization_permission() and super().has_permission()
def format_error_with_support(self, message, **kwargs):
rixx commented 2025-07-07 11:58:45 +00:00
Member
Copy link

Pretty repetitive to have this method on both the form and the view – I think it’d be cleaner to centralise this:

class Organization(...):
    def add_support_message(self, message):
        support_message = _(
            "Need help? We're happy to help via the <a href='{support_url}'>support form</a>."
        ).format(support_url=self.urls.support)
        return mark_safe(f"{message} {support_message}")
Pretty repetitive to have this method on both the form and the view – I think it’d be cleaner to centralise this: ```python class Organization(...): def add_support_message(self, message): support_message = _( "Need help? We're happy to help via the <a href='{support_url}'>support form</a>." ).format(support_url=self.urls.support) return mark_safe(f"{message} {support_message}") ```
tobru marked this conversation as resolved
tobru added 1 commit 2025-07-10 14:11:49 +00:00
refactor into a shared function
All checks were successful
Tests / test (push) Successful in 27s
Details
3c270d9c12
tobru added 2 commits 2025-07-10 14:32:45 +00:00
format and retext control plane errors
All checks were successful
Tests / test (push) Successful in 28s
Details
9c3ce54bb3
tobru commented 2025-07-10 14:34:02 +00:00
Author
Owner
Copy link

@rixx Thanks for the review. I iterated over it again, and I'm happy to get another review before merging it.

@rixx Thanks for the review. I iterated over it again, and I'm happy to get another review before merging it.
rixx reviewed 2025-07-11 10:09:36 +00:00
rixx left a comment
Member
Copy link

Looks good to me! One comment on using mark_safe on input that we do not control directly, which I don't have enough k8s knowledge to judge (should be safe?), ready to merge from my POV.

Looks good to me! One comment on using `mark_safe` on input that we do not control directly, which I don't have enough k8s knowledge to judge (should be safe?), ready to merge from my POV.
src/servala/core/models/service.py Outdated
@ -606,0 +627,4 @@
error_list = "".join(f"<li>{error}</li>" for error in errors)
# Replace the bracketed section with the formatted list
formatted_message = re.sub(pattern, f"<ul>{error_list}</ul>", error_message)
return mark_safe(formatted_message)
rixx commented 2025-07-11 10:06:56 +00:00
Member
Copy link

Using mark_safe means that we trust k8s error messages to never contain valid markup/other injections, e.g. by way of user-defined names. I don't know enough about how k8s errors look and are processed, just wanted to point that out.

If we can't rely on that, an alternative would be to use a django template here, which would apply the standard Django safeguards to all rendered variables

Using mark_safe means that we trust k8s error messages to never contain valid markup/other injections, e.g. by way of user-defined names. I don't know enough about how k8s errors look and are processed, just wanted to point that out. If we can't rely on that, an alternative would be to use a django template here, which would apply the standard Django safeguards to all rendered variables
tobru commented 2025-07-11 12:44:42 +00:00
Author
Owner
Copy link

With the help of Copilot and some manual tweaking / reviewing I was able to introduce a way to sanitize the error messages, which certainly makes sense. I hope it's not overengineered now.

With the help of Copilot and some manual tweaking / reviewing I was able to introduce a way to sanitize the error messages, which certainly makes sense. I hope it's not overengineered now.
tobru marked this conversation as resolved
tobru added 1 commit 2025-07-11 12:43:40 +00:00
sanitize kubernetes messages
All checks were successful
Tests / test (push) Successful in 28s
Details
8a45a22759
tobru added 1 commit 2025-07-11 12:45:33 +00:00
remove unused def
All checks were successful
Tests / test (push) Successful in 24s
Details
b5d691e407
rixx reviewed 2025-07-11 14:29:16 +00:00
rixx left a comment
Member
Copy link

Looks good to me!

Looks good to me!
tobru merged commit da2a1f6c64 into main 2025-07-11 14:54:01 +00:00
tobru deleted branch error-with-help 2025-07-11 14:54:01 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
dependencies

Dependencies Update

  
bug

Something is not working

  
change

Change an existing feature

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
question

More information is needed

  
wontfix

This won't be fixed

No labels
dependencies
bug
change
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
rixx
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: servala/servala-portal#149
No description provided.
Delete branch "error-with-help"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?