fix(app): avoid the name conflict issue in the same network #10723

koji · 2022-06-10T15:37:14Z

Overview

This PR will avoid the same name robot issue in the same network. if there is the same name in the same network, the app shows an error message.

In terms of the error message, this should be temporary. We need to ask about the reasonable message to @emilywools and @ecormany
Also need support from the design team since if we remove unavailable robot, we will need to add a modal to get confirmation from users.

#10709

Changelog

Add validation to the form
Add the test case

Review requests

If type an existing name, the slideout shows an error message (This function only works with a real robot)

Risk assessment

low

…twork this PR will avoid the same name robot issue in the same network. if there is the same name in the same network, the app shows error message.

codecov · 2022-06-10T15:48:54Z

Codecov Report

Merging #10723 (c5eb334) into release_6.0.0 (28e6aa5) will increase coverage by 0.00%.
The diff coverage is 60.00%.

@@              Coverage Diff               @@
##           release_6.0.0   #10723   +/-   ##
==============================================
  Coverage          73.84%   73.84%           
==============================================
  Files               2158     2158           
  Lines              58341    58361   +20     
  Branches            5961     5966    +5     
==============================================
+ Hits               43079    43094   +15     
- Misses             13989    13994    +5     
  Partials            1273     1273

Flag	Coverage Δ
app	`71.60% <60.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...edTab/AdvancedTabSlideouts/RenameRobotSlideout.tsx	`66.66% <60.00%> (-1.34%)`	⬇️
app/src/molecules/JogControls/index.tsx	`100.00% <0.00%> (ø)`

vabruzzo · 2022-06-10T15:59:28Z

...src/organisms/Devices/RobotSettings/AdvancedTab/AdvancedTabSlideouts/RenameRobotSlideout.tsx

@@ -64,6 +70,11 @@ export function RenameRobotSlideout({
      if (!regexPattern.test(newName)) {
        errors.newRobotName = t('rename_robot_input_limitation_detail')
      }
+      if (
+        healthyReachableRobots.find(robot => newName === robot.name) != null


small suggestion, the some method here would avoid the need for the null/undefined check as it returns true or false

yeah, you are right. Thank you, @vabruzzo!

vabruzzo

lgtm 👍

mcous · 2022-06-10T16:27:32Z

...src/organisms/Devices/RobotSettings/AdvancedTab/AdvancedTabSlideouts/RenameRobotSlideout.tsx

+  const healthyReachableRobots = useSelector((state: State) =>
+    getConnectableRobots(state)
+  )


Why is this limited to connectable robots? If a robot is merely "reachable" (but not "connectable"), it may still be on the network advertising over mDNS and therefore a potential source of name conflict.

I also think there's a strong argument to include all robots, not just the currently connectable or reachable one. If you rename a robot to the same name as an unreachable one, the app will think: "hey, this unreachable robot just became reachable", which is not what happened. And, if that unreachable robot comes back online after you've renamed another robot, it will have a new naming conflict and rename itself to avoid that conflict.

I personally think it's reasonable to force the user to "Forget unreachable robot" before allowing them to re-use a name

Mike thank you for your feedback!

Merge branch 'edge' into app_fix-temporary-solution-for-the-same-name

ecormany · 2022-06-13T15:36:39Z

Regarding the error message, I think we should be a little more specific than "Robot name already exists". Maybe something like "Another robot on your network already has this name".

As for the feature itself, what happens if I name robot 1 "BitterWood", turn it off, turn on robot 2, name it "BitterWood", and then turn robot 1 back on again?

koji · 2022-06-15T18:22:52Z

Regarding the error message, I think we should be a little more specific than "Robot name already exists". Maybe something like "Another robot on your network already has this name".

Thanks, @ecormany ! (I will post your suggestion to the team channel).

As for the feature itself, what happens if I name robot 1 "BitterWood", turn it off, turn on robot 2, name it "BitterWood", and then turn robot 1 back on again?

When turning on robot 1, the name of robot1 will be BitterWoodNum2.
Backend does this
However, this case needs some time. If a user does this rename process really quick(during starting up) or renames via API.
The name conflict happens in the app.

There would be 2 cases (this depends on users' env)

[start]
robot1 Otie on (available) [connectableRobot]
robot2 BitterWood off (unavailable) [reachableRobot]

[rename]
The app shows an error message BitterWood already exists

[start]
robot1 Otie on (available) [connectableRobot]
robot2 BitterWood off (unavailable) [unReachableRobot] (not cached)

[rename]
The app removes BitterWood from unavailable robots and sends a request to a server
BitterWood shows up as an available robot when turning on robot 2
BitterWoodNum2 shows up when turning on robot1

koji · 2022-06-15T18:53:00Z

The message for name conflict
Another robot on your network already has this name. Please choose a different name.

update the error message

useGetRobotName

test the branch with a robot app_fix-

This PR is for robot rename function and this PR will be used by #10723 to retrieve the new robot name partially fix #10709

koji · 2022-06-22T23:55:06Z

merge [Pending] feat(app) App feat add get robotName #10871 from release_6.0.0
use useRobotName

nusrat813 · 2022-06-30T15:11:05Z

Verified fixed

fix(app): this PR will avoid the same name robot issue in the same ne…

6ef4fd5

…twork this PR will avoid the same name robot issue in the same network. if there is the same name in the same network, the app shows error message.

koji requested a review from a team as a code owner June 10, 2022 15:37

koji requested review from TamarZanzouri, vabruzzo, aptrons22, b-cooper and jerader and removed request for a team and TamarZanzouri June 10, 2022 15:37

vabruzzo reviewed Jun 10, 2022

View reviewed changes

koji changed the title ~~fix(app): this PR will avoid the same name robot issue in the same network~~ fix(app): avoid the same name robot issue in the same network Jun 10, 2022

address pr comment

2aafb4e

koji requested a review from vabruzzo June 10, 2022 16:11

vabruzzo approved these changes Jun 10, 2022

View reviewed changes

mcous reviewed Jun 10, 2022

View reviewed changes

koji added 4 commits June 10, 2022 14:40

sync with edge

2f7b58c

Merge branch 'edge' into app_fix-temporary-solution-for-the-same-name

check connectable, reachable and unreachable robots

6b0b8cb

change var name

12a8322

Add a new test case

c5eb334

koji requested a review from mcous June 10, 2022 22:49

koji changed the title ~~fix(app): avoid the same name robot issue in the same network~~ fix(app): avoid the name conflict issue in the same network Jun 13, 2022

koji changed the base branch from edge to release_6.0.0 June 13, 2022 15:29

koji deleted the branch release_6.0.0 June 13, 2022 23:53

koji closed this Jun 13, 2022

koji reopened this Jun 13, 2022

koji added the required-for-6.0 label Jun 15, 2022

koji added a commit that referenced this pull request Jun 22, 2022

feat(app): this PR is for robot rename function

5015c92

This PR is for robot rename function and this PR will be used by #10723 to retrieve the new robot name partially fix #10709

koji mentioned this pull request Jun 22, 2022

[Pending] feat(app) App feat add get robotName #10871

Closed

koji merged commit 9ba3c37 into release_6.0.0 Jun 23, 2022

koji deleted the app_fix-temporary-solution-for-the-same-name branch June 23, 2022 21:04

nusrat813 added the complete Completely resolved and/or verified fixed. label Jun 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(app): avoid the name conflict issue in the same network #10723

fix(app): avoid the name conflict issue in the same network #10723

koji commented Jun 10, 2022 •

edited

Loading

codecov bot commented Jun 10, 2022 •

edited

Loading

vabruzzo Jun 10, 2022

koji Jun 10, 2022

vabruzzo left a comment

mcous Jun 10, 2022

koji Jun 11, 2022

ecormany commented Jun 13, 2022

koji commented Jun 15, 2022 •

edited

Loading

koji commented Jun 15, 2022 •

edited

Loading

koji commented Jun 22, 2022 •

edited

Loading

nusrat813 commented Jun 30, 2022

fix(app): avoid the name conflict issue in the same network #10723

fix(app): avoid the name conflict issue in the same network #10723

Conversation

koji commented Jun 10, 2022 • edited Loading

Overview

Changelog

Review requests

Risk assessment

codecov bot commented Jun 10, 2022 • edited Loading

Codecov Report

vabruzzo Jun 10, 2022

Choose a reason for hiding this comment

koji Jun 10, 2022

Choose a reason for hiding this comment

vabruzzo left a comment

Choose a reason for hiding this comment

mcous Jun 10, 2022

Choose a reason for hiding this comment

koji Jun 11, 2022

Choose a reason for hiding this comment

ecormany commented Jun 13, 2022

koji commented Jun 15, 2022 • edited Loading

koji commented Jun 15, 2022 • edited Loading

koji commented Jun 22, 2022 • edited Loading

nusrat813 commented Jun 30, 2022

koji commented Jun 10, 2022 •

edited

Loading

codecov bot commented Jun 10, 2022 •

edited

Loading

koji commented Jun 15, 2022 •

edited

Loading

koji commented Jun 15, 2022 •

edited

Loading

koji commented Jun 22, 2022 •

edited

Loading