timgarrels / gameprog-detective-server

Server Komponente für "A Detecitve Game" des Gameprog Seminars
https://hpi.de/studium/lehrveranstaltungen/it-systems-engineering-ma/lehrveranstaltung/wise-19-20-2889-game-programming.html
3 stars 0 forks source link

route for only getting current answers without providing a reply #32

Closed robinwersich closed 4 years ago

robinwersich commented 4 years ago

Problem

After the /start command the bot asks the server for new answers. The users answer (/start <token> is then checked for validity, but doesn't appear in any answer options and is thus neglected.

Suggestion

with the new route, the bot could ask for the current answers for a given user, without needing to provide a reply. This could be used after the user sends the start command

timgarrels commented 4 years ago

The problem with the bot using a reply independent answer endpoint is that the server can not handle invalid replies or loop messages anymore. Currently the server has logic to decide what to do with a current reply. Changing the endpoint to be independent of the reply would pose the need of making [...]/answers stateful, which is a terrible solution.

However, this seems to be a mistake in the bot as well. The bot should have a /start-command handler, that differs from the behavior of the normal text handler by not calling a reply dependent endpoint. It should instead just try to register the user and get answers in return.

I think this is part of a bigger problem on the server side actually. Does the server set a new story point when ansers?reply=reply is sent?

There should be separate endpoints like

  1. user/<id>/story/replies
  2. user/<id>/story/answers
  3. user/<id>/story/proceed?reply=reply

which are used to access/debug/edit the story only and are not access by the bot!

There should be endpoints for the bot to react to /start and all other text. This could look like this (I think we should keep telegram out of the endpoints to emphasize that we could use any bot enabled chat program)

  1. users/registerBot?handle=<handle>&token=<token>
  2. user/<id>/bot/answers?reply=<reply>
  3. user/<id>/bot/replyOptions

This way the server exposes an interface to the bot (4 to 6) that does not necessarily influence the story progression. That would be handled by the logic behind those endpoints on the server side. This logic then could call actual story endpoints (1 to 3) that cause the user state to change.

I numbered the endpoints to easier talk about them.

robinwersich commented 4 years ago

This is actually close to what I meant (more elaborated of course) I mostly agree with what you said :) However, I'm not sure what you mean with .../answers would be stateful. If any, it's less stateful (it's equivalent to 2. with the difference that I would let the bot access it)

So: I like the interface you proposed, and would like to make the addition, that reply is optional in 5., so that if it's omitted the story is not progressed. This could be used when handeling the /start command, because there is no (real) user reply (this would mean the story state starts with the start_point instead of null which makes more sense imo anyway - the "user not yet registered"-state is still modelles by the telegram handle being null)

btw, answers and replies are synonyms and should not be used to differentiate between bot and user ^^ botMessages and userReplies would be more appropriate

also, REST suggests not using camelCase but hyphens (thus bot-messages) we might consider doing so

timgarrels commented 4 years ago

Lets do that

timgarrels commented 4 years ago

Changes

A write up of the discussion above

Add /users/<id>/story endpoint

There should be endpoints regarding the story independent of the bots interface. Needed endpoints would include:

Refactor current Bot & Bot API to /users/<id>/bot endpoint

The bot should access the users via the userID and not via a second primary key (the handle).

Bot Refactor

Bot API Refactor

Furthermore the endpoint /users?bot_handle=<telegram_handle> is needed to be implemented for this. See #33

robinwersich commented 4 years ago
timgarrels commented 4 years ago

Despite this post beeing German and therefore completly unreadable, I still agree with its main points.

I dont think that the start command should be coded into story.json as we then would have a static beginning that would always include a unnecessary start point, whose only purpose is to house the /start reply and the next story point. Instead we could leave story.josn as is and code the artificial "start command storypoint" into the storycontroller.py, so the user does not see engine specific things in the story file. This would be really easy todo, as we just need to add said storypoint into the loaded dictionary and add a path {start, initialStorypoint}, where initialStorypoint is just the start point from story.json

timgarrels commented 4 years ago

Is users/<id>/story/next-messages-and-replies?reply=<reply> even needed? The bot could just call users/<id>/story/proceed?reply=<reply>, which could contain the logic for proceeding based on the reply. A debug interface to proceed is not good, as it would actually break the bot

robinwersich commented 4 years ago

I though that YOU discouraged the idea and I saw reasons for that (the already mentioned race conditions). But we could drop the proceed endpoint

timgarrels commented 4 years ago

Nope. At least I hope. I was against a bot-answer endpoint independent of reply used by the bot.

robinwersich commented 4 years ago

You can try if there are problems with the race conditions. But I still think there might be

timgarrels commented 4 years ago

Could you go more into detail about those race conditions? Where did you describe them?

robinwersich commented 4 years ago

If the bot sends proceed, and shortly after bot-messages, the server might not have processed the old requests and send the old messages. However, we could probably fix this with a delay

timgarrels commented 4 years ago

This can not happen, as neither the bot nor the server works async in the context of a single user. The bot waits for the response to proceed and the server does not trigger async function in order to respond to proceed Therefore bot-messages can not be the old messages.

(If there would be multiple bot instances this would be a problem. But this is not the case)

robinwersich commented 4 years ago

oh, of course - then let's drop the endpoint in question :)

robinwersich commented 4 years ago

After another discussion, we decided, that we should replace bot-messages (which also provided error messages on incompleted taskswith/current-story-point/description(which does only provide the description but not error messages). In return, theproceed` endpoint should return the new bot messages

robinwersich commented 4 years ago

resolved by #46