krakenjs / kraken-example-with-passport

An example integrating kraken with passport authentication
53 stars 33 forks source link

AdminModel? (also, unused `auth` reference?) #16

Open dennishall1 opened 8 years ago

dennishall1 commented 8 years ago

The AdminModel appears to be meaningless indirection. I removed it, and I also removed the unused reference to auth that is in controllers/index.js.

My controllers/index.js file now looks like this (notice that /admin simply uses profilemodel, because the admin is just a user who happens to have a role of admin):

'use strict';

var IndexModel = require('../models/index'),
    ProfileModel = require('../models/profile');

module.exports = function (router) {

    var indexmodel = new IndexModel();
    var profilemodel = new ProfileModel();

    router.get('/', function (req, res) {
        res.render('index', indexmodel);
    });

    router.get('/profile', function(req, res) {
        res.render('profile', profilemodel);
    });

    router.get('/admin', function(req, res) {
        res.render('admin', profilemodel);
    });

    /**
     * Allow the users to log out
     */
    router.get('/logout', function (req, res) {
        req.logout();
        res.redirect('/login');
    });

};
aredridel commented 8 years ago

Tidy! Want to make a PR for discussion in context?

aredridel commented 8 years ago

(lots of this code is not minimal, because it's trying to make space to show user-defined code hanging off of the structure, but some of it is pretty meaningless)

dennishall1 commented 8 years ago

I may get around to making a PR for it. To me, what's slightly confusing about the AdminModel and IndexModel .. they probably shouldn't be models, but instances of a PageModel (doesn't exist yet). Or at least, use the word page in the model name when referring to a page model, to help disambiguate. (UserModel is not a model for the /user page, there is no such page, but AdminModel is a model for the Admin page, and not a model of an Admin-kind-of-user.)

grawk commented 8 years ago

I might suggest using "view" instead of "page" but agree with the general thrust of your comment.