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

645 streamline network calls for resetting level progress #872

Merged

Conversation

pmarsh-scottlogic
Copy link
Contributor

@pmarsh-scottlogic pmarsh-scottlogic commented Mar 21, 2024

Description

Here's what happens when you used to reset level progress:

  • openai/clear clears the chat history for given level.
  • email/clear clears the emails for given level.
  • openai/addHistory adds the "Level progress reset" info message to given level's chat history

Now we just have

  • resetlevel which clears the chat history and emails of current level and returns the resulting into chatMessage

Screenshots

image

Notes

  • adds the resetlevel endpoint, and related service methods and models
  • adds tests for the above
  • removes the email/clear endpoint (and therefore the whole email controller and email service and related models)
  • removes the openai/clear endpoint (and related service and model code)
  • removes tests for the above
  • fixes a typo on StartResponse
  • renames the frontend's models/combined.ts to models/apiResponse which @chriswilty suggested a few weeks back

Concerns

  • not sure about the endpoint name
  • not sure whether we should wrap the resulting message in an object {resultingChatInfoMessage: //} . I think it's better to make it clear what this message is by giving it a name in the response.

Checklist

Have you done the following?

  • Linked the relevant Issue
  • Added tests
  • Ensured the workflow steps are passing

Copy link
Member

@chriswilty chriswilty left a comment

Choose a reason for hiding this comment

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

This looks really good, lovely to have so much code removed! A couple of minor suggestions only.

backend/src/models/api/LevelResetRequest.ts Outdated Show resolved Hide resolved

// model configurations
router.post('/openai/model', handleSetModel);
router.post('/openai/model/configure', handleConfigureModel);

// reset progress for all levels
router.post('/reset', handleResetProgress);
router.post('/resetlevel', handleResetLevel);
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking of something like this for the endpoints. Originally I considered a path param for reset level, but it didn't seem quite right, plus this is nice as it's consistent:

POST /restart?currentLevel=N

POST /reset?level=N

I also note that the /level endpoint seems a touch redundant, and could definitely use a path param, but that's a separate matter.

GET /level?level=1
vs
GET /level/1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dislike using restart and reset to distinguish between resetting the whole game and resetting a single level - think it should be spelled out which does which. I'm also keen to keep the language consistent with the frontend.

happy with other alternatives that do this. e.g:

POST /resetall?currentLevel=N

POST /reset?level=N
POST /reset/all?currentlLevel=N

POST /reset?level=n

Copy link
Member

Choose a reason for hiding this comment

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

Well, there's little difference between resetall and reset and restart, so it's going to be confusing whichever of these we would pick...

How about "resetgame" or similar?

Alternatively, we could use my initial thought about using a path param combined with yours for reset all:

POST /reset/all?currentLevel=N
POST /reset/{level}

Then it comes down to making it work in Express; likely we need them defined in the above order, so that express looks for "all" before trying a path param for level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I like this. Done

Copy link
Member

@chriswilty chriswilty left a comment

Choose a reason for hiding this comment

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

Nearly there...

frontend/src/models/apiResponse.ts Outdated Show resolved Hide resolved
frontend/src/service/resetService.ts Show resolved Hide resolved
Copy link
Member

@chriswilty chriswilty left a comment

Choose a reason for hiding this comment

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

Lovely 👍

@pmarsh-scottlogic pmarsh-scottlogic merged commit 3c0cefb into dev Mar 28, 2024
4 checks passed
@pmarsh-scottlogic pmarsh-scottlogic deleted the 645-streamline-network-calls-for-resetting-level-progress branch March 28, 2024 09:50
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.

Streamline network calls for resetting level progress 🛠🛠
2 participants