Skip to content
This repository has been archived by the owner on Jun 27, 2019. It is now read-only.

Fix #171 #172

Merged
merged 2 commits into from
Oct 10, 2018
Merged

Fix #171 #172

merged 2 commits into from
Oct 10, 2018

Conversation

roschaefer
Copy link
Contributor

I still don't understand why this does not happen on the server...

@@ -13,8 +13,11 @@ module.exports = function excludeBlacklisted() {
if (usersettings.total <= 0){
return hook;
}
const blacklist = usersettings.data[0].blacklist;
hook.params.query.userId = {$nin: blacklist};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the query.userId got overwritten

@roschaefer
Copy link
Contributor Author

roschaefer commented Oct 9, 2018

for some strange reason hook.params.query.userId gets set to

{ '$ne': '5bb78a5c1afa23002c128dfa' }

Where does that come from? I cannot see any query.userId in the code base?

@roschaefer roschaefer requested a review from a user October 9, 2018 21:38
@roschaefer roschaefer force-pushed the fix_171 branch 3 times, most recently from 9962068 to 65f907d Compare October 9, 2018 23:41
@Lulalaby Lulalaby self-requested a review October 10, 2018 08:28
Copy link
Contributor

@Lulalaby Lulalaby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seem's ok, on fast look

@appinteractive
Copy link
Member

appinteractive commented Oct 10, 2018

Why against develop and nich master was it’s a hotfix.

@appinteractive
Copy link
Member

appinteractive commented Oct 10, 2018

params.userId comes from feathers! Do not alter it!(at least not without extra really taking care)

@roschaefer
Copy link
Contributor Author

@appinteractive it's params.query.userId not params.userId

@appinteractive
Copy link
Member

appinteractive commented Oct 10, 2018

@roschaefer yeah you are right! But search for userId: in the webapp, there are planty of places where it is used.

When you are changing it, you would have to not overwrite it but merge it like so:

params.query.userId = { ...({ $nin: blacklist }), ...(params.query.userId || {}) }

@roschaefer
Copy link
Contributor Author

@appinteractive I thought this is exactly what I did in the PR. Though, you code is a one-liner while I need two lines.

@appinteractive
Copy link
Member

Okay now I see it, just saw the removed part, not the new one somehow.

@appinteractive appinteractive self-requested a review October 10, 2018 12:59
@appinteractive
Copy link
Member

Still, hotfix for master or develop only?

@roschaefer roschaefer merged commit da96078 into develop Oct 10, 2018
@appinteractive appinteractive deleted the fix_171 branch October 10, 2018 14:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants