search

neothematrix / noip-renew

Auto renew (confirm) noip.com free hosts
https://hub.docker.com/r/moebiuss/noip-renew
Apache License 2.0
54 stars 20 forks source link

Concerns about Merging #48

Closed Angel0ffDeath closed 8 months ago

Angel0ffDeath commented 8 months ago

@neothematrix Thank you for merging everything and issuing a new version. However I have some concerns which I cannot test right now:

  1. You moved this line according to my recommendations: host_button = self.get_host_button(host, iteration) # This is the button to confirm our free host

  2. Meanwhile you keep line 183 as: return host.find_element(By.XPATH, ".//following-sibling::td[4]/button[contains(@class, 'btn')]")

My recommendation about item 2 above in case you fulfil item 1 was: return host.find_element(By.XPATH, "//td[6]/button[contains(@class, 'btn-success')]")

If you don't move button search (item 1 above) then you should keep my other proposal: return host.find_element(By.XPATH, "//following-sibling::td[6]/button[contains(@class, 'btn')]")

I don't think keeping ".//following-sibling::td[4]/button[contains(@class, 'btn')]" is correct since exactly that was the error I received. Are you sure current code works correct in both cases - with confirmed host (should work because we don't search for confirm button) and without confirmed host (according to me should not work since we are searching with old xpath pattern). Can't test the last...

Please check again.

neothematrix commented 8 months ago

ciao @Angel0ffDeath , you're totally right, it was a messy cut&paste where I actually pasted the wrong line after testing, I'll fix it very soon and release a new version. thanks for catching that!

neothematrix commented 8 months ago

fixed by 88cfe44c2b0524aaed20790bee0d3a809c16bb08

again thanks @Angel0ffDeath !

Angel0ffDeath commented 8 months ago

@neothematrix Glad to help. Welcome. Next time will make PR. This time I didnt, because was not sure with which file I should work.... You didn't accepted my last PR's more than an year.... but no problems. Now we are clear and next time there will e PR. Thank you.