search

SUSE / velum

Dashboard for CaaS Platform clusters (v1, v2 and v3)
https://www.suse.com/
Apache License 2.0
54 stars 30 forks source link

Prevent changed external ldap values from skipping checks #652

Closed nanoscopic closed 5 years ago

nanoscopic commented 5 years ago

Alter external ldap auth form to redisable save and rerequire 'check connection' when values are changed.

vitoravelino commented 5 years ago

From what I could investigate so far, the usage trigger('change') was necessary because of execute_script. So, instead of using that we can try to use the built-in matchers to avoid unexpected results and forcing us to manually trigger an event.

nanoscopic commented 5 years ago

It is a well known issue that you have to trigger changes using jquery for Capybara to properly trigger changes. Fill in does not do it. I tested this. Stop wasting time and approve this.

vitoravelino commented 5 years ago

I understand the urgency of getting this merged to unleash the LDAP feature, I understand the lack of convention rule and all the other minor things. I'll start a discussion around having these conventions to lower the contribution bar.

In the meantime, I tried to perform the remaining changes I proposed and I got the tests to pass. Here's the link to the PR with my changes. Could you test it locally to see if they are not passing on your machine?

I'm bringing this up because I'm concerned we are making things less reliable by exaggerating on the usage of js things inside of capybara tests and lack of proper external services mocking in order to have tests green.

nanoscopic commented 5 years ago

I have taken a look at the suggested changes. As stated there they look good. I am still confused by adding id to the labels instead of the fields. That is the magic stuff I am referring to that I don't like. What you've done in that PR is what I tried before doing the more complicated way; I wouldn't go out of my way to do it as I did if the simple method had worked from the start.

My point is still that both ways work and I don't think there is anything wrong with the JS method functionality wise. It is hardly concerning nor "making the tests green". I don't feel the response you've given overall is warranted.