[verisure] Adapted to new authentication process and support for non MFA activated user #11265

jannegpriv · 2021-09-18T20:32:50Z

[verisure] Adapted to new authentication process and support for non MFA activated user. (#11228)

Signed-off-by: Jan Gustafsson jannegpriv@gmail.com

bundles/org.openhab.binding.verisure/README.md

...ab.binding.verisure/src/main/java/org/openhab/binding/verisure/internal/VerisureSession.java

...isure/src/main/java/org/openhab/binding/verisure/internal/handler/VerisureBridgeHandler.java

...ab.binding.verisure/src/main/java/org/openhab/binding/verisure/internal/VerisureSession.java

…MFA activated user. (openhab#11228) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>

lolodomo · 2021-10-09T09:28:27Z

This is better to push an update rather than the full PR because now the reviewer has to do again a full review! Lost of time for the reviewer and more time before your work is merged.

jannegpriv · 2021-10-09T10:08:57Z

Sorry, can you describe exactly what I have done wrong so that I will get it right next time? I've always used the command:

git push --force-with-lease

when pushing changes.

lolodomo

Please only oush an update containing the changes you will do to handle my comments.

...ab.binding.verisure/src/main/java/org/openhab/binding/verisure/internal/VerisureSession.java

...in/java/org/openhab/binding/verisure/internal/handler/VerisureClimateDeviceThingHandler.java

...ab.binding.verisure/src/main/java/org/openhab/binding/verisure/internal/VerisureSession.java

lolodomo · 2021-10-09T10:24:27Z

Sorry, can you describe exactly what I have done wrong so that I will get it right next time?

Commit your new changes and just run a git push without forcing anything. Something like that:

git commit
git push origin 11228-verisure

jannegpriv · 2021-10-09T15:03:36Z

Sorry, can you describe exactly what I have done wrong so that I will get it right next time?

Commit your new changes and just run a git push without forcing anything. Something like that:
git commit
git push origin 11228-verisure

OK, I actually was instructed by another maintainer to use -force-with-lease, but then i will not use force anymore.

jannegpriv · 2021-10-09T15:28:20Z

The instructions here uses git push --force-with-lease.

lolodomo · 2021-10-09T15:44:52Z

There are situations where forcing push is adapted or even required but during the review process it is clearly not recommended for the reason I previously explained, that is it forces all reviewers to restart a full review of your PR. I did it in your case because your PR was not too large but I may have forgiven for a big PR.

Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>

lolodomo

This is almost ok for me.
I only discovered something that you should improve, even if not something directly related to this PR.

...erisure/src/main/java/org/openhab/binding/verisure/internal/VerisureBridgeConfiguration.java

...isure/src/main/java/org/openhab/binding/verisure/internal/handler/VerisureBridgeHandler.java

Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>

lolodomo

LGTM

lolodomo · 2021-10-14T05:52:58Z

Build failed due to timeout.

jannegpriv · 2021-10-16T07:21:44Z

@lolodomo Thanks for the review. :-)
I got a question regarding catching RunTimeException in VerisureBridgeHandler.java on line 131.
Looking at the code, should the try catch be moved to after the scheduler.execute to be able to catch it if it is thrown by VerisureSession.initialize()?

lolodomo · 2021-10-16T07:51:20Z

I got a question regarding catching RunTimeException in VerisureBridgeHandler.java on line 131.
Looking at the code, should the try catch be moved to after the scheduler.execute to be able to catch it if it is thrown by VerisureSession.initialize()?

Oh yes, you're right, the try/catch is not at the right level.

jannegpriv · 2021-10-16T08:30:13Z

I got a question regarding catching RunTimeException in VerisureBridgeHandler.java on line 131.
Looking at the code, should the try catch be moved to after the scheduler.execute to be able to catch it if it is thrown by VerisureSession.initialize()?

Oh yes, you're right, the try/catch is not at the right level.

Should I create a new PR to fix that?
I would be nice to get this fixed to the upcoming 3.2.0 release.

lolodomo · 2021-10-16T09:32:10Z

Should I create a new PR to fix that?

Yes, please.
I merged this PR to not delay your enhancement and because the build was successful (which is something difficult to get since few days).

…MFA activated user. (openhab#11228) (openhab#11265) * [verisure] Adapted to new authentication process and support for non MFA activated user. (openhab#11228) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>

…MFA activated user. (openhab#11228) (openhab#11265) * [verisure] Adapted to new authentication process and support for non MFA activated user. (openhab#11228) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> Signed-off-by: Dave J Schoepel <dave@theschoepels.com>

…MFA activated user. (openhab#11228) (openhab#11265) * [verisure] Adapted to new authentication process and support for non MFA activated user. (openhab#11228) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> Signed-off-by: Nick Waterton <n.waterton@outlook.com>

…MFA activated user. (openhab#11228) (openhab#11265) * [verisure] Adapted to new authentication process and support for non MFA activated user. (openhab#11228) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>

Skinah added the enhancement An enhancement or new feature for an existing add-on label Sep 30, 2021

lolodomo requested changes Oct 2, 2021

View reviewed changes

[verisure] Adapted to new authentication process and support for non …

65e5b7d

…MFA activated user. (openhab#11228) Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>

jannegpriv force-pushed the 11228-verisure branch from 3194ac6 to 65e5b7d Compare October 6, 2021 20:42

lolodomo requested changes Oct 9, 2021

View reviewed changes

Updated after code review.

78555b2

Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>

lolodomo requested changes Oct 12, 2021

View reviewed changes

jannegpriv added 3 commits October 13, 2021 22:05

Updated after code review.

64d67e3

Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>

Updated after code review.

0a972a2

Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>

Updated after code review.

4dd084f

Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>

lolodomo approved these changes Oct 13, 2021

View reviewed changes

lolodomo added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Oct 14, 2021

lolodomo merged commit 18d26aa into openhab:main Oct 16, 2021

lolodomo added this to the 3.2 milestone Oct 16, 2021

wborn changed the title ~~[verisure] Adapted to new authentication process and support for non MFA activated user. (#11228)~~ [verisure] Adapted to new authentication process and support for non MFA activated user Dec 18, 2021

wborn mentioned this pull request Dec 18, 2021

[verisure] Support for non MFA activated user #11228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[verisure] Adapted to new authentication process and support for non MFA activated user #11265

[verisure] Adapted to new authentication process and support for non MFA activated user #11265

jannegpriv commented Sep 18, 2021

lolodomo commented Oct 9, 2021

jannegpriv commented Oct 9, 2021

lolodomo left a comment

lolodomo commented Oct 9, 2021 •

edited

Loading

jannegpriv commented Oct 9, 2021

jannegpriv commented Oct 9, 2021

lolodomo commented Oct 9, 2021

lolodomo left a comment •

edited

Loading

lolodomo left a comment

lolodomo commented Oct 14, 2021

jannegpriv commented Oct 16, 2021

lolodomo commented Oct 16, 2021

jannegpriv commented Oct 16, 2021

lolodomo commented Oct 16, 2021

[verisure] Adapted to new authentication process and support for non MFA activated user #11265

[verisure] Adapted to new authentication process and support for non MFA activated user #11265

Conversation

jannegpriv commented Sep 18, 2021

lolodomo commented Oct 9, 2021

jannegpriv commented Oct 9, 2021

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo commented Oct 9, 2021 • edited Loading

jannegpriv commented Oct 9, 2021

jannegpriv commented Oct 9, 2021

lolodomo commented Oct 9, 2021

lolodomo left a comment • edited Loading

Choose a reason for hiding this comment

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo commented Oct 14, 2021

jannegpriv commented Oct 16, 2021

lolodomo commented Oct 16, 2021

jannegpriv commented Oct 16, 2021

lolodomo commented Oct 16, 2021

lolodomo commented Oct 9, 2021 •

edited

Loading

lolodomo left a comment •

edited

Loading