Conversation
styrix560
force-pushed
the
2129-update-contact-modal
branch
from
41f249c to
013b600
Compare
styrix560
force-pushed
the
2129-update-contact-modal
branch
from
e262c2f to
b945b30
Compare
niklasmohrin
left a comment
niklasmohrin
left a comment
There was a problem hiding this comment.
I think the end goal should be to not use django include for modals at all; this is definitely a step in the right direction.
| self.wait.until(visibility_of_element_located((By.ID, "feedbackModalMessageText"))) | ||
| self.selenium.find_element(By.ID, "feedbackModalMessageText").send_keys("Test message") | ||
| self.selenium.find_element(By.ID, "feedbackModalActionButton").click() | ||
| self.selenium.find_element(By.CSS_SELECTOR, "span[slot='action-text']").click() |
There was a problem hiding this comment.
is there any selector that ensures that we click on the contact modal?
| self.evaluation.save() | ||
| page = self.app.get(self.url, user=self.responsible, status=200) | ||
|
|
||
| self.assertIn("changeEvaluationRequestModalLabel", page) |
There was a problem hiding this comment.
Before, this was the code <h5 class="modal-title" id="{{ modal_id }}Label">{{ title }}</h5>. Now, the id also reflects, that this is the title and not some kind of label: <span slot="title" id="{{ modal_id }}Title">{{ title }}</span>
There was a problem hiding this comment.
but the "change evaluation" part changed too, I think this is the more interesting change
There was a problem hiding this comment.
I think I know why this is happening and the reason is a bit hilarious.
Our test was (kinda) supposed to fail always. Before, we included the modals unconditionally and only showed the show-buttons conditionally. Now, we also show modals conditionally. Where before both modals lay in the DOM, now only the createContributorAccountRequestModal appears.
| </div> | ||
| </div> | ||
| </div> | ||
| <form reload-on-success id="contact-modal-form-{{ modal_id }}" method="POST" action="{% url 'evaluation:contact' %}"> |
There was a problem hiding this comment.
Looks like this ID is now unused -- can we remove it?
There was a problem hiding this comment.
No, it is required for the test test_contact_modal_escape in contributor:test_views
There was a problem hiding this comment.
It does not use contact-modal-form-...?
| </div> | ||
| <form reload-on-success id="contact-modal-form-{{ modal_id }}" method="POST" action="{% url 'evaluation:contact' %}"> | ||
| {% csrf_token %} | ||
| <input hidden name="title" value="{{ title }}"/> |
There was a problem hiding this comment.
Before, we passed the title to a hidden input with id="{{ modal_id }}Subject" and no name. We still have the corresponding input below, but now we also have name="title". Is this intended and required?
There was a problem hiding this comment.
Before, we had custom logic to fire the http request, that passed the title value with key 'title' as body parameters. Because we now use html forms for this, name="title" submits the title the same way as before.
The input you mention was only for UI and was never submitted, because it had no name.
There was a problem hiding this comment.
But then let's just use one input instead of having two of them? I'd propose to remove this one here and juts add name="title" to the one below in line 26
4 participants
Fixes #2129