Skip to content

Commit

Permalink
Fix bug with votes count (#1585)
Browse files Browse the repository at this point in the history 
* Script up to down or down to up improperly counts script votes and messes up good/bad bar. Shouldn't affect Rating in Script or Group but will recheck a few.
* At the same time move to MVC *(Currently isolated to Script e.g. parallel change)*... Fixes TODO in routes.js
* Some styleguide.md conformance
* Simplify some views. e.g. turn some things off when they aren't able to be used. Save b/w and possibly improve understanding for most
* Still would prefer returning `err` *(aErr)* with libs but that's probably a separate PR since there are still inconsistent return callback parms
* Some filling in for #1548
* Some additions for #430. e.g. we want to know if a DB action fails currently

NOTES:
* Structure mirrored and altered from `flagsLib`
Applies to #262 and Active Maintainer override (Sorry sizzle but needed to be done so it doesn't mess up other areas down the line)
* Will run through scripts and mitigate any script `votes` counts.

Auto-merge
  • Loading branch information
@Martii
Martii authored Feb 10, 2019
1 parent 36d39c4 commit bf885af
Show file tree
Hide file tree
Showing 10 changed files with 390 additions and 163 deletions.
2 changes: 2 additions & 0 deletions controllers/flag.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ var formidable = require('formidable');

//--- Model inclusions
var Flag = require('../models/flag').Flag;

var User = require('../models/user').User;
var Script = require('../models/script').Script;

Expand Down Expand Up @@ -109,6 +110,7 @@ exports.flag = function (aReq, aRes, aNext) {
});

});

break;
case 'users':
username = aReq.params[1];
Expand Down
160 changes: 10 additions & 150 deletions controllers/script.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ var SPDX = require('spdx-license-ids');
var Discussion = require('../models/discussion').Discussion;
var Group = require('../models/group').Group;
var Script = require('../models/script').Script;
var Vote = require('../models/vote').Vote;

//--- Controller inclusions
var scriptStorage = require('./scriptStorage');
Expand All @@ -29,6 +28,7 @@ var getFlaggedListForContent = require('./flag').getFlaggedListForContent;

var isSameOrigin = require('../libs/helpers').isSameOrigin;

var voteLib = require('../libs/vote');
var flagLib = require('../libs/flag');
var removeLib = require('../libs/remove');

Expand Down Expand Up @@ -233,35 +233,17 @@ var getScriptPageTasks = function (aOptions) {
aOptions.voteDownUrl = voteUrl + '/down';
aOptions.unvoteUrl = voteUrl + '/unvote';

aOptions.voteable = false;
aOptions.votedUp = false;
aOptions.votedDown = false;

// Can't vote when not logged in or when user owns the script.
if (!authedUser || aOptions.isOwner) {
aCallback();
return;
}

Vote.findOne({
_scriptId: script._id,
_userId: authedUser._id
}, function (aErr, aVoteModel) {
// WARNING: No err handling

aOptions.voteable = !script.isOwner;

if (aVoteModel) {
if (aVoteModel.vote) {
aOptions.votedUp = true;
voteLib.voteable(script, authedUser,
function (aCanVote, aAuthor, aVote) {
if (aVote) {
aOptions.votedUp = aVote.vote === true;
aOptions.votedDown = aVote.vote === false;
aOptions.canVote = true;
} else {
aOptions.votedDown = true;
aOptions.canVote = aCanVote;
}
}

aCallback();
});

aCallback();
});
});

// Setup the flagging UI
Expand Down Expand Up @@ -506,125 +488,3 @@ exports.edit = function (aReq, aRes, aNext) {
}
});
};

// Script voting
exports.vote = function (aReq, aRes, aNext) {
//
var uri = aReq._parsedUrl.pathname.split('/');
var vote = aReq.params.vote;
var unvote = false;

var isLib = aReq.params.isLib;
var installNameBase = scriptStorage.getInstallNameBase(aReq);

// ---
if (uri.length > 5) {
uri.pop();
}
uri.shift();
uri.shift();
uri = '/' + uri.join('/');

if (vote === 'up') {
vote = true;
} else if (vote === 'down') {
vote = false;
} else if (vote === 'unvote') {
unvote = true;
} else {
aRes.redirect(uri);
return;
}

Script.findOne({
installName: scriptStorage.caseSensitive(installNameBase +
(isLib ? '.js' : '.user.js'))
}, function (aErr, aScript) {
//
var authedUser = aReq.session.user;

// ---
if (aErr || !aScript) {
aRes.redirect(uri);
return;
}

Vote.findOne({ _scriptId: aScript._id, _userId: authedUser._id },
function (aErr, aVoteModel) {
// WARNING: No err handling

var votes = aScript.votes || 0;
var flags = 0;
var oldVote = null;

function saveScript() {
if (!flags) {
aScript.save(function (aErr, aScript) {
// WARNING: No err handling

var script = null;

if (vote === false) {
script = modelParser.parseScript(aScript);

// Gently encourage browsing/creating an issue with a down vote
aRes.redirect(script.scriptIssuesPageUri);
} else {
aRes.redirect(uri);
}
});
return;
}

flagLib.getAuthor(aScript, function (aAuthor) {
flagLib.saveContent(Script, aScript, aAuthor, flags, false,
function (aFlagged) {
aRes.redirect(uri);
});
});
}

if (!aScript.rating) {
aScript.rating = 0;
}

if (!aScript.votes) {
aScript.votes = 0;
}

if (authedUser._id == aScript._authorId || (!aVoteModel && unvote)) {
aRes.redirect(uri);
return;
} else if (!aVoteModel) {
aVoteModel = new Vote({
vote: vote,
_scriptId: aScript._id,
_userId: authedUser._id
});
aScript.rating += vote ? 1 : -1;
aScript.votes = votes + 1;
if (vote) {
flags = -1;
}
} else if (unvote) {
oldVote = aVoteModel.vote;
aVoteModel.remove(function () {
aScript.rating += oldVote ? -1 : 1;
aScript.votes = votes <= 0 ? 0 : votes - 1;
if (oldVote) {
flags = 1;
}
saveScript();
});
return;
} else if (aVoteModel.vote !== vote) {
aVoteModel.vote = vote;
aScript.rating += vote ? 2 : -2;
flags = vote ? -1 : 1;
}

aVoteModel.save(saveScript);
}
);
});
};
97 changes: 97 additions & 0 deletions controllers/vote.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,111 @@ var isDbg = require('../libs/debug').isDbg;
//

//--- Dependency inclusions
var formidable = require('formidable');

//--- Model inclusions
var Script = require('../models/script').Script;

//--- Controller inclusions
var scriptStorage = require('./scriptStorage');


//--- Library inclusions
var voteLib = require('../libs/vote');

var modelParser = require('../libs/modelParser');

var statusCodePage = require('../libs/templateHelpers').statusCodePage;

//--- Configuration inclusions

//---

// Controller for Script voting
exports.vote = function (aReq, aRes, aNext) {
var form = null;

// Check to make sure multipart form data submission header is present
if (!/multipart\/form-data/.test(aReq.headers['content-type'])) {
statusCodePage(aReq, aRes, aNext, {
statusCode: 400,
statusMessage: 'Missing required header.'
});
return;
}

form = new formidable.IncomingForm();
form.parse(aReq, function (aErr, aFields) {
// WARNING: No err handling

var vote = aFields.vote;
var unvote = false;

var type = aReq.params[0];
var isLib = null;

var installNameBase = null;
var authedUser = aReq.session.user;

switch (vote) {
case 'up':
// fallthrough
case 'down':
// fallthrough
case 'un':
break;
default:
statusCodePage(aReq, aRes, aNext, {
statusCode: 400,
statusMessage: 'Missing required field value.'
});
return;
}

switch (type) {
case 'libs':
isLib = true;
// fallthrough
case 'scripts':
aReq.params.username = aReq.params[2];
aReq.params.scriptname = aReq.params[3]

installNameBase = scriptStorage.getInstallNameBase(aReq);

Script.findOne({
installName: scriptStorage.caseSensitive(installNameBase +
(isLib ? '.js' : '.user.js'))
}, function (aErr, aScript) {
var fn = voteLib[vote + 'vote'];

// ---
if (aErr || !aScript) {
aRes.redirect((isLib ? '/libs/' : '/scripts/') + scriptStorage.getInstallNameBase(
aReq, { encoding: 'uri' }));
return;
}

fn(aScript, authedUser, function (aErr) {
var script = null;

if (vote === 'down') {
script = modelParser.parseScript(aScript);

// Gently encourage browsing/creating an issue with a down vote
aRes.redirect(script.scriptIssuesPageUri);

} else {
aRes.redirect((isLib ? '/libs/' : '/scripts/') + scriptStorage.getInstallNameBase(
aReq, { encoding: 'uri' }));
}
});

});

break;
default:
aNext();
return;
}
});
};
7 changes: 6 additions & 1 deletion libs/flag.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,10 @@ function getFlag(aModel, aContent, aUser, aCallback) {
function getAuthor(aContent, aCallback) {
User.findOne({ _id: aContent._authorId }, function (aErr, aAuthor) {
// Content without an author shouldn't exist
if (aErr || !aAuthor) { return aCallback(null); }
if (aErr || !aAuthor) {
aCallback(null);
return;
}

aCallback(aAuthor);
});
Expand Down Expand Up @@ -152,6 +155,8 @@ function flag(aModel, aContent, aUser, aAuthor, aReason, aCallback) {
});

flag.save(function (aErr, aFlag) {
// WARNING: No err handling

if (!aContent.flags) {
aContent.flags = {};
}
Expand Down
4 changes: 4 additions & 0 deletions libs/modelParser.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,10 @@ var parseScript = function (aScript) {
// Urls: Moderation
script.scriptRemovePageUrl = '/remove' + (script.isLib ? '/libs/' : '/scripts/') +
script.installNameSlugUrl;

script.scriptVotePageUrl = '/vote' + (script.isLib ? '/libs/' : '/scripts/') +
script.installNameSlugUrl;

script.scriptFlagPageUrl = '/flag' + (script.isLib ? '/libs/' : '/scripts/') +
script.installNameSlugUrl;

Expand Down
Loading

0 comments on commit bf885af

Please sign in to comment.