CLI registration #530

nono · 2017-02-17T14:27:01Z

No description provided.

sebn · 2017-02-17T14:50:26Z

src/bin/cli.js

@@ -79,18 +78,17 @@ program
 program
  .command('sync')
  .description('Synchronize the local filesystem and the remote cozy')
-  .option('-k, --insecure', 'Turn off HTTPS certificate verification.')


Is this not something self-hosted users will want?

Probably, but it is a priority for the short term, and the code we had for that will no longer work

sebn · 2017-02-17T14:55:49Z

src/bin/repl.js

@@ -12,7 +12,7 @@ The following objects are available:
  app     The cozy-desktop app
  config  Your active cozy-desktop configuration`)

-if ((config.deviceName != null) && (config.url != null) && (config.path != null)) {
+if (config.url) {


app.config.isValid()?

sebn · 2017-02-17T15:02:05Z

src/config.js

-    } else {
-      return false
+  saveMode (mode) {
+    let old = this.config.mode


sebn · 2017-02-17T15:03:42Z

src/config.js

+    if (old === mode) {
+      return true
+    } else if (old) {
+      throw new Error('Incompatible mode')


throw new Error(`Once you set mode to "${old}", you cannot switch to "${mode}"`)

sebn · 2017-02-17T15:06:48Z

src/config.js

+
+  save (key, value) {
+    this.config.credentials[key] = value
+    return Promise.resolve(value)


Have some kind Config#oauthStorage() return the expected interface? (not sure how you expect to use it)

The goal is to persist what cozy-client-js puts in this storage in the config file (it is the client informations and the OAuth tokens).

(and move save / load / ... to the new object)

Was this the reason for the persist() method not to be named save()?

sebn · 2017-02-17T15:07:43Z

src/config.js

 }
+
+export default Config


export default class Config directly?

sebn · 2017-02-17T15:10:20Z

test/unit/config.js

-        url: 'none'
-      }
-      this.config.save()
+      this.config.setCozyUrl('http://cozy.local:8080/')


const url = 'http://cozy.local:8080/'

sebn · 2017-02-17T15:12:09Z

test/unit/config.js

-      process.argv = []
-      let name = this.config.getDefaultDeviceName()
-      name.should.equal('tester')
+      conf.getCozyUrl().should.equal('http://cozy.local:8080/')


I started using the following pattern in new code:

should(conf.getCozyUrl()).equal(...)

Pros:

no monkey-patching involved...

... which makes it more Flow-friendly

assertion error in case getCozyUrl() returns undefined, instead of undefined-has-no-should-property

Please let me now in case you disagree, so I stop using it everywhere.

sebn · 2017-02-17T15:18:31Z

src/bin/cli.js

-      if (err) { return process.exit(1) }
-    })
-  } else {
+  if (!app.config.hasClient()) {


app.config.isValid()

sebn · 2017-02-17T15:20:51Z

src/bin/repl.js

@@ -3,7 +3,7 @@ import { start } from 'repl'
 import App from '../app'

 const app = new App(process.env.COZY_DESKTOP_DIR)
-const config = app.config.getDevice()
+const config = app.config.config


Do you plan to remove the multi-config support? (just out of curiosity)

Well, there were no multi-config support. I had tried it, but it was too error-prone and even if the config file can have several "devices", only the first one was really used.

sebn · 2017-02-17T16:44:56Z

src/config.js

-    }
+  // Get the path on the local file system of the synchronized folder
+  getSyncPath () {
+    return this.config.path


I'm not sure whether es getters/setters are supported... Would it make sense to use them?

I mean:

get syncPath() { ... } set syncPath(path) { ... }

sebn · 2017-02-17T16:47:30Z

package.json

@@ -2,7 +2,7 @@
  "name": "cozy-desktop",
  "version": "0.16.0",
  "description": "File Synchronization Client for Cozy Cloud",
-  "homepage": "https://github.com/cozy-labs/cozy-desktop",
+  "homepage": "https://docs.cozy.io/en/mobile/desktop.html",


Shouldn't we just disable the cozy-desktop github pages if they are not supposed to be the main entry point for users?

We try to keep documentation for users on cozy.io, and on github for people who want to contribute to our projects

codecov-io · 2017-02-17T16:50:40Z

Codecov Report

Merging #530 into master will decrease coverage by -0.03%.
The diff coverage is 51.16%.

@@            Coverage Diff             @@
##           master     #530      +/-   ##
==========================================
- Coverage   71.22%   71.19%   -0.03%     
==========================================
  Files          18       19       +1     
  Lines        1449     1399      -50     
==========================================
- Hits         1032      996      -36     
+ Misses        417      403      -14

Impacted Files	Coverage Δ
src/app.js	`7.54% <ø> (-13.51%)`	❌
src/remote/cozy.js	`100% <100%> (ø)`	✅
src/remote/index.js	`68.96% <100%> (-2.01%)`	❌
src/local/index.js	`93.61% <100%> (ø)`	✅
src/remote/registration.js	`45.83% <45.83%> (ø)`
src/config.js	`91.89% <90.62%> (+5.22%)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0afbf22...89242cd. Read the comment docs.

sebn · 2017-02-17T20:54:51Z

src/remote/registration.js

+    })
+    return cozy.authorize().then(
+      (res) => res,
+      (err) => { throw err }


Why not just .catch(err => { throw err }) ?
Also, since process() is not async and it returns the promise, is the catch block even needed?

You are right. I've added them to console.log this step. Then I removed the console.log, but the then() can also be removed.

sebn · 2017-02-17T21:49:12Z

test/unit/remote/cozy.js

@@ -14,25 +15,37 @@ const cozyStackDouble = new CozyStackDouble()
 describe('RemoteCozy', function () {
  before(() => cozyStackDouble.start())
  beforeEach(deleteAll)
+  before('instanciate config', configHelpers.createConfig)
+  before('register OAuth client', configHelpers.registerClient)
+  after('clean config directory', configHelpers.cleanConfig)


Won't mocha use the function name as a description anyway? (assuming it is named)

sebn · 2017-02-17T22:08:37Z

Looks good to me overall.

The only thing really bothering me is the Config class exposing oauth-related save / load / delete methods:

I assume no cozy-desktop code will ever use them
save() / load() can be used to change / read whatever property of the class
they are not of the same abstraction level (hence the name conflict with persist())

nono · 2017-02-20T13:30:37Z

For the config class that also exports the storage interface, I'm OK with you that it should be refactored. But I'd prefer to keep that for later. I'd like to explore token refresh and device revocation before.

sebn · 2017-02-20T14:05:54Z

But I'd prefer to keep that for later

👌

nono added 5 commits February 17, 2017 14:30

Comment some unused code (temporary)

3f8af40

Refactor the config

8bb2e2a

Add a first version with CLI registration

62f5086

Fix some tests

f13cc45

Fix app.synchronize

758ba8f

nono requested a review from sebn February 17, 2017 14:27