New: Added Admin page (fixes #30)

[vegetableman]

fixes #30

Added .gittouch

If src/admin/public/js/app.js doesn't exist, the site fails to load (404, can't GET /js/app.js). Reproduced on OS X and Debian Wheezy.

But app.js is still not part of the repo. Should I add it on gitignore ? or a build script?

[taoeffect]

But app.js is still not part of the repo. Should I add it on gitignore ? or a build script?

So this was covered in a previous comment somewhere, basically:

• For this PR, app.js shouldn't be anywhere in the repo.
• We will generate it for each release for dist/app.js and add that to the repo.
• Git should always ignore src/admin/public/js/*.js
• Important: if both files exist, the one in src/admin/public/js must take precedence.

Previous conversation in PR #62.

[taoeffect]

Note that with this latest PR I'm still experiencing the bug where is src/admin/public/js/app.js doesn't exist dist/app.js isn't used.

[vegetableman]

As i mentioned in my comment, It requires the usage of a stringifier. on passing the config:-

{
    log: {
        level: 'debug',
        colors: true,
        pretty: true,
        timestamp: true
    },
    dns: {
        port: 5333,
        host: '0.0.0.0',
        externalIP: '192.168.1.100',
        oldDNSMethod: 0,
        oldDNS: {
            address: '8.8.8.8',
            port: 53,
            type: 'udp'
        }
    },
    https: {
        port: 443,
        host: '0.0.0.0',
        internalTLSPort: 15001,
        key: './key.pem',
        cert: './cert.pem'
    },
    http: {
        port: 8088,
        tlsPort: 4443,
        host: '0.0.0.0'
    }
}

it returns : log = [object Object]\ndns = [object Object]\nhttps = [object Object]\nhttp = [object Object] which is not useful.

security reasons.

you lost me here. could you please elaborate on this?.

[taoeffect]

it returns : log = [object Object]\ndns = [object Object]\nhttps = [object Object]\nhttp = [object Object] which is not useful.

I see, for nested ones it doesn't do it.

OK, but config.coffee already replaced .stringify with the stringifier that does this properly:

props.stringify = _.partialRight props.stringify, fileFormatOpts

So just use that.

you lost me here. could you please elaborate on this?.

Each dependency is more code that could have security problems in it and requires auditing.

[taoeffect]

@vegetableman OK, so I tested your latest code on OS X and Linux. Now if src/admin/public/js/app.js is missing it works, so :+1: on fixing that! :smile:

Next, I tested what would happen if /etc/dnschain/dnschain.conf didn't exist. Here I found a bug: it tries to save to /etc/dnschain/dnschain.conf instead of $HOME/.dnschain.conf, which is what it should save to, and it should get this path from here (see config.coffee):

    if process.env.HOME?
        dnscConf = path.join process.env.HOME, ".#{appname}.conf"
        unless fs.existsSync dnscConf
            dnscConf = path.join process.env.HOME, ".#{appname}", "#{appname}.conf"
        nconf.file 'user', {file: dnscConf, format: props}

The reason it should save to $HOME/.dnschain.conf (or whatever the value of nconf.file.user is) is because it mostly likely will be able to write to that file, whereas it's a high probability that DNSChain isn't running as root (we recommend that, after all) and so it won't be able to write to /etc/dnschain/dnschain.conf.

Fix that, and I think it should be good to merge (I'll still need to test it again). Although not necessary for merging, you would save us some hassle if you could also use properties instead of ini to stringify on save.

[vegetableman]

Pushed with properties fix. Check again. If it still saves to etc, you may not have HOME env set.

[taoeffect]

OK, preliminary testing indicates this may be good to merge! I have to head out right now and will do final testing tomorrow, only thing I noticed is that the settings page takes a long time to load, and that's because app.js is 1MB!

Though not a requirement, if you can figure out how to bring down the size of this file before I finish my review tomorrow it would be appreciated (would greatly improve the user experience).

Ideally the page should load instantly (300ms or less). Right now it's taking over 4 seconds.

[vegetableman]

Added an uglify task for dist/app.js.

​

On Mon, Dec 8, 2014 at 7:35 AM, Greg Slepak notifications@github.com wrote:

OK, preliminary testing indicates this may be good to merge! I have to head out right now and will do final testing tomorrow, only thing I noticed is that the settings page takes a long time to load, and that's because app.js is 1MB!

[image: screen shot 2014-12-07 at 6 04 37 pm] https://cloud.githubusercontent.com/assets/138706/5333930/8f27a770-7e3b-11e4-96e4-8f3a462f3bbd.jpg

Though not a requirement, if you can figure out how to bring down the size of this file before I finish my review tomorrow it would be appreciated (would greatly improve the user experience).

— Reply to this email directly or view it on GitHub https://github.com/okTurtles/dnschain/pull/69#issuecomment-65967868.

[taoeffect]

OK, great job @vegetableman! The load time has been reduced by about half. Still not as snappy as I'd like it to be but tolerable, and I'll create a separate issue for getting total page load down to ~300ms.

You've really done a fantastic job. I like your design sense, the admin page looks very sexy. 😄👍

Merging the PR! Thank you for your fantastic contribution! You've also been added to the list of contributors on the README. Let me know if you'd like me to use some other link there for you!

[vegetableman]

Thanks @taoeffect for the detailed feedback and review. Got to polish my rusty coffeescript skills. On the size, If IE 8 support is not required, jquery could be removed. http://youmightnotneedjquery.com/