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

feat(app, app-shell): add ability to disable discovery cache #5759

Merged
merged 10 commits into from
Jun 4, 2020

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented May 28, 2020

Closes #4853

overview

This PR adds ability to disable caching new robots to discovery.json using a toggle.

changelog

  • added logic to disable caching robots to discovery.json on discovery.disableCache change
  • added labeled toggle to NetworkSettingsCard
  • attached state and dispatch props to turn caching on/off
  • added toggle component tests
  • added app-shell discovery tests that check caching behavior on config change

review requests

  • Check if code looks good
  • In discovery test, flow wasn't used initially and I have kept it that way. Should I update the file or let it continue without flow? Out of scope?
  • Check if it works:
  • Disable cache toggle component is populated on Network & System page with full description & is off by default
  • Toggling it ON clears the cache & stops adding to cache
  • If possible, introduce new instances of robots on the network and verify that they don't get added to discovery.json One way to mock this is by changing a connected robot's name (/server/name POST request with body {"name": "Some new name"})

risk assessment

Medium. Touches implementation of discovery logic but is quite a small change.

@sanni-t sanni-t requested a review from mcous May 28, 2020 15:52
Copy link
Contributor

@mcous mcous 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! Initial thoughts

app/src/components/NetworkSettingsCard/index.js Outdated Show resolved Hide resolved
app-shell/src/discovery.js Show resolved Hide resolved
app/src/config/actions.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Last two changes to get typechecking and tests passing

@sanni-t sanni-t marked this pull request as ready for review June 3, 2020 17:30
@sanni-t sanni-t requested a review from a team as a code owner June 3, 2020 17:30
@sanni-t sanni-t added app Affects the `app` project ready for review labels Jun 3, 2020
@mcous mcous added robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). feature Ticket is a feature request / PR introduces a feature labels Jun 3, 2020
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Code and behavior LGTM 🌵

@sanni-t sanni-t requested a review from a team June 4, 2020 15:12
Copy link
Contributor

@Kadee80 Kadee80 left a comment

Choose a reason for hiding this comment

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

works on my 🤖 !

@sanni-t sanni-t merged commit 5ad57d9 into edge Jun 4, 2020
@sanni-t sanni-t deleted the app_add-cache-disabling branch June 4, 2020 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project feature Ticket is a feature request / PR introduces a feature robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: App should allow the user to disable discovery caching
3 participants