Add destinations page - #205
Conversation
|
Lookswise: Having "Label", "Media", "Value" inside the same box as the actual changeable value looks terrible. I think the login form has the value on the outside. There should be the rounded box around each if we want to have boxes, again see existing code. I'm not sure the dropdowns are needed at all. We should not expect a zillion destinations per user. The grey behind each box has to go. Too low contrast. The "media" input in each box is redundant. I think a header would look nicer than having one box for each type. |
|
Functions wise: make it work without HTMx first. Django has something called "formsets" you can use, see the abandoned code in #109 (which tries to do too much at once as well). |
|
DELETE and PATCH is API-stuff and htmx-stuff, not plain HTML. Make it work with plain HTML first. |
hmpf
left a comment
hmpf
left a comment
There was a problem hiding this comment.
I think this is a good candidate for class based views actually. We should have a meeting.
c293c0e to
dc49e30
Compare
hmpf
left a comment
hmpf
left a comment
There was a problem hiding this comment.
I've tested it and it works except it's a bit confusing that when I edit/delete the url changes. Maybe redirect to "/destination/" after change? @podliashanyk might have a better idea.
Almost only renames following.
| ) | ||
|
|
||
|
|
||
| def _get_settings_key_for_media(media: Media) -> str: |
There was a problem hiding this comment.
NTS: should be in the destination plugin machinery.
There was a problem hiding this comment.
As is, assumes that there is only one setting a user can set per plugin, which currently holds but might not in the future.
There was a problem hiding this comment.
Yes, there should be a very clear way to ask the NotificationMedium where the email address or phone number etc is. Currently its a bit of a guess since there could be multiple required properties
| def _init_serializer(self): | ||
| serializer = RequestDestinationConfigSerializer( | ||
| data={ | ||
| "media": self.cleaned_data["media"], | ||
| "label": self.cleaned_data.get("label", ""), | ||
| "settings": self.cleaned_data["settings"], | ||
| }, | ||
| context={"request": self.request}, | ||
| ) | ||
| self.serializer = serializer | ||
|
|
||
| def _validate_serializer(self): | ||
| media = self.cleaned_data["media"] | ||
| settings_key = _get_settings_key_for_media(media) | ||
|
|
||
| # Add error messages from serializer to form | ||
| if not self.serializer.is_valid(): | ||
| for error_name, error_detail in self.serializer.errors.items(): | ||
| if error_name in ["media", "label", settings_key]: | ||
| if error_name == settings_key: | ||
| error_name = "settings" | ||
| self.add_error(error_name, error_detail) | ||
| # Serializer might add more data to the JSON dict | ||
| if settings := self.serializer.data.get("settings"): | ||
| self.cleaned_data["settings"] = settings | ||
| else: | ||
| # Serializer might add more data to the JSON dict | ||
| if settings := self.serializer.validated_data.get("settings"): | ||
| self.cleaned_data["settings"] = settings | ||
|
|
||
| if label := self.cleaned_data["label"]: | ||
| filter = DestinationConfig.objects.filter(label=label) | ||
| if self.instance: | ||
| filter = filter.exclude(pk=self.instance.pk) | ||
| if filter.exists(): | ||
| self.add_error("label", "Name must be unique per media") |
There was a problem hiding this comment.
Hopefully all of this will go away when the plugins are changed.
|
Ehh, and I almost forgot: media-field of the creation-form is bugged in "argus"-theme if the OS/browser is set to dark-mode:
Not a show-stopper but we ought to figure out how to fix it. |
Pretty much copied from the destinations API, needs some changes so it wont just crash to 500 when it raises a ValidationError This commit doesnt make it show in the frontend, the button for deleting a destination will be grouped with the forms for updating destinations
Seems like a lot of changes .. but this is what the wailwindcss script generated
I imagine HTMXifying this properly should fix this. Instead of loading the entire page when deleting, only the element containing the update/delete forms is updated etc. Without HTMX: redirect works fine for deleting and updating when there is no error message, but when theres an error message you need the context from the delete/update view to show the error messages, in this case redirect feels bad |
|
Moved to argus-server |
2 participants
Fixes Uninett/Argus#1001
Replaced by Uninett/Argus#970
Adds page where you can see and edit existing destinations and create new ones.
Since we are kinda locked to using the serializer for validating destinations (for now, changes will be made at some point) I ended up baking the serializer into the forms for creating/updating destinations. Its a bit wonky but should work fine until we can move on from the serializer and have
ModelFormdo everything they way it is supposed to.Needs more styling to be complete, but not sure how to do it properly.. Error in fields are now added to the right of the field the error belongs to, so the buttons and everything gets moved to the right when theres an error.