keatontaylor / alexa-actions

A README and associated code to get actionable notifications setup for Alexa devices.
GNU General Public License v3.0
399 stars 187 forks source link

147 get rid of okay confirmation #148

Closed DEADSEC-SECURITY closed 1 year ago

DEADSEC-SECURITY commented 1 year ago

This PR adds the possibility to suppress the Okay response Alexa has when you respond to her question allowing you to send a next question or do a custom response.

elmar-hinz commented 1 year ago

The following is just an annotation and nothing to work on top of this already large pull request:

Screenshot 2023-01-07 at 07 25 24

It looks semantically strange to me, first to speak out and then to add an empty question instead of just asking a question or speaking a text and then asking a question.

It may be a design flow of the ASK python api, though. I didn't go deeper into this. If the ASK api is well done, I would expect a special accentuation for questions given directly to ask("Do you really want me to do that?").

In this case it may even be reasonable to differ text and question into two variables.

.speak(text)
.ask(question)

Something in this direction.

       self.ha_state = {
            "error": False,
            "event_id": response.get('event'),
            "suppress_confirmation": _string_bool_to_bool(response.get('suppress_confirmation')),
            "text": response.get('text'),
            "question": response.get('question') if response.get('question') else "",
        }

(Empty question allowed for backwards compatibility.)

DEADSEC-SECURITY commented 1 year ago

I noticed that too a while ago when i took on as a maintainer but never touched that too.

DEADSEC-SECURITY commented 1 year ago

Maybe we can make a new issue and analyze this more carefully. @elmar-hinz

DEADSEC-SECURITY commented 1 year ago

Besides that did you manage to test the okay confirmation suppression?

elmar-hinz commented 1 year ago

See #147

elmar-hinz commented 1 year ago

@DEADSEC-SECURITY The first error is caused here

AttributeError: 'NoneType' object has no attribute 'lower'

Screenshot 2023-01-11 at 11 45 13

The value of suppress_confirmation is not given in all cases.

Screenshot 2023-01-11 at 11 45 39

elmar-hinz commented 1 year ago

Note: the method _string_bool_to_bool does not know if a missing value defaults to true or false. I think the most elegant fix, is to provide a second parameter naming the default.

DEADSEC-SECURITY commented 1 year ago

Correct, in my tests I was taking it a always pass. Good find.

DEADSEC-SECURITY commented 1 year ago

Note: the method _string_bool_to_bool does not know if a missing value defaults to true or false. I think the most elegant fix, is to provide a second parameter naming the default.

Fair enough, I agree with that approach. Let me fix the bug really quick

DEADSEC-SECURITY commented 1 year ago

New commit added which should fix the bug in question and also add a default for later usage if required. By default the default values is False

DEADSEC-SECURITY commented 1 year ago

@elmar-hinz

elmar-hinz commented 1 year ago

@DEADSEC-SECURITY

OK: tested for three cases: true, false, void

DEADSEC-SECURITY commented 1 year ago

Cool. Are you able to use the supression properly now? Can we move on with merging this?

elmar-hinz commented 1 year ago

go

rwunsch commented 1 year ago

Hi @elmar-hinz , Hi @DEADSEC-SECURITY ,

shouldn't the alexa-actions script on HA side also be updated? I think we need to add the "suppress_confirmation"-parameter there as well, to be able to use it. Right?

I am talking about the "script.yaml": `activate_alexa_actionable_notification: alias: activate_alexa_actionable_notification description: Activates an actionable notification on a specific echo device fields: text: description: The text you would like alexa to speak. event_id: description: Correlation ID for event responses alexa_device: description: Alexa device you want to trigger suppress_confirmation: description: Set true if you want to suppress 'okay' confirmation sequence:

Or what would be the way to use this? I could not find it in the documentation - and attempting to reverse engineer what you did lead me to this above.... Is that the intention?

DEADSEC-SECURITY commented 1 year ago

Hi @elmar-hinz , Hi @DEADSEC-SECURITY ,

shouldn't the alexa-actions script on HA side also be updated? I think we need to add the "suppress_confirmation"-parameter there as well, to be able to use it. Right?

I am talking about the "script.yaml": `activate_alexa_actionable_notification: alias: activate_alexa_actionable_notification description: Activates an actionable notification on a specific echo device fields: text: description: The text you would like alexa to speak. event_id: description: Correlation ID for event responses alexa_device: description: Alexa device you want to trigger suppress_confirmation: description: Set true if you want to suppress 'okay' confirmation sequence:

  • service: input_text.set_value data_template: entity_id: input_text.alexa_actionable_notification value: '{"text": "{{ text }}", "event": "{{ event_id }}", "suppress_confirmation": "{{ suppress_confirmation }}"}'
  • service: media_player.play_media data_template: entity_id: "{{ alexa_device }}" media_content_type: skill media_content_id: amzn1.ask.skill. mode: single`

Or what would be the way to use this? I could not find it in the documentation - and attempting to reverse engineer what you did lead me to this above.... Is that the intention?

You need to update to version 0.9.1 ( i think this already had that update) and then update the script to send that parameter

rwunsch commented 1 year ago

Hi @DEADSEC-SECURITY ,

I have already updated to 0.9.1 (which works well, and the StringIntent is very, very cool!). I also have the "suppress_confirmation", and it works well - ONCE I have "updated" the script (reverse engineered where and what to change, for the lamda-function to work with HA-script on "suppress_confirmation". "suppress_confirmation" is also very, very helpful, as I am now using this repo to have an ongoing conversation with ChatGPT, and the "Okay" after each response was a bit annoying. This is gone now.

I was wondering if the HA-Script should be in the repository. I would favor to have the script in HA to be checked into the repo as well, with a version which matches the functionalities of the lambda-function/SKILL version.

I think right now, the HA-script for "alexa-actions" is only available somewhere in the documentation.

DEADSEC-SECURITY commented 1 year ago

Hi @DEADSEC-SECURITY ,

I have already updated to 0.9.1 (which works well, and the StringIntent is very, very cool!). I also have the "suppress_confirmation", and it works well - ONCE I have "updated" the script (reverse engineered where and what to change, for the lamda-function to work with HA-script on "suppress_confirmation". "suppress_confirmation" is also very, very helpful, as I am now using this repo to have an ongoing conversation with ChatGPT, and the "Okay" after each response was a bit annoying. This is gone now.

I was wondering if the HA-Script should be in the repository. I would favor to have the script in HA to be checked into the repo as well, with a version which matches the functionalities of the lambda-function/SKILL version.

I think right now, the HA-script for "alexa-actions" is only available somewhere in the documentation.

Isnt the supress ok notification in docs?

DEADSEC-SECURITY commented 1 year ago

Hi @DEADSEC-SECURITY ,

I have already updated to 0.9.1 (which works well, and the StringIntent is very, very cool!). I also have the "suppress_confirmation", and it works well - ONCE I have "updated" the script (reverse engineered where and what to change, for the lamda-function to work with HA-script on "suppress_confirmation". "suppress_confirmation" is also very, very helpful, as I am now using this repo to have an ongoing conversation with ChatGPT, and the "Okay" after each response was a bit annoying. This is gone now.

I was wondering if the HA-Script should be in the repository. I would favor to have the script in HA to be checked into the repo as well, with a version which matches the functionalities of the lambda-function/SKILL version.

I think right now, the HA-script for "alexa-actions" is only available somewhere in the documentation.

https://github.com/keatontaylor/alexa-actions/discussions/163

DEADSEC-SECURITY commented 1 year ago

Added to docs: https://github.com/keatontaylor/alexa-actions/wiki/Initial-Home-Assistant-Configuration

rwunsch commented 1 year ago

Thanks @DEADSEC-SECURITY ! I took the liberty and also added the information/script-template here: https://github.com/keatontaylor/alexa-actions/wiki/Home-Assistant-Talking-to-Alexa-(The-Script)