New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor flags to become a first-class entity #5282

Merged
merged 59 commits into from Dec 19, 2016

Conversation

Projects
None yet
3 participants
@julianlam
Copy link
Member

julianlam commented Dec 12, 2016

This is the pull request re: #5232

Some things to consider:

Public review encouraged as well!

julianlam added some commits Nov 24, 2016

@julianlam

This comment has been minimized.

Copy link
Member

julianlam commented Dec 12, 2016

Misty... 😆

@julianlam julianlam referenced this pull request Dec 12, 2016

Closed

Flags System Re-build #5232

32 of 36 tasks complete

@julianlam julianlam removed the invalid label Dec 12, 2016

switch (payload.type) {
case 'post':
async.parallel({
editable: async.apply(privileges.posts.canEdit, payload.id, payload.uid)

This comment has been minimized.

@barisusakli

barisusakli Dec 13, 2016

Member

dont need parallel here

case 'user':
async.parallel({
editable: async.apply(privileges.users.canEdit, payload.uid, payload.id)
}, function (err, subdata) {

This comment has been minimized.

@barisusakli

barisusakli Dec 13, 2016

Member

dont need parallel here

});
}, next);
}
], function (err, flags) {

This comment has been minimized.

@barisusakli

barisusakli Dec 13, 2016

Member

can just replace this with callback

};

Flags.getTarget = function (type, id, uid, callback) {
switch (type) {

This comment has been minimized.

@barisusakli

barisusakli Dec 13, 2016

Member

no default case for this switch

This comment has been minimized.

@julianlam

julianlam Dec 13, 2016

Member

👍 Added one now.

},
async.apply(Flags.get)
], callback);
// if (!parseInt(uid, 10) || !reason) {

This comment has been minimized.

@barisusakli

barisusakli Dec 13, 2016

Member

lots of commented out code :tmi:

};

Flags.exists = function (type, id, uid, callback) {
db.isObjectField('flagHash:flagId', [type, id, uid].join(':'), callback);

This comment has been minimized.

@barisusakli

barisusakli Dec 13, 2016

Member

can flagHash:flagId grow beyond 16mb? Might be better to store is as zset if it can. [type, id, uid].join(':') would be the value and flagId would be the score

This comment has been minimized.

@julianlam

julianlam Dec 13, 2016

Member

Unlikely since 16mb is quite large, but possible. I've changed it accordingly.


describe('.create()', function () {
it('should create a flag and return its data', function (done) {
Flags.create('post', 1, 1, 'Test flag', function (err, flagData) {

This comment has been minimized.

@barisusakli

barisusakli Dec 13, 2016

Member

tests are not using socket.io/flags

This comment has been minimized.

@julianlam
@julianlam

This comment has been minimized.

Copy link
Member

julianlam commented Dec 13, 2016

Review again @barisusakli ? 😄

} else {
return;
}
}).filter(Boolean).join('&');

This comment has been minimized.

@barisusakli

barisusakli Dec 14, 2016

Member

This can be replaced with

var payload = filtersEl.serializeArray();
var qs = $.param(payload);
ajaxify.go('flags?' + qs);

http://api.jquery.com/jquery.param/

This comment has been minimized.

@julianlam

julianlam Dec 14, 2016

Member

Whoa nice, TIL...

var adminFlagsController = require('./admin/flags');
var flags = require('../flags');
var analytics = require('../analytics');
// var adminFlagsController = require('./admin/flags');

This comment has been minimized.

@barisusakli

@julianlam julianlam merged commit 283ae56 into develop Dec 19, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on develop at 79.468%
Details
licence/cla Contributor License Agreement is signed.
Details

@julianlam julianlam deleted the flagging-refactor branch May 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment