Move network selection to login module - Closes #89 #102

reyraa · 2017-04-06T10:03:09Z

Changes made:

The network select element and corresponding watcher is moved to login module
Instead added a label showing the selected network in top module
Tests are changed accordingly

Closes #89

coveralls · 2017-04-06T10:39:55Z

Coverage increased (+0.8%) to 83.013% when pulling 877eb84 on 89-move-network-selection-to-login into f22ae91 on development.

slaweet

Overall it looks very good.
Only issues:

network dropdown should have a default
end-to-end tests need to be updated, to see what's not passing run:

npm run e2e-test

slaweet · 2017-04-06T10:25:22Z

src/karma.conf.js

@@ -95,6 +95,7 @@ module.exports = function (config) {
    // If true, Karma captures browsers, runs the tests and exits
    singleRun: !opts.live,
    client: {
+      captureConsole: true,


This should go a separate commit

slaweet · 2017-04-06T10:25:38Z

src/webpack.config.babel.js

@@ -16,6 +16,7 @@ const PATHS = {
 };

 const common = {
+  devtool: 'source-map',


This should go a separate commit

coveralls · 2017-04-06T14:51:57Z

Coverage increased (+0.06%) to 82.315% when pulling 80a1a2a on 89-move-network-selection-to-login into bfe45a0 on development.

slaweet

Login screen layout is broken in Safari, but I'm not sure if we care

slaweet · 2017-04-07T06:16:45Z

src/spec/spec.js

@@ -120,22 +125,19 @@ function testAddress() {
 }

 function testPeer() {
-  login(masterAccount);
-  waitForElemAndCheckItsText('.peer md-select-value .md-text', 'localhost:4000');
+  expect(element.all(by.css('form md-select md-select-value span:first-child')).get(0).getText()).toEqual('Choose your network node');


This test doesn't work if you run only this one because you first need to launchApp();

slaweet · 2017-04-07T06:19:13Z

src/app/components/top/top.pug

-      md-select(ng-model='$ctrl.$peers.currentPeerConfig', aria-label='Peer')
-        md-optgroup(ng-repeat='(name, peers) in $ctrl.$peers.stack', ng-if='peers.length', label='{{ name }}')
-          md-option(ng-repeat='peer in peers', ng-value='peer') {{ peer.node }}{{ peer.port ? ':' + peer.port : '' }}
+      div.address.value {{ $ctrl.$peers.currentPeerConfig.node }}:{{ $ctrl.$peers.currentPeerConfig.port }}


.address class shouldn't be here, it causes this warning in e2e tests:

W/element - more than one element found for locator By(css selector, .address) - the first result will be used

karmacoma · 2017-04-11T10:37:08Z

Login screen layout is broken in Safari, but I'm not sure if we care

I think we should try to fix at least. Or at least find out why it is broken specifically on Safari.

coveralls · 2017-04-11T10:42:54Z

Coverage increased (+0.3%) to 82.524% when pulling 0729eda on 89-move-network-selection-to-login into 6b5f02a on development.

karmacoma

I have requested some changes.

The main problem I encountered is that upon selecting a node and logging in, the user is present with the following empty peer.

karmacoma · 2017-04-11T10:41:35Z

src/karma.conf.js

@@ -95,6 +95,7 @@ module.exports = function (config) {
    // If true, Karma captures browsers, runs the tests and exits
    singleRun: !opts.live,
    client: {
+      captureConsole: true,


karmacoma · 2017-04-11T10:41:54Z

src/package.json

@@ -52,7 +52,7 @@
    "jasmine-spec-reporter": "^3.2.0",
    "jit-grunt": "=0.10.0",
    "json-loader": "=0.5.4",
-    "karma": "^1.5.0",
+    "karma": "1.4.1",


1.5.0 has some bugs, according to the authors

karmacoma · 2017-04-11T10:43:40Z

src/spec/spec.js

@@ -83,11 +84,15 @@ function testNewAccount() {
  launchApp();

  element.all(by.css('.md-button.md-primary')).get(0).click();
+  /**
+   * To generate a random pattern of mousemove events, we're randomizing the x,y pairs that
+   * based on even/odd valkuds of i will locate right/left halves of screen


Spelling and grammar.

/** * To generate a random pattern of mousemove events, we are randomizing the x,y pairs that * based on even/odd values we locate the right/left halves of the screen */

karmacoma · 2017-04-11T10:44:22Z

src/test/components/login/login.spec.js

@@ -67,6 +72,29 @@ describe('Login controller', () => {
    controller.passphrase = '';
  });

+  describe('controller()', () => {
+    it('Should define a watcher for $ctrl.$peers.currentPeerConfig', () => {


karmacoma · 2017-04-11T10:44:29Z

src/test/components/login/login.spec.js

+      expect(spy).to.have.been.calledWith();
+    });
+
+    it('Should be able to change the active peer', () => {


karmacoma · 2017-04-11T10:44:43Z

src/webpack.config.babel.js

@@ -16,6 +16,7 @@ const PATHS = {
 };

 const common = {
+  devtool: 'source-map',


karmacoma · 2017-04-11T10:50:56Z

src/app/components/login/login.pug

    form(ng-submit='$ctrl.doTheLogin()')
      md-input-container.md-block(md-is-error='$ctrl.valid === 0')
-        label Enter your passphrase
+        label.select Choose your network node


I would phrase it as: Choose a peer.

Also, upon logging in, the selection is referred to as a "Peer", not a node.

I'm happy to see either or, as long as the terminology is consistently applied.

reyraa · 2017-04-11T12:56:47Z

I'm not sure if the interpolation issue with the active peer is related to this ticket. based on the templates I suppose such issue must happen in many cases. besides, we're using interpolation markup in many cases, taking in to account that it uses watchers, replacing it with ngBind, ngValue and other directives can help improve performance.

…om/LiskHQ/lisk-nano into 89-move-network-selection-to-login

coveralls · 2017-04-11T13:18:10Z

Coverage increased (+0.3%) to 82.524% when pulling c72a85d on 89-move-network-selection-to-login into 6b5f02a on development.

karmacoma · 2017-04-11T14:21:35Z

@alihaghighatkhah Can we remove colon from hostname when there is no port? Looks like so atm:

karmacoma · 2017-04-11T14:43:03Z

@Tosch110 @slaweet @alihaghighatkhah: If there is a failure to connect to a selected peer, then the current behaviour results in a another mainnet node being selected. Even if localhost or testnet peer is chosen. I think this should be fixed in: lisk-js. We should keep users on same network at least.

See: LiskArchive/lisk-elements#96

coveralls · 2017-04-13T08:36:13Z

Coverage increased (+0.3%) to 82.524% when pulling 42d8b65 on 89-move-network-selection-to-login into 6b5f02a on development.

coveralls · 2017-04-13T08:58:58Z

Coverage increased (+0.3%) to 82.524% when pulling 59fc3d3 on 89-move-network-selection-to-login into 6b5f02a on development.

move network selection to login module, closes #89

877eb84

karmacoma self-assigned this Apr 6, 2017

karmacoma added the enhancement label Apr 6, 2017

slaweet self-requested a review April 6, 2017 10:23

karmacoma changed the base branch from development to 1.0.0 April 6, 2017 10:41

slaweet reviewed Apr 6, 2017

View reviewed changes

karmacoma changed the base branch from 1.0.0 to development April 6, 2017 10:55

set one if the official nodes by default.

80a1a2a

slaweet reviewed Apr 7, 2017

View reviewed changes

karmacoma changed the title ~~move network selection to login module, closes #89~~ Move network selection to login module - Closes #89 Apr 7, 2017

move login-related tests to login.spec.js, increase test overage

f2a83cd

reyraa added 2 commits April 11, 2017 12:37

fix the warning in e2e tests related to address element

47035c6

Merge branch 'development' into 89-move-network-selection-to-login

0729eda

karmacoma suggested changes Apr 11, 2017

View reviewed changes

reyraa added 5 commits April 11, 2017 14:28

minor fixing in dependency versions

4e5edef

Add a hack to fix login [age in safari

83a1afb

Change 'network node' in title to 'peer'

cef335a

fix spelling issues in test files

c8e1571

hide the ng interpolation markup before binding the actual data

00c3b1f

Merge branch '89-move-network-selection-to-login' of https://github.c…

c72a85d

…om/LiskHQ/lisk-nano into 89-move-network-selection-to-login

reyraa force-pushed the 89-move-network-selection-to-login branch from 42d8b65 to c72a85d Compare April 13, 2017 08:51

remove colon when port is not defined

59fc3d3

karmacoma approved these changes Apr 13, 2017

View reviewed changes

karmacoma changed the base branch from development to 1.0.0 April 13, 2017 09:25

karmacoma merged commit a0cb4d7 into 1.0.0 Apr 13, 2017

karmacoma deleted the 89-move-network-selection-to-login branch April 13, 2017 09:25

karmacoma removed bug enhancement test update labels Aug 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move network selection to login module - Closes #89 #102

Move network selection to login module - Closes #89 #102

reyraa commented Apr 6, 2017 •

edited by karmacoma

Loading

coveralls commented Apr 6, 2017

slaweet left a comment

slaweet Apr 6, 2017

karmacoma Apr 11, 2017

slaweet Apr 6, 2017

karmacoma Apr 11, 2017

coveralls commented Apr 6, 2017 •

edited

Loading

slaweet left a comment

slaweet Apr 7, 2017

slaweet Apr 7, 2017

karmacoma commented Apr 11, 2017

coveralls commented Apr 11, 2017

karmacoma left a comment

karmacoma Apr 11, 2017

karmacoma Apr 11, 2017

reyraa Apr 11, 2017

karmacoma Apr 11, 2017

karmacoma Apr 11, 2017

karmacoma Apr 11, 2017

karmacoma Apr 11, 2017

karmacoma Apr 11, 2017

reyraa commented Apr 11, 2017

coveralls commented Apr 11, 2017

karmacoma commented Apr 11, 2017

karmacoma commented Apr 11, 2017 •

edited

Loading

coveralls commented Apr 13, 2017

coveralls commented Apr 13, 2017

Move network selection to login module - Closes #89 #102

Move network selection to login module - Closes #89 #102

Conversation

reyraa commented Apr 6, 2017 • edited by karmacoma Loading

coveralls commented Apr 6, 2017

slaweet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 6, 2017 • edited Loading

slaweet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karmacoma commented Apr 11, 2017

coveralls commented Apr 11, 2017

karmacoma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyraa commented Apr 11, 2017

coveralls commented Apr 11, 2017

karmacoma commented Apr 11, 2017

karmacoma commented Apr 11, 2017 • edited Loading

coveralls commented Apr 13, 2017

coveralls commented Apr 13, 2017

reyraa commented Apr 6, 2017 •

edited by karmacoma

Loading

coveralls commented Apr 6, 2017 •

edited

Loading

karmacoma commented Apr 11, 2017 •

edited

Loading