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

Flow #657

Open
wants to merge 6 commits into
base: v3
Choose a base branch
from
Open

Flow #657

wants to merge 6 commits into from

Conversation

shindakioku
Copy link

@shindakioku shindakioku commented Feb 25, 2024

Let's discuss the flow process based on live examples.
The code may not be production-ready, of course, and without tests, but it should be sufficient for manual testing. Unfortunately, I haven't worked with the Telegram bot API for the last two years, so I may not be able to cover all cases. I'm asking for help from anyone who wants to contribute. Just pull my branch, try to describe your own flows, and provide feedback.

Anyone who wants to help can take a look at flow_simple_manual_testing/main.go, please. I've written two examples there, so you can test and modify as you want.

Updated: Please do not attempt to use my bot API token; I have already revoked it.

@shindakioku shindakioku mentioned this pull request Feb 25, 2024
@shindakioku
Copy link
Author

I will soon add some details for the "short" features.

@tucnak
Copy link
Owner

tucnak commented Feb 25, 2024

Hey, first of all, thanks for putting in the effort; state-machine behaviour is one aspect of the API that we had discussed time and time again with very little to show in terms of implementation. Well, I have built quite a few bots over the years, and indeed employed various approaches to state-management. Flow at the time was my best attempt at "generalising" that experience, yet unfortunately it coincided with big changes in my life, that is to say I haven't touched Telegram bots in a long time now.

My money would be on @demget to provide most accurate feedback as he's much more knowledgable than me, however I'll do my best to point out some things about your implemnetation along the way.

Copy link
Owner

@tucnak tucnak left a comment

Choose a reason for hiding this comment

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

First-pass

)

// Machine describes the contract for the flow handling
type Machine interface {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't see how this interface would correspond to actual FSM backend... ideally, we want some kind of interface that would be easy to implement, as it may vary based on whether if you want to use cache, what kind of it, if you want to use code-generation, and so on. Consider https://github.com/d5/go-fsm for example of how FSM may be pushed quite considerably based on application.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I've thought about that, but ultimately, my idea is very simple: you only need to call one function
for each step (that interacts with the user, I mean). But you have good thoughts regarding caching and so on.
With further consideration, we can explore alternative approaches:

flow.New().
  Step(sendUserMessage("Enter email:")).
  Step(validateEnteredEmail).
  Step(assignMailToVariable).
  Step(sendUserMessage("Enter password:")).

Based on the code above, we can define any steps we want. Each step should return an error, indicating that the user did not successfully pass the step, and we need to wait for the next prompt and rerun the step. It's a very simple interface to implement.
By the way, we have some questions to discuss:

  • Do we need to provide common handlers for the entire flow's completion? Such as fail/success handlers as implemented in my idea here.
  • Do we need to have an opportunity to mark steps as internal/external? Internal means that the step doesn't interact with the user (just simple logging, for example).
  • How can we solve the following problem: we want to log every step using only one function. For that, we need to provide an interface similar to the one I have now.

That's why I'm not sure if the ftm is still a good idea. But I agree that it is easier for the user and looks clearer.

Copy link
Author

Choose a reason for hiding this comment

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

I'm quite certain we need to address that functionality, even if the user can perform validation/logging, etc., within the step function. It's unclear, and I suspect no one would want to do it. Additionally, it doesn't resolve the issue with global handlers (such as fail/logging, etc.).

flow/machine.go Outdated
// ToStep Move to the step
ToStep(step int, state *State) error
// Success stop processing and call the final function
Success(state *State) error
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure what value Success/Fail determination would add beyond general-purpose state abstraction. My intuition would be to simply process fails using a custom error type that would encompass all the information necessary for propagation.

flow/state.go Outdated
type StateHandler func(state *State) error

// State defines the user's state and persists common elements, such as the bot instance, flow handler instance, etc.
type State struct {
Copy link
Owner

Choose a reason for hiding this comment

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

This feels a little backwards, I would much rather have State implement some kind of interface

@shindakioku
Copy link
Author

shindakioku commented Feb 25, 2024

I've completed some refactoring. Firstly, I updated the API, and now it looks like:

b := &tele.Bot{}
sendUserMessage := func(message string) func(flow.State) error {
	return func(state flow.State) error {
		return state.Read(flow.StateContextKey).(tele.Context).Reply(message)
	}
}
stepCompletedLogging := func(state flow.State, step *flow.Step) error {
	log.Println("Step completed")

	return nil
}
// Configure flow bus
flowBus := flow.NewBus(5 * time.Minute)
// Handle any text by flow bus
b.Handle(tele.OnText, flowBus.Handle)
// First flow
var email, password string
b.Handle("/start", flowBus.Flow(
	flow.New().
		Next(
			flow.NewStep(sendUserMessage("Enter email:")).
				Validate(nonEmptyValidator).
				Assign(TextAssigner(&email)).
				Then(stepCompletedLogging),
		).
		Next(
			flow.NewStep(sendUserMessage("Enter password:")).
				Assign(TextAssigner(&password)),
		).
		Then(func(state flow.State) error {
			log.Println("Steps are completed!")

			return state.Read(flow.StateContextKey).(tele.Context).Reply("Done")
		}),
))

Secondly, I had to make [State] an interface as you suggested. It's a good point.

Additionally, I've simplified the contract for [Machine].

type Machine interface {
	Back(state State) error
	Next(state State) error
	ToStep(step int, state State) error
	ActiveStep() int
}

Now, I'm considering using 'then' for the step. I believe we need to provide some DTO/interface for the completed step's information. With that opportunity, we can create a generalized solution (logging/metrics, etc.).

@shindakioku
Copy link
Author

shindakioku commented Feb 26, 2024

I suppose we can begin discussing the code now. The main area is ready, particularly the API (I don't plan to change it at the moment). Additionally, I've added metadata information for the flow/steps (the last one is a bit lacking, but...).
There are a few features that I need to implement, but we can review them later since they don't require changes to the code base; these features are just supplementary.

Idle flows: Naturally, we need to terminate idle flows after a user timeout and provide control over that to the user. I intend to achieve this by introducing a new [MetaDataFailureStage]. Since we already know the last step that the user completed, this shouldn't pose a problem.
We need to discuss the behavior of [assigner] and [validation]. I'm confident that there's a good opportunity here, and we need to develop some useful implementations. At the very least, we should validate prompts by type (text/media/etc.), implement a letters range validator, and for the assigner, a basic one that fills the user variable.

The final API appears as follows:

b := &tele.Bot{}
sendUserMessage := func(message string) func(flow.State) error {
	return func(state flow.State) error {
		return state.Read(flow.StateContextKey).(tele.Context).Reply(message)
	}
}
loggingEachStep := func(state flow.State, metadata flow.StepMetaData) {
	log.Println(fmt.Sprintf("Step completed [%d]", metadata.Step))
}

flowBus := flow.NewBus(b, 5*time.Minute)
b.Handle(tele.OnText, flowBus.Handle)

var email, password string
err := flowBus.Flow(
	"/start",
	flow.New().
                OnEachStep(loggingEachStep).
		Next(flow.NewStep(sendUserMessage("Enter email:")).Assign(TextAssigner(&email))).
		Next(flow.NewStep(sendUserMessage("Enter password:")).Assign(TextAssigner(&password))).
		Then(registerUser).
		Catch(flowFailed),
)

@AnatoliiKobzarGL
Copy link

@shindakioku

Then(registerUser).

How registerUser will have access to the variables email and password?
will this approach force me to use handlers as anonymous functions?

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.

3 participants