Skip to content
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

FIX: Hide ignoring users in preference for users with TL less than a member #7415

Conversation

@khalilovcmded
Copy link
Contributor

khalilovcmded commented Apr 23, 2019

No description provided.

@discoursebot

This comment has been minimized.

Copy link

discoursebot commented Apr 23, 2019

You've signed the CLA, khalilovcmded. Thank you! This pull request is ready for review.

@khalilovcmded khalilovcmded requested review from ZogStriP and eviltrout Apr 23, 2019
@discoursebot

This comment has been minimized.

Copy link

discoursebot commented Apr 23, 2019

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/cannot-remove-ignored-user-on-meta-discourse-org/115407/7

ZogStriP and others added 2 commits Apr 23, 2019
….js.es6

Co-Authored-By: khalilovcmded <45508821+khalilovcmded@users.noreply.github.com>
@khalilovcmded khalilovcmded merged commit dc60128 into discourse:master Apr 23, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@khalilovcmded khalilovcmded deleted the khalilovcmded:tk/hide-ignoring-user-in-preferences branch Apr 23, 2019
@@ -6,6 +6,8 @@ import User from "discourse/models/user";
export default Ember.Controller.extend(PreferencesTabController, {
saveAttrNames: ["muted_usernames", "ignored_usernames"],
ignoredUsernames: Ember.computed.alias("model.ignored_usernames"),
userIsMemberOrAbove: Ember.computed.gte("model.trust_level", 2),
ignoredEnabled: Ember.computed.or("userIsMemberOrAbove", "model.staff"),

This comment has been minimized.

Copy link
@eviltrout

eviltrout Apr 23, 2019

Member

You are duplicating the logic here from the server side, which is always dangerous as they can easily go out of sync. Instead you should pass can_ignore_users via a serializer from the server side to the client and use that.

This comment has been minimized.

Copy link
@khalilovcmded

khalilovcmded Apr 23, 2019

Author Contributor

I initially thought of doing so, then I thought that perhaps the front-end should be responsible for this because it is logic to show/hide a component (it is UI specific).

It may be better be to keep it in the backend because of the go out of sync problem, but I disagree that it is dangerous as it is, the side effect of a bug showing the ignore in preferences isn't going to affect how the feature work, because we always have the guardian on the backend checking the ignore requests to be valid or not.

This comment has been minimized.

Copy link
@eviltrout

eviltrout Apr 23, 2019

Member

Sorry "dangerous" was too severe a word. It is more of an "easy bug" waiting to happen in the future if we change the logic. For other code like this we pass it through in a serializer, and we should be consistent with this feature.

This comment has been minimized.

Copy link
@khalilovcmded

khalilovcmded Apr 24, 2019

Author Contributor

For other code like this we pass it through in a serializer, and we should be consistent with this feature.

That makes sense 👍 I will cut another PR to refactor it. Thanks for the feedback 🙏

@khalilovcmded khalilovcmded self-assigned this Apr 24, 2019
elepedus added a commit to elepedus/discourse that referenced this pull request May 9, 2019
…member (discourse#7415)

* FIX: Hide ignoring users in preference for users with TL less than a member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.