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

Button of choosing reply always expires #323

Closed
MrOrz opened this issue Sep 14, 2022 · 2 comments · Fixed by #324
Closed

Button of choosing reply always expires #323

MrOrz opened this issue Sep 14, 2022 · 2 comments · Fixed by #324
Labels
bug help wanted P0 High priority issue / bug

Comments

@MrOrz
Copy link
Member

MrOrz commented Sep 14, 2022

Steps to reproduce

  1. Send the following text:

    蔡英文的堂弟 蔡世能 :在桃園成立大日本帝國,希望“重建日本政府”

    若是沒有蔡英文的認同與支持,他敢這樣幹!?

  2. See the reply list, choose any reply

Expected

User should be able to read reply after choosing reply

Actual

"Button has been expired" error is shown after choosing reply

image

@MrOrz MrOrz added bug help wanted P0 High priority issue / bug labels Sep 14, 2022
@MrOrz
Copy link
Member Author

MrOrz commented Sep 15, 2022

Seems that context is empty after providing text that has 100% match.

image

It is supposed to have at least a data attribute and a new sessionId. No data in context will trigger this logic, responding that the button is expired.

Note that the action buttons do have sessionId in its postback data, so data is passed to choosingArticle handler correctly. Then what happens when choosingArticle returns its data?

Still needs further investigation.

@MrOrz
Copy link
Member Author

MrOrz commented Sep 15, 2022

Root cause

handlePostback, a function that returns {context: data}, is used in handlers that should return {data}.

  1. handleInput, handlePostback and processImage are top-level functions that returns {context, replies} for singleUserHandler to set redis context and submit replies.
  2. These functions calls handlers such as initState and choosingArticle, who returns data in {data, replies, ...} format.
  3. However, in initState and choosingArticle, in some scenarios we may return results from handlePostback.
    • The caller of initState expects a result in the form {data, replies, ...} but in this case it returns {context, replies}
    • The caller tries to grab data (which is undefined in this case) and put in context, thus the context is dropped.
  4. After context is dropped, any postback action will trigger this logic, rendering the "button is expired" error.
  5. In the unit test, we mock the return result of handlePostback all together, so that we cannot spot this issue.

Suggested fix

  1. In the context of handlers such as initState and choosingArticle, instead of calling handlePostback({data}, NEXT_ACTION), we should call the handler for the NEXT_ACTION and return its result. This ensures that the signature stays the same.
  2. We do not mock the called handler in unit test so that we can inspect the response of the called handler. Although the snapshot may overlap and changes to the called handler may incur multiple snapshot changes, we can at least see if the full response is as expected.

Impact

  • handlePostback({ data }, 'CHOOSING_ARTICLE', event, userId) In initState under condition edgesSortedWithSimilarity.length === 1 && hasIdenticalDocs
    • --> If there is only 1 identical text, the resulting buttons have no context and always yields "button is expired" error
    • Example: the example in the ticket description
  • handlePostback({ data }, 'CHOOSING_REPLY', event, userId) in choosingArticle under condition articleReplies.length === 1
    • --> if there is only 1 reply, the "current context" will be dropped. If user chooses another article in the same search session, the LINE bot will respond with "expired button" error
    • Example:
      image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug help wanted P0 High priority issue / bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant