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

Simplify state, return state only when we need it #219

Merged
merged 3 commits into from Sep 23, 2020
Merged

Conversation

nonumpa
Copy link
Member

@nonumpa nonumpa commented Sep 16, 2020

Fixes #177

  1. Remove useless state assignment and return state only when we need it

    • ASKING_REPLY_FEEDBACK state is replaced by input.startsWith(UPVOTE_PREFIX or DOWNVOTE_PREFIX).
    • In [handleInput], context.state is mainly used for postback message. But now it's also used to distinguish state ASKING_ARTICLE_SUBMISSION_CONSENT and ASKING_REPLY_REQUEST_REASON from input.startsWith(SOURCE_PREFIX). Maybe we can use different prefixes for them in the future.
  2. Move reason message handler from [initState] to [askingReplyRequestReason]

    • Merge reason message(original in initState) to ASKING_REPLY_REQUEST_REASON.
  3. I don't update test in this PR, so we can see if there's fatal test fail after this refactoring.
    Will fix test in another PR.

    • In [initState.test], all handles reason LIFF should test in [askingReplyRequestReason.test].
    • In [handleInput.test], the test processes first article reason submission is wrong, REASON_PREFIX was processed in [initState] not [askingArticleSubmissionConsent].
    • In [askingReplyRequestReason.test], because we merged reason message handler into [askingReplyRequestReason], the test should block incorrect prefix shouldn't input REASON_PREFIX now.

@nonumpa nonumpa requested a review from MrOrz September 16, 2020 13:03
Copy link
Member

@MrOrz MrOrz left a comment

Choose a reason for hiding this comment

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

The code change looks reasonable and it works on my local machine / test accounts.

Thanks a million for simplifying the code base! Looking forward to the fix of test cases.

@nonumpa nonumpa mentioned this pull request Sep 21, 2020
@MrOrz MrOrz merged commit 67a5245 into dev Sep 23, 2020
@MrOrz MrOrz deleted the simplify-state branch September 23, 2020 07:19
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.

Simplify __INIT__ handler by disconnecting inbound actions
2 participants