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.

Sign up for GitHub

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

Jump to bottom

824 streamline frontend change level network calls #839

Merged
merged 28 commits into from
Mar 4, 2024
Merged

824 streamline frontend change level network calls #839

merged 28 commits into from
Mar 4, 2024

Conversation

pmarsh-scottlogic
Copy link
Contributor

@pmarsh-scottlogic pmarsh-scottlogic commented Feb 22, 2024 •
edited
Loading

Description

combines the change level network calls into one

Notes

  • makes new endpoint /level which returns the emails, chat history and defences for a given level
  • deletes the now unused endpoints that get the email, chat history and defences individually
  • renames the existing levelService to resetService (because all it was doing was resetting a level)
  • adds new levelService for accessing our new endpoint

Further PRs

  • I notice that we pass the entire defences object to the front end when switching to and loading up levels 1 and 2. However we don't use any of this information unless we're showing it in the ControlPanel. Which is both confusing to read and also a waste of bandwidth. To fix this I think would warrant a separate PR.
  • There is also the problem of having lots of getModel calls when we switch to sandbox. That can also be a separate PR

Concerns

Checklist

Have you done the following?

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

@pmarsh-scottlogic pmarsh-scottlogic linked an issue Feb 22, 2024 that may be closed by this pull request
Streamline frontend change level network calls 🛠🛠🛠 #824
Closed
@pmarsh-scottlogic pmarsh-scottlogic marked this pull request as ready for review February 22, 2024 11:57
@pmarsh-scottlogic pmarsh-scottlogic marked this pull request as draft February 22, 2024 12:34
@pmarsh-scottlogic pmarsh-scottlogic marked this pull request as ready for review February 22, 2024 14:25
chriswilty
chriswilty requested changes Feb 23, 2024
View reviewed changes
Copy link
Member

@chriswilty chriswilty left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I love a good cleanup 👌

I'd ideally like to see replacement tests rather than just deleting the old ones, but if you think it's really not worth the effort (as it's only really extracting stuff from local storage and sending it in a response) then I can live with that.

I am far more concerned with our lack of UI component tests in general, which is not something you can or should tackle here!

backend/test/unit/controller/chatController.test.ts Show resolved Hide resolved
frontend/src/App.tsx Show resolved Hide resolved
chriswilty
chriswilty requested changes Feb 27, 2024
View reviewed changes
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.

Looking good 👍

Handful of minor suggestions.

backend/src/controller/handleError.ts Outdated Show resolved Hide resolved
backend/test/unit/controller/levelController.test.ts Outdated Show resolved Hide resolved
backend/test/unit/controller/levelController.test.ts Outdated Show resolved Hide resolved
backend/test/unit/controller/startController.test.ts Outdated Show resolved Hide resolved
frontend/src/service/levelService.ts Outdated Show resolved Hide resolved
@pmarsh-scottlogic pmarsh-scottlogic self-assigned this Feb 27, 2024
chriswilty
chriswilty approved these changes Mar 1, 2024
View reviewed changes
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.

Beautiful! One test name looking like it needs correcting, else good to go.

backend/test/unit/controller/levelController.test.ts Outdated
});

test('WHEN client does not provide a level THEN the backend sends the level information for the given level', () => {
test('WHEN client provides an invalid level THEN the backend sends the level information for the given level', () => {
Copy link
Member

Choose a reason for hiding this comment

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

THEN the backend responds with BadRequest ?

@pmarsh-scottlogic pmarsh-scottlogic merged commit ee5bec5 into dev Mar 4, 2024
4 checks passed
@pmarsh-scottlogic pmarsh-scottlogic deleted the 824-streamline-frontend-change-level-network-calls branch March 4, 2024 09:25
@pmarsh-scottlogic pmarsh-scottlogic mentioned this pull request Mar 7, 2024
Merged
3 tasks
chriswilty pushed a commit that referenced this pull request Apr 8, 2024
@pmarsh-scottlogic @chriswilty
824 streamline frontend change level network calls (#839)
c861962
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriswilty chriswilty chriswilty approved these changes

@AAloshine-scottlogic AAloshine-scottlogic Awaiting requested review from AAloshine-scottlogic

@dhinrichs-scottlogic dhinrichs-scottlogic Awaiting requested review from dhinrichs-scottlogic

Assignees

@pmarsh-scottlogic pmarsh-scottlogic

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Streamline frontend change level network calls 🛠🛠🛠
2 participants
@pmarsh-scottlogic @chriswilty