-
Notifications
You must be signed in to change notification settings - Fork 290
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
0.4.0 blake #491
0.4.0 blake #491
Conversation
* @static | ||
* @method getRoleToDisplayNameMap | ||
* @param {Localization} ls | ||
* @return {Object} | ||
*/ | ||
SecurityService.getRoleToDisplayNameMap = function(ls) { | ||
if (util.isFunction(ls)) { | ||
if (util.isFunction(ls.get)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this get changed? There were some instances where the localization service wasn't getting passed. The if check is there to provide support for backward compatiablity. This change will cause a null pointer error in those cases. Please change it back to if (util.isFunction(ls)) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was returning false in my tests, so I moved it to the get function and it worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K
@@ -147,7 +147,8 @@ module.exports = function(pb) { | |||
'ACCESS_ADMINISTRATOR': ls.get('ACCESS_ADMINISTRATOR'), | |||
}; | |||
} | |||
return null; | |||
// Return an empty object instead of null, so things won't break after return | |||
return {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null is here so that the code explicitly has to handle cases where it is calling the function in a deprecated manner. Was there a specific use case where it needed the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code wasn't handling a null return, it was going straight to acting upon an object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What code exactly? That code that calls should be providing the Localization instance. Returning null was intentional to highlight code that used the call incorrectly.
UserService.prototype.getEditorSelectList(currId, cb) { | ||
pb.log.warn('UserService: getEditorSelectList is deprecated. Use getWriterOrEditorSelectList instead'); | ||
this.getWriterOrEditorSelectList(currId, cb); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the function signature back. I did this because we don't know if someone was already using it. I'd like to a take a deprecation strategy where possible to reduce churn on our users.
}); | ||
}); | ||
if(!self.article.author) { | ||
self.article.author = self.session.authentication.user._id.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is uncomfortably verbose but we need to refer to the _id
paramter via:
self.session.authentication.user[pb.DAO.getIdField()].toString()
This helps reduce churn to support multiple DBs in the future.
@@ -72,7 +92,7 @@ module.exports = function(pb) { | |||
|
|||
for(i = 0; i < data.article.article_media.length; i++) { | |||
for(j = 0; j < data.media.length; j++) { | |||
if(pb.DAO.areIdsEqual(data.media[j][pb.DAO.idField()], data.article.article_media[i])) { | |||
if(pb.DAO.areIdsEqual(data.media[j][pb.DAO.getIdField()], data.article.article_media[i])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. I botched that.
Conflicts: plugins/pencilblue/controllers/api/admin/site_settings/email/send_test.js public/js/angular/filters/parsable_date.js
No description provided.