Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ignore echo messages from facebook #10097

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aristidednd
Copy link

According to Facebook Messenger API documentation https://developers.facebook.com/docs/messenger-platform/reference/webhook-events/message-echoes/?locale=en_En, an echo message is also triggered for image, video, file, and audio attachments.
These changes ignore those echo messages.

Proposed changes:

  • Ignore echo messages related to file, audio, video, and image attachments from Facebook

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

According to messenger API documentation https://developers.facebook.com/docs/messenger-platform/reference/webhook-events/message-echoes/?locale=en_En, an echo message is also triggered for image, video, file, and audio attachments. 
These changes ignore those echo messages.
@aristidednd aristidednd requested a review from a team as a code owner November 7, 2021 14:23
@aristidednd aristidednd requested review from usc-m and removed request for a team November 7, 2021 14:23
@CLAassistant
Copy link

CLAassistant commented Nov 7, 2021

CLA assistant check
All committers have signed the CLA.

@aristidednd aristidednd changed the title unhandle echo messages from messenger ignore echo messages from facebook Nov 9, 2021
@stale
Copy link

stale bot commented Apr 16, 2022

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 16, 2022
@ancalita ancalita removed the request for review from usc-m May 18, 2022 13:15
@stale stale bot removed the stale label May 18, 2022
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aristidednd Thank you for your contribution! 💯
Could you please add a changelog entry specifying the why and how of your improvement.
Also if possible please add unit test coverage.

@@ -45,6 +45,7 @@ def _is_audio_message(message: Dict[Text, Any]) -> bool:
"message" in message
and "attachments" in message["message"]
and message["message"]["attachments"][0]["type"] == "audio"
and not message["message"].get("is_echo")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding this in the different _is_<type>_message checks leads to these methods having more than 1 purpose. I suggest instead extracting this boolean variable in a separate function _is_echo_message and then adding this check once only at the top of the message method before all the other checks:

    async def message(
        self, message: Dict[Text, Any], metadata: Optional[Dict[Text, Any]]
    ) -> None:
        """Handle an incoming event from the fb webhook."""
        if self._is_echo_message(message):
                logger.warning("Received a message echo from Facebook which will be ignored.")
                return None
        # quick reply and user message both share 'text' attribute
        # so quick reply should be checked first
        if self._is_quick_reply_message(message):
              ...

@Tung8787
Copy link

Tui chả hiểu hì :)))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants