Connected Mode #823

cdifino · 2019-09-21T19:23:12Z

[x ] By placing an X in the preceding checkbox, I verify that I have signed the Contributor License Agreement

V3

Fixed side-runner foreach command

Fixed the for each test.

Fixed failed test for Foreach bug fix

merge last version of V3

V3 update

V3

API bug (SeleniumHQ#809)

Fixed typo. Action reference should be Actions reference to avoid cas…

cdifino · 2019-09-21T19:33:03Z

Hello @corevo here is the modifications I have done for the connected mode feature.
Here is how it currently works:
If it is in connected mode and we want to interact with Selenium IDE, it will always need to match the connection id. If it is not in connected mode, there is no need to pass a connection ID in the payload and everything should work fine.
The only way to create another connection is by calling "connect with the new connection id"
When connecting to a new extension it will prompt the user is he/she wants to connect.
If extension wants to load project to Selenium IDE, and the user has unsaved changes, it will prompt the user to save files or just load. This will happen if there is a connection or not.
If you need an extension to test these changes I made this little extension.
ext.zip

packages/selenium-ide/src/api/v1/index.js

packages/selenium-ide/src/background/background.js

packages/selenium-ide/src/neo/stores/view/UiState.js

corevo · 2019-09-22T11:02:48Z

packages/selenium-ide/src/neo/stores/view/UiState.js

  pauseNotificationSent = false
+  @observable
+  isControlledBy = {
+    connectionId: null,


Not sure why we need connectionId we can use the extensionId instead.

Just want to make it more flexible to the user, if there are multiple instance within the same extension ID you want the user to be able to control what instance is talking to Selenium IDE.

corevo · 2019-09-22T11:04:16Z

packages/selenium-ide/src/api/v1/index.js


 router.post('/register', (req, res) => {
+  if (req.connectionId != UiState.isControlledBy.connectionId)
+    throw new Error('Another connection is established')


I think we can register other plugins, we should just block requests to /control so only one can control it, thoughts?

I think that the idea is to block everything but /control. When Selenium is controlled by a specific extension, no other extension should be listening to events for security purposes. On the other hand, the only way to interrupt the connection is to request control through the /control call. If the user accepts to change the connection, the new extension will have control, and all the plugins will be unplugged.

packages/selenium-ide/src/api/v1/index.js

corevo · 2019-09-24T10:34:38Z

packages/selenium-ide/src/api/v1/index.js

-})
+router.post(
+  '/register',
+  controlledOnly((req, res) => {


I think we should only block other extensions from calling /control right?

If the extension is in controlled mode, other plugins should still work, what do you think?

packages/selenium-ide/src/neo/stores/view/UiState.js

Connected Mode Implementation Fixed check for connection ID bug Fixed prettier bug and changed description of pop up Fixed prettier bug Fixed prettier bug Plugin Manager Fix, modified dialog, window focus addition Closure Prettier Prettier Prettier Restart to welcome sccreen when entering connected mode Prettier Prettier Prettier Prettier Prettier Prettier Prettier debug travis fail restart capabiulity everytime is starting a connection prettier travis bugs travis bugs

melode11

I had concern of the selenium IDE's extension id. For now third party plugin need to use a hardcoded string as extension id to send message to Selenium IDE. Selenium IDE's extension id could change when manifest file changes. Shall we have a way of storing the extension id to a special key in local storage whenever the IDE extension's background or contentScript is executed. So third party extension can look for that key to always get the right extension id.
@corevo Thoughts?

packages/selenium-ide/src/api/v1/index.js

packages/selenium-ide/src/neo/IO/filesystem.js

packages/selenium-ide/src/api/v1/index.js


 const router = new Router()

+const controlledOnly = apiFn => {


cdifino · 2019-09-27T06:52:05Z

packages/selenium-ide/src/api/v1/index.js

+  if (
+    (Manager.controller &&
+      Manager.controller.connectionId != req.connectionId) ||
+    !Manager.controller


when manager.controller is undefined it only means that there is no assisted control yet. This scenario is directed to the case when there is a connected mode but the connection id is a different one or when there is not connected mode and there is a potential for connecting.

cdifino · 2019-09-27T06:53:09Z

packages/selenium-ide/src/background/background.js

+  if (
+    message.control &&
+    message.controller.connectionId &&
+    message.controller.id


the senders. Thant is how the plugin manifesto works. The sender (or extension Id) turn out to be the plugin id.

Changing errors to response with error code

Connected Mode Implementation Fixed check for connection ID bug Fixed prettier bug and changed description of pop up Fixed prettier bug Fixed prettier bug Plugin Manager Fix, modified dialog, window focus addition Closure Prettier Prettier Prettier Restart to welcome sccreen when entering connected mode Prettier Prettier Prettier Prettier Prettier Prettier Prettier debug travis fail restart capabiulity everytime is starting a connection prettier travis bugs travis bugs documentation of connected mode deleted end connection

Nbansal/connected mode4

Nbansal/connected mode3

… into nbansal/connectedMode3

Nbansal/connected mode3

melode11 · 2019-10-09T17:29:00Z

All the changes are done in our side. current state of the connected Mode PR is thoroughly tested by our testing team, including 10+ different corner cases are covered and all work as expected.

corevo

Overall looks good & clean.

corevo · 2019-10-10T14:26:37Z

packages/selenium-ide/src/background/background.js

+    let payload = message.payload
+
+    payload.sender = sender.id
+    if (message.uri === '/_connect' || message.uri === '/_close') {


We could have an internal API that starts with /private instead of prefixing with _.

This sounds good. I will do these changes and push them to PR

corevo · 2019-10-10T14:28:13Z

packages/selenium-ide/src/neo/components/PauseBanner/index.jsx

+    } else if (UiState.isControlled) {
+      return (
+        <div className="connected-mode">
+          <div className="connected-mode">


Why both divs have the same class attached?
Also controlled-banner instead of connected-mode.

corevo · 2019-10-10T14:28:36Z

packages/selenium-ide/src/plugin/manager.js

    return this.plugins.find(p => p.id === pluginId)
  }

+  unregisterAllPlugins() {


Unnecessary

Unnecessary

Fixed.

corevo · 2019-10-10T14:29:45Z

docs/api/plugins/system.md

 ```
+### `POST /control`

+Start a connection with a specific connection Id from another chrome extension. When this connection is accepted by the user, Selenium IDE restarts and registers the caller, and the extension takes exclusive control of Selenium IDE until user closes Selenium IDE or another connection is accepted. When this mode is on, the save to computer functionality gets overritten by sending the side file to the extension controlling Selenium IDE. When in this mode, the user needs to add connectionId variable to make other API calls such as `POST /project`, `GET /project`, `POST /log`, `POST /register`.


Why use connectionId instead of using the controller extension ID? seems less secure since this is a user-generated token rather than a browser secure token.

Agree, we can switch to use sender id.

corevo · 2019-10-10T14:35:33Z

packages/selenium-ide/src/api/v1/index.js

+  },
+}
+
+function checkControl(req) {


This function name is very ambiguous.

The function returns true in 2 cases

Extension is not controlled by anyone

Extension is controlled, and the request is sent by the controller

While returning false in 1 case

Extension is controlled by someone else

So I think we should split this function (and the decorator) into two that would be more readable
isControlled - would return true/false if the IDE is under control of any extension
isControlledByThisExtension - would return true only if the current extension is the controller

Then we can compose the decorators.

I think I can make the name to be more clear. Split to more decorators sounds OK but we are never going to used the sparated decorators in other places. So rather keep it in a single function to simply the call hierarchy. Is that make sense?

Actually it is hard to think a good name for it. I'll just do split.

corevo · 2019-10-10T14:44:42Z

packages/selenium-ide/src/neo/IO/filesystem.js

-      conflictAction: 'overwrite',
-    })
+    return UiState.isControlled
+      ? Promise.resolve()


This should not silently fail, we should send a message to the controller that the user requested to save.

Look at PlaybackState to get an idea of how to send an event over to an extension.

I was thinking of something like:

{ action: 'event', event: 'saveProject', options: { project } }

We already send the plugin with the emit of the project entity. Do we expect to send another event here?

Emit project is something else, in controlled mode we shouldn't emit to begin with.

So to clarify.
You want in control mode, IDE will send a saveProject event, do you still want to keep the emit? I guess we should still keep it for consistency between control and non-control mode?

…ctedMode

* resolve review comments. * Fix some indention issue. * Fix the max-width of banner.

… into nbansal/feedbackchanges

Nbansal/feedbackchanges

indentation changes to fix build

fixing css indentation issues

corevo · 2019-10-10T21:50:56Z

packages/selenium-ide/src/background/background.js

+          delete payload.restart
+
+          const newMessage = {
+            uri: '/privateConnect',


I meant /private/connect and use the /private/ prefix in general for internal IDE messages.

Also keep things lower cased.

changing private apis to /private/ sending project in message on save

cdifino and others added 10 commits July 18, 2019 08:08

Merge pull request #1 from SeleniumHQ/v3

44a5552

V3

Fixed side-runner foreach command

be598d5

Merge pull request #3 from cdifino/foreachbug

99a2c87

Fixed side-runner foreach command

Fixed Test for foreach in Selianize

59f33f8

Fixed the for each test.

Merge pull request #4 from cdifino/testFix

11c3247

Fixed failed test for Foreach bug fix

Merge pull request #5 from SeleniumHQ/v3

adf91f1

merge last version of V3

Merge pull request #6 from SeleniumHQ/v3

bf81408

V3 update

Merge pull request #9 from SeleniumHQ/v3

db7a39a

V3

Merge pull request #10 from SeleniumHQ/v3

2d5ec4f

API bug (SeleniumHQ#809)

Merge pull request #11 from SeleniumHQ/v3

e40e900

Fixed typo. Action reference should be Actions reference to avoid cas…

corevo reviewed Sep 22, 2019

View reviewed changes

packages/selenium-ide/src/api/v1/index.js Outdated Show resolved Hide resolved

corevo reviewed Sep 22, 2019

View reviewed changes

packages/selenium-ide/src/background/background.js Outdated Show resolved Hide resolved

corevo reviewed Sep 22, 2019

View reviewed changes

packages/selenium-ide/src/neo/stores/view/UiState.js Outdated Show resolved Hide resolved

corevo reviewed Sep 22, 2019

View reviewed changes

packages/selenium-ide/src/api/v1/index.js Outdated Show resolved Hide resolved

corevo reviewed Sep 24, 2019

View reviewed changes

packages/selenium-ide/src/neo/stores/view/UiState.js Outdated Show resolved Hide resolved

cdifino force-pushed the connectedMode branch from 325573b to 081f824 Compare September 26, 2019 00:29

cdifino added 6 commits September 25, 2019 22:50

documentation of connected mode

247e6f6

deleted end connection

805222c

change saving message

7a46350

fix bug

276f9ef

prettier

dbabaac

deleted a unwanted file

04aacdc

melode11 reviewed Sep 27, 2019

View reviewed changes

jlam and others added 2 commits September 27, 2019 15:54

changing errors to response with error code

d0b8112

Merge pull request #13 from cdifino/nbansal/connectedMode

62b342e

Changing errors to response with error code

nbansal1993 and others added 3 commits October 8, 2019 15:43

fixing error message

691137e

reindent using eslint instead of prettier

edfc31b

cdifino force-pushed the connectedMode branch from df575cb to 39ab8f7 Compare October 8, 2019 22:44

nbansal1993 added 8 commits October 8, 2019 16:07

Merge branch 'connectedMode' into nbansal/connectedMode4

785aa08

fixing bugs added in squash

368e8c6

Merge pull request #26 from cdifino/nbansal/connectedMode4

21abbbe

Nbansal/connected mode4

Merge pull request #27 from cdifino/nbansal/connectedMode3

9f47cf4

Nbansal/connected mode3

adding import back

ea7504a

Merge branch 'connectedMode' of https://github.com/cdifino/selenium-ide…

c020788

… into nbansal/connectedMode3

adding import back

14f70d0

Merge pull request #28 from cdifino/nbansal/connectedMode3

1c783b6

Nbansal/connected mode3

corevo requested changes Oct 10, 2019

View reviewed changes

Yuxing Yao and others added 12 commits October 10, 2019 10:25

resolve review comments.

6554751

Merge remote-tracking branch 'origin/connectedMode' into yuxyao-conne…

fb31932

…ctedMode

Fix some indention issue.

6099569

Fix the max-width of banner.

eac8de6

connected mode resolve comments. (#29)

880db1e

* resolve review comments. * Fix some indention issue. * Fix the max-width of banner.

Merge branch 'connectedMode' of https://github.com/cdifino/selenium-ide…

d781dca

… into nbansal/feedbackchanges

updating the api documentation

65e43ba

Merge pull request #30 from cdifino/nbansal/feedbackchanges

a10d1a4

Nbansal/feedbackchanges

indentation changes to fix build

f81cec2

Merge pull request #31 from cdifino/nbansal/feedbackchanges

8ea36ae

indentation changes to fix build

fixing css indentation issues

fe56128

Merge pull request #32 from cdifino/nbansal/feedbackchanges

f3476b8

fixing css indentation issues

corevo reviewed Oct 10, 2019

View reviewed changes

Nbansal/feedbackchanges (#33)

b068a29

changing private apis to /private/ sending project in message on save

corevo approved these changes Oct 11, 2019

View reviewed changes

corevo merged commit 42c79a2 into SeleniumHQ:v3 Oct 11, 2019


		const router = new Router()

		const controlledOnly = apiFn => {

Uh oh!

Connected Mode #823

Connected Mode #823

Uh oh!

Conversation

cdifino commented Sep 21, 2019

Uh oh!

cdifino commented Sep 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

melode11 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melode11 commented Oct 9, 2019

Uh oh!

corevo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

melode11 left a comment •

edited

Loading