Skip to content
This repository has been archived by the owner on Dec 15, 2020. It is now read-only.

Fixed the sample that goes with salti #7

Merged
merged 8 commits into from
Jul 25, 2016
Merged

Conversation

andrew-aladev
Copy link
Contributor

@andrew-aladev andrew-aladev commented Jul 20, 2016

fixes #4

Fixed sample, fixed acl schema, added strip slashes option and its test.


This change is Reviewable

@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage increased (+1.4%) to 95.763% when pulling e20e201 on vNext_aaladev_4 into 78a8bc6 on master.

@yaronyg
Copy link
Member

yaronyg commented Jul 20, 2016

Reviewed 16 of 16 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


.gitignore, line 42 [r1] (raw file):

db/*
sample/db/*
!sample/db/.gitkeep

gitkeep is a made up convention as far as I'm aware. If we need a particular directory to exist than rather than shoving it into the repo can't we just create it as part of an install script?


sample/server.js, line 18 [r1] (raw file):

var webAppPort = normalizePort(process.env.PORT || '3000');

var serverCommonOptions = {

If we are using psk then we don't need the public key certs anymore. They were required in the old days because of a bug. So we can pull out the PEM and files, the code to load them, the assigns, etc.


test/test-trailing-slash.js, line 118 [r1] (raw file):

        .expect(401, done);
    })
    it('/bar/ should be 200', function(done) {

I would throw in a test using a URL encoded / just to be paranoid and make sure we don't treat it as part of the path segment.


Comments from Reviewable

@yaronyg yaronyg assigned andrew-aladev and unassigned yaronyg Jul 20, 2016
@coveralls
Copy link

coveralls commented Jul 21, 2016

Coverage Status

Coverage increased (+1.4%) to 95.763% when pulling 8bcfb3f on vNext_aaladev_4 into 78a8bc6 on master.

@coveralls
Copy link

coveralls commented Jul 21, 2016

Coverage Status

Coverage increased (+1.4%) to 95.763% when pulling 1287a83 on vNext_aaladev_4 into 78a8bc6 on master.

@yaronyg
Copy link
Member

yaronyg commented Jul 21, 2016

@andrew-aladev - It would be good if we could the areas of the code that aren't being tested (e.g. https://coveralls.io/builds/7100824/source?filename=lib%2Findex.js) covered with the obvious exception of line 60.


Reviewed 4 of 5 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


sample/server.js, line 25 [r3] (raw file):

var pouchDBName = 'db';
if (! fs.existsSync('./' + pouchDBName)){

This code shouldn't be necessary. PouchDB should create the directory for the DB.


sample/server.js, line 28 [r3] (raw file):

    fs.mkdirSync('./' + pouchDBName);
}
var pbsetup = PouchDB.defaults({ prefix: './' + pouchDBName + '/' });

Prefix is supposed to identify the directory where the file can be found, not the file name itself. PouchDB will handle creating any directories it needs to hold the database. Prefix is just to tell it where to put that directory if you don't want it to use the current directory.


Comments from Reviewable

@yaronyg yaronyg assigned andrew-aladev and unassigned yaronyg Jul 21, 2016
@coveralls
Copy link

coveralls commented Jul 21, 2016

Coverage Status

Coverage increased (+1.4%) to 95.763% when pulling 41ccfd0 on vNext_aaladev_4 into 78a8bc6 on master.

@coveralls
Copy link

coveralls commented Jul 22, 2016

Coverage Status

Coverage increased (+4.8%) to 99.153% when pulling 96fceb7 on vNext_aaladev_4 into 78a8bc6 on master.

@coveralls
Copy link

coveralls commented Jul 22, 2016

Coverage Status

Coverage increased (+4.8%) to 99.153% when pulling eafb5b4 on vNext_aaladev_4 into 78a8bc6 on master.

@coveralls
Copy link

coveralls commented Jul 22, 2016

Coverage Status

Coverage increased (+5.7%) to 100.0% when pulling 983b3a9 on vNext_aaladev_4 into 78a8bc6 on master.

@andrew-aladev
Copy link
Contributor Author

andrew-aladev commented Jul 22, 2016

Pouch DB will create dir for its db. But it will throw an exception If its prefix dir does not exist.
Many directories will be auto created. I think previous developer used prefix to hide these auto created directories. Should we remove prefix directory? Thank you.

@yaronyg
Copy link
Member

yaronyg commented Jul 22, 2016

:lgtm:

I think the prefix directory is fine.


Reviewed 1 of 3 files at r4, 1 of 3 files at r5, 2 of 3 files at r6, 3 of 3 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@yaronyg yaronyg removed their assignment Jul 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix the sample that goes with salti
3 participants