COMPASS-697: Backport COMPASS-666: Keyboard Enter on Create Collection Modal sometimes cause page refresh to 1.5-releases #758

pzrq · 2017-01-23T04:30:42Z

This backports COMPASS-666 to our 1.5-releases branch.

NB: Due to merge conflicts in our rapidly evolving functional test suite since creating the 1.5-releases branch:

The addition of the new data-test-id HTML <form> attributes and the actual code fix of the onSubmit handlers have been backported and manually tested to work
the functional test changes have not been backported

Would be appreciated if someone with time confirms this 🐛-fix works for them too on the 1.5-releases branch locally or via an Evergreen patch build before merging.

… refresh (#753) NOTE: Due to merge conflicts in our rapidly evolving functional test suite since creating the 1.5.x branch, ‘data-test-id’ values and the actual code fix has been backported and manually tested, but the functional test changes have not been back ported.

* Standardise mongodb-runner CI mongodb-js/connection-model#138 NOTE: `npm run check && npm run test-unit` passes for me locally on macOS, but the functional tests `npm test ` still error with results: 177 passing (50s) 2 pending 10 failing 1) Compass #spectron when working with the application when working with collections when selecting a collection renders the sample collection in the title: Error: element (button[data-hook=start-button]) still not visible after 5000ms at elements("button[data-hook=start-button]") - isVisible.js:54:17 at isVisible("button[data-hook=start-button]") - waitForVisible.js:37:22 2) Compass #spectron when working with the application when working with collections when selecting a collection renders the schema tab: Error: An element could not be located on the page using the given search parameters. at element("li#SCHEMA") - click.js:12:17 at elementIdDisplayed("0.9192226665154364-7") - isVisible.js:71:55 3) Compass #spectron when working with the application when working with collections when selecting a collection when applying a filter the text in refine bar matches query: Error: Promise was rejected with the following reason: Error: An element could not be located on the page using the given search parameters ("input#refine_input"). at elements("input#refine_input") - getValue.js:18:17 at getValue("input#refine_input") - waitForValue.js:37:22 4) Compass #spectron when working with the application when working with collections when selecting a collection when applying a filter samples the matching documents: Error: element (div.sampling-message) still not visible after 15000ms at elements("div.sampling-message") - isVisible.js:54:17 at isVisible("div.sampling-message") - waitForVisible.js:37:22 5) Compass #spectron when working with the application when working with collections when selecting a collection when applying a filter updates the schema view: Error: An element could not be located on the page using the given search parameters ("li.bubble code.selectable"). at elements("li.bubble code.selectable") - getText.js:18:17 at getText("li.bubble code.selectable") - compass-functional.test.js:120:16 6) Compass #spectron when working with the application when working with collections when selecting a collection when applying a filter filters out non-matches from the document list: Error: An element could not be located on the page using the given search parameters. at element("li#DOCUMENTS") - click.js:12:17 at elementIdDisplayed("0.9192226665154364-7") - isVisible.js:71:55 7) Compass #spectron when working with the application when working with collections when selecting a collection when applying a filter includes documents that match the filter: Error: An element could not be located on the page using the given search parameters ("div.element-value-is-string"). at elements("div.element-value-is-string") - getText.js:18:17 at getText("div.element-value-is-string") - compass-functional.test.js:138:16 8) Compass #spectron when working with the application when working with collections when working in the documents tab when viewing documents renders the documents in the list: Error: An element could not be located on the page using the given search parameters. at element("li#DOCUMENTS") - click.js:12:17 at elementIdDisplayed("0.9192226665154364-7") - isVisible.js:71:55 9) Compass #spectron when working with the application when working with collections when working in the explain tab when viewing the explain tab renders the stages: Error: An element could not be located on the page using the given search parameters. at element("li#EXPLAIN_PLAN") - click.js:12:17 at elementIdDisplayed("0.9192226665154364-7") - isVisible.js:71:55 10) Compass #spectron when working with the application when working with collections when working in the indexes tab when viewing the indexes tab renders the indexes: Error: An element could not be located on the page using the given search parameters. at element("li#INDEXES") - click.js:12:17 at elementIdDisplayed("0.9192226665154364-7") - isVisible.js:71:55 * Go back to the full pretest and post test hooks Annoying as we can’t test multiple topologies, but probably a decent idea for Compass itself as we should delegate as much as possible to each public mongodb-js dependency. Note: Needed as otherwise TravisCI hangs for 10 minutes then kills the job even though it completes with exit code 0, i.e. successfully. e.g. https://travis-ci.com/10gen/compass/jobs/57691028 * Drop lts/boron We can add it in when we decide we actually want to switch, as our private Travis seems to be a little under-powered at this time (build queues backing up).

rueckstiess

Couple small comments to fix. Thanks!

rueckstiess · 2017-01-24T11:52:12Z

.travis.yml

 language: node_js
 node_js:
- 6.3.1
+  - 6.3.1


While you're fixing this up, can you either add or remove the space before the dash for all array elements?

rueckstiess · 2017-01-24T11:53:31Z

src/internal-packages/database/lib/components/create-collection-dialog.jsx

-          <form name="create-collection-dialog-form">
+          <form name="create-collection-dialog-form"
+            onSubmit={this.onCreateCollectionButtonClicked.bind(this)}
+            data-test-id="create-collection-modal"


do we need these data-test-id markers if we're not going to test these? Is 1.5 before Durran introduced data-test-id even? In that case, I'd remove them.

rueckstiess · 2017-01-24T11:54:16Z

src/internal-packages/database/lib/components/drop-collection-dialog.jsx

          </div>
-          <form>
+          <form data-test-id="drop-collection-modal"
+              onSubmit={this.onDropCollectionButtonClicked.bind(this)}>


The indentation looks a little off here. Can you use 2 spaces?

extra indent to indicate that it's part of the previous line, is this not right?

I always use 2 spaces for that, this is 4. not a big deal, if the linter doesn't complain about it.

rueckstiess · 2017-01-24T11:54:22Z

src/internal-packages/server-stats/lib/component/create-database-dialog.jsx

        <Modal.Body>
-          <form name="create-collection-dialog-form">
+          <form name="create-collection-dialog-form"
+              onSubmit={this.onCreateDatabaseButtonClicked.bind(this)}


same here. 2 spaces.

rueckstiess · 2017-01-24T11:54:48Z

src/internal-packages/server-stats/lib/component/drop-database-dialog.jsx

          </div>
-          <form>
+          <form
+              onSubmit={this.onDropDatabaseButtonClicked.bind(this)}


2 spaces indentation

rueckstiess · 2017-01-24T11:55:51Z

src/internal-packages/database/lib/components/drop-collection-dialog.jsx

+    evt.stopPropagation();
+
+    // prevent drop of collection if names don't match
+    if (this.state.confirmName !== this.state.name) {


Did we not confirm the typed name is identical before? What was the behaviour for 1.5.1?

Pressing enter would've caused a refresh in 1.5.1, after adding an onSubmit method it would drop the collection without checking if it matches.

rueckstiess · 2017-01-24T12:05:26Z

Tested locally too, seems to fix the issue. 👍

It’s that way on master, just too lazy to mine git log to cherry-pick the change that did it somewhere between 1.5 and 1.7, to pass code review comments. #758

pzrq · 2017-01-25T15:05:24Z

Applied changes as requested.

Thanks for reviewing @rueckstiess @KeyboardTsundoku 👍

pzrq added 2 commits January 23, 2017 14:30

pzrq added the ready for review label Jan 23, 2017

rueckstiess suggested changes Jan 24, 2017

View reviewed changes

pzrq added work in progress and removed ready for review labels Jan 24, 2017

rueckstiess approved these changes Jan 25, 2017

View reviewed changes

pzrq added 2 commits January 26, 2017 01:48

Standardise on no space

1e57ec2

It’s that way on master, just too lazy to mine git log to cherry-pick the change that did it somewhere between 1.5 and 1.7, to pass code review comments. #758

Drop data-test-ids we do not need on 1.5-releases

5b100c9

pzrq merged commit ef915d1 into 1.5-releases Jan 25, 2017

pzrq deleted the COMPASS-697-backport-enter-key-1-5 branch January 25, 2017 15:04

COMPASS-697: Backport COMPASS-666: Keyboard Enter on Create Collection Modal sometimes cause page refresh to 1.5-releases #758

COMPASS-697: Backport COMPASS-666: Keyboard Enter on Create Collection Modal sometimes cause page refresh to 1.5-releases #758

Uh oh!

Conversation

pzrq commented Jan 23, 2017

Uh oh!

rueckstiess 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!

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!

rueckstiess commented Jan 24, 2017

Uh oh!

pzrq commented Jan 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants