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

Replication page update #669

Closed
wants to merge 13 commits into from
Closed

Conversation

benkeen
Copy link
Member

@benkeen benkeen commented Mar 16, 2016

This PR refactors the replication page for a simpler wizard-like structure. Advanced replication options are coming down the pipe in a different PR.

Replication scenarios now working:

local -> new local db 
local -> existing local db
local -> new remote db
local -> existing remote db
remote -> remote new DB
remote -> remote existing DB
remote -> new local db
remote -> existing local db

Yaaaaaaaayyyyyyyyy.

@benkeen benkeen force-pushed the replication-page-update branch 3 times, most recently from 8a897c3 to 51fe845 Compare June 8, 2016 09:43
@benkeen
Copy link
Member Author

benkeen commented Jul 4, 2016

Hey @justin-mcdavid-ibm, thanks for the feedback. Mind taking a look at this PR and seeing what you think? All feedback has been done (see below), except for this one:

A source database that is open to/readable by the public does not require $USERNAME and $PASSWORD in its URL, right?

Yeah you're right. I was able to replicate a publicly available db without specifying credentials. What do you suggest for the help blurb? Right now it explicitly mentioned creds.(https://$REMOTE_USERNAME:$REMOTE_PASSWORD@$REMOTE_SERVER/$DATABASE).

Done

  • The replicate button activates after fields have been clicked on, but not before information is actually entered. Can we have the replication button activate only after the fields are populated? +1
  • Text on the replication button should be “Start Replication”
  • Increase the widths of all dropdowns
  • The in-field explanatory text for Remote Sources and Targets should be https://$REMOTE_USERNAME:$REMOTE_PASSWORD@$REMOTE_SERVER/$DATABASE -
  • Can we leave the replication button inactive if the DB listed as an existing local source doesn’t actually exist? I can enter an db that doesn’t exist and still trigger replication. Same for non-existing local targets.
  • There doesn’t appear to be any checking for the pre-existence of a “new” database. I selected “new local database,” and used the same DB for source and target. Can we put in check in to make sure that users aren’t accidentally overwriting existing DBs? I’m assuming that there are back-end safeguards, but my test-replication job didn’t throw any errors.

@benkeen benkeen force-pushed the replication-page-update branch 2 times, most recently from ed459f6 to e4943f8 Compare July 12, 2016 02:50
@justinmcdavid
Copy link

Hey Ben,

I'm testing this, and it's awesome. Great work on this. It's a big improvement.

There are a couple of things I'm seeing that we probably should tweak.

The "Select source" and "Select target" in-field explanatory test should be gray, though values selected through those dropdowns should remain the color of regular text.

When you specify a custom replication doc ID, it has to be a new ID (folks might think they can specify an existing doc-- which causes a conflict), so we should elaborate on the explanatory text as [Custom, new ID (optional)]. Also, because you can't use the same custom ID for multiple jobs, can we add a clear (x) inside the custom ID field, so that after starting a replication job, users can easily clear just that field if they're reusing selected dbs or remote creds?

If possible, we should remove the verification step where the account password is entered. I'm not sure why it's a requirement. If we do keep it, the button should be "Start Replication" rather than "Continue Replication," and we should specify which password is required, because it could be interpreted as a remote account's password.

Icing, but after selecting from a dropdown, can we make focus automatically change to the following object?

If there are illegal characters (spaces, prohibited symbols), or if the db name starts with something other than a lower-case letter in new-database-name field, can we disable the replicate button? I was able to "trigger" a replication from a local db to a new one called "spa ces" which then failed.

Thanks for your work on this.

@benkeen
Copy link
Member Author

benkeen commented Jul 20, 2016

Thanks @justin-mcdavid-ibm! Just emailed you - sorry, didn't realize you responded here.

@benkeen benkeen force-pushed the replication-page-update branch 4 times, most recently from 4782891 to 6ee4d60 Compare July 24, 2016 03:11
@benkeen benkeen changed the title [WIP] Replication page update Replication page update Jul 24, 2016
@benkeen
Copy link
Member Author

benkeen commented Jul 24, 2016

Hey @garrensmith / @robertkowalski - took a thousand years, but could I get one of you to review this sucker? Hope all is well on your ends.

});
promise.fail(errorHandler);
},
promise.done(function () {
Copy link
Member

Choose a reason for hiding this comment

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

promise.done should be promise.then that is more semantic.

@garrensmith
Copy link
Member

Hey @benkeen this is making good progress. The basic functionality looks great. Just some code clean ups needed. We will need some nightwatch tests before we can merge this in.

@benkeen
Copy link
Member Author

benkeen commented Jul 26, 2016

Nice, thanks @garrensmith!

},

updateTypeahead: function () {
$(ReactDOM.findDOMNode(this.refs.field)).typeahead({
Copy link
Contributor

Choose a reason for hiding this comment

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

we are removing the jquery typeahed and are using react-select instead.

it is very easy to integrate, here is an example: https://github.com/apache/couchdb-fauxton/blob/5be261085761c5f1d0510f2d28117775ea3e0da2/app/addons/documents/components/jumptodoc.react.jsx

@robertkowalski
Copy link
Contributor

good work! agree with garren on smaller components.

using const would be great

@benkeen benkeen force-pushed the replication-page-update branch 2 times, most recently from 3639c15 to 2830341 Compare July 31, 2016 22:57
@benkeen
Copy link
Member Author

benkeen commented Aug 1, 2016

Thanks again for the feedback, guys. All fixed up except for the NW test. @garrensmith did you ever get NW running locally? Not sure what I'm missing but I get a "Connection refused! Is selenium server started?" error when running grunt nightwatch locally.

@garrensmith
Copy link
Member

garrensmith commented Aug 1, 2016

Hey @benkeen, yes its working. We have changed how it works though. It now runs through docker. So you need to install docker. See here and here

}
FauxtonAPI.navigate('/');
});
promise.fail(errorHandler);
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be changed to the .then

@benkeen
Copy link
Member Author

benkeen commented Aug 1, 2016

Too cool about Docker. Sheesh! Step away for 2 months and all this activity happens when you're gone. Can't say I approve! ;)

@benkeen benkeen force-pushed the replication-page-update branch 2 times, most recently from 34d462a to b974110 Compare August 14, 2016 20:29
@benkeen
Copy link
Member Author

benkeen commented Aug 15, 2016

Hey @robertkowalski or @garrensmith, I think I need an extra pair of eyes on this. This ticket has dragged onnnnn and on and I have supremely limited time to work on it. I just can't get the NW tests to run reliably. Running just the one replication.js file works fine both locally & on travis, but when running all of them, it fails every time on Travis. Possibly it's actually a couch error silently failing to create the new replicated DB...? Maybe you can see something I can't.

I left all the debugging code in.

@garrensmith
Copy link
Member

@benkeen you have done an awesome job. Thanks. I can take a look. I can replicate the failing test locally. I'll try and fix it.

@garrensmith garrensmith mentioned this pull request Aug 15, 2016
@garrensmith
Copy link
Member

@benkeen I've continuing the work in #761

@garrensmith
Copy link
Member

@benkeen this worked was merged in with the other PR. Could you close this PR.

@benkeen
Copy link
Member Author

benkeen commented Feb 23, 2017

No problem. Hope all's going well, @garrensmith!

@benkeen benkeen closed this Feb 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants