Use forms in destination page #1161
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
hmpf
force-pushed
the
destination-with-forms
branch
6 times, most recently
from
a51d4e7 to
b1c02c5
Compare
hmpf
force-pushed
the
destination-with-forms
branch
from
b1c02c5 to
b4c8a33
Compare
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1161 +/- ##
==========================================
- Coverage 77.36% 77.07% -0.29%
==========================================
Files 141 140 -1
Lines 5548 5675 +127
==========================================
+ Hits 4292 4374 +82
- Misses 1256 1301 +45 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Some issues while testing this:
Trying to update a destination gives the error "This email destination already exists." This happens if you make no changes to the form and click "update", or only change either label or the setting. But if you change both to unique values that are not used in another destination, then it updates. It seems like the validation doesn't understand that the updated destination will replace the original, and just treats it like you're trying to create a new destination with the same label and or setting.
Simultaneously, a new collapse elements is created that is empty with the title (0)
Another problem (relevant comment here is when you try to create a new destination (or update) with a label or email that collides with an existing destination, you get the error message "This email destination already exists", but it does not indicate in any way if the problem is the label, the email or both
hmpf
added
the
blocked
| {% with update=label_form.instance.pk %} | ||
| {% csrf_token %} | ||
| <fieldset class="p-2 border rounded-box border{% if not update %}-primary{% endif %} items-center gap-4 flex flex-col items-end justify-center"> | ||
| {% if not label_form.instance.pk %}<legend class="menu-title">Create destination</legend>{% endif %} |
There was a problem hiding this comment.
| {% if not label_form.instance.pk %}<legend class="menu-title">Create destination</legend>{% endif %} | |
| {% if not update %}<legend class="menu-title">Create destination</legend>{% endif %} |
There was a problem hiding this comment.
"update" does not exist in the context.
There was a problem hiding this comment.
Oh, why is that? I thought it is available everywhere that is framed by the {% with update=label_form.instance.pk %}
There was a problem hiding this comment.
I was thinking context sent by the view. Fooled by github diff view again.
.. also make it easioer to validate the settings-field with a django form.
hmpf
force-pushed
the
destination-with-forms
branch
from
bf5af7b to
58fe34d
Compare
hmpf
force-pushed
the
destination-with-forms
branch
from
c7ac6be to
014c962
Compare
Co-authored-by: Johanna England <[email protected]>
|
|
Immediate problem I noticed here: If you try to create a new destination that should fail (has same label or email has a pre-existing destination), it does not give an error message. The form is emptied and a success message appears, like it does when you actually succesfulyl create a destination, but no new destination is added. Edit: If both fields are indentical to a pre existing email it gives the success message, if only the label is duplicate but email is new, it gives an error |
Depends on #936
Review file by file
Will be squashed