From 76981605fa10975e2e7af457e2f6a31909e0c811 Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Wed, 12 Jul 2017 15:31:10 +0530 Subject: [PATCH] FIX: don't raise error when inviting existing user to private topic via email https://meta.discourse.org/t/inviting-existing-user-to-a-private-topic-message-via-email-shows-error-message/65994 --- .../discourse/controllers/invite.js.es6 | 79 +++++++++++-------- app/models/topic.rb | 24 ++++-- config/locales/client.en.yml | 1 + spec/models/topic_spec.rb | 2 +- 4 files changed, 65 insertions(+), 41 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/invite.js.es6 b/app/assets/javascripts/discourse/controllers/invite.js.es6 index f9d4d52dcdd..90020f449df 100644 --- a/app/assets/javascripts/discourse/controllers/invite.js.es6 +++ b/app/assets/javascripts/discourse/controllers/invite.js.es6 @@ -13,6 +13,7 @@ export default Ember.Controller.extend(ModalFunctionality, { hasCustomMessage: false, customMessage: null, inviteIcon: "envelope", + invitingExistingUserToTopic: false, @computed('isMessage', 'invitingToTopic') title(isMessage, invitingToTopic) { @@ -25,9 +26,10 @@ export default Ember.Controller.extend(ModalFunctionality, { } }, - isAdmin: function(){ + @computed + isAdmin() { return Discourse.User.currentProp("admin"); - }.property(), + }, @computed('isAdmin', 'emailOrUsername', 'invitingToTopic', 'isPrivateTopic', 'model.groupNames', 'model.saving', 'model.details.can_invite_to') disabled(isAdmin, emailOrUsername, invitingToTopic, isPrivateTopic, groupNames, saving, can_invite_to) { @@ -46,29 +48,32 @@ export default Ember.Controller.extend(ModalFunctionality, { return false; }, - disabledCopyLink: function() { - if (this.get('hasCustomMessage')) return true; - if (this.get('model.saving')) return true; - if (Ember.isEmpty(this.get('emailOrUsername'))) return true; - const emailOrUsername = this.get('emailOrUsername').trim(); + @computed('isAdmin', 'emailOrUsername', 'model.saving', 'isPrivateTopic', 'model.groupNames', 'hasCustomMessage') + disabledCopyLink(isAdmin, emailOrUsername, saving, isPrivateTopic, groupNames, hasCustomMessage) { + if (hasCustomMessage) return true; + if (saving) return true; + if (Ember.isEmpty(emailOrUsername)) return true; + const email = emailOrUsername.trim(); // email must be valid - if (!emailValid(emailOrUsername)) return true; + if (!emailValid(email)) return true; // normal users (not admin) can't invite users to private topic via email - if (!this.get('isAdmin') && this.get('isPrivateTopic') && emailValid(emailOrUsername)) return true; + if (!isAdmin && isPrivateTopic && emailValid(email)) return true; // when inviting to private topic via email, group name must be specified - if (this.get('isPrivateTopic') && Ember.isEmpty(this.get('model.groupNames')) && emailValid(emailOrUsername)) return true; + if (isPrivateTopic && Ember.isEmpty(groupNames) && emailValid(email)) return true; return false; - }.property('emailOrUsername', 'model.saving', 'isPrivateTopic', 'model.groupNames', 'hasCustomMessage'), + }, - buttonTitle: function() { - return this.get('model.saving') ? 'topic.inviting' : 'topic.invite_reply.action'; - }.property('model.saving'), + @computed('model.saving') + buttonTitle(saving) { + return saving ? 'topic.inviting' : 'topic.invite_reply.action'; + }, // We are inviting to a topic if the model isn't the current user. // The current user would mean we are inviting to the forum in general. - invitingToTopic: function() { - return this.get('model') !== this.currentUser; - }.property('model'), + @computed('model') + invitingToTopic(model) { + return model !== this.currentUser; + }, @computed('model', 'model.details.can_invite_via_email') canInviteViaEmail(model, can_invite_via_email) { @@ -91,9 +96,7 @@ export default Ember.Controller.extend(ModalFunctionality, { isMessage: Em.computed.equal('model.archetype', 'private_message'), // Allow Existing Members? (username autocomplete) - allowExistingMembers: function() { - return this.get('invitingToTopic'); - }.property('invitingToTopic'), + allowExistingMembers: Ember.computed.alias('invitingToTopic'), // Show Groups? (add invited user to private group) @computed('isAdmin', 'emailOrUsername', 'isPrivateTopic', 'isMessage', 'invitingToTopic', 'canInviteViaEmail') @@ -141,29 +144,34 @@ export default Ember.Controller.extend(ModalFunctionality, { } }, - showGroupsClass: function() { - return this.get('isPrivateTopic') ? 'required' : 'optional'; - }.property('isPrivateTopic'), + @computed('isPrivateTopic') + showGroupsClass(isPrivateTopic) { + return isPrivateTopic ? 'required' : 'optional'; + }, groupFinder(term) { return Group.findAll({search: term, ignore_automatic: true}); }, - successMessage: function() { + @computed('isMessage', 'emailOrUsername', 'invitingExistingUserToTopic') + successMessage(isMessage, emailOrUsername, invitingExistingUserToTopic) { if (this.get('hasGroups')) { return I18n.t('topic.invite_private.success_group'); - } else if (this.get('isMessage')) { + } else if (isMessage) { return I18n.t('topic.invite_private.success'); - } else if ( emailValid(this.get('emailOrUsername')) ) { - return I18n.t('topic.invite_reply.success_email', { emailOrUsername: this.get('emailOrUsername') }); + } else if (invitingExistingUserToTopic) { + return I18n.t('topic.invite_reply.success_existing_email', { emailOrUsername }); + } else if (emailValid(emailOrUsername)) { + return I18n.t('topic.invite_reply.success_email', { emailOrUsername }); } else { return I18n.t('topic.invite_reply.success_username'); } - }.property('model.inviteLink', 'isMessage', 'emailOrUsername'), + }, - errorMessage: function() { - return this.get('isMessage') ? I18n.t('topic.invite_private.error') : I18n.t('topic.invite_reply.error'); - }.property('isMessage'), + @computed('isMessage') + errorMessage(isMessage) { + return isMessage ? I18n.t('topic.invite_private.error') : I18n.t('topic.invite_reply.error'); + }, @computed('canInviteViaEmail') placeholderKey(canInviteViaEmail) { @@ -172,15 +180,17 @@ export default Ember.Controller.extend(ModalFunctionality, { 'topic.invite_reply.username_placeholder'; }, - customMessagePlaceholder: function() { + @computed + customMessagePlaceholder() { return I18n.t('invite.custom_message_placeholder'); - }.property(), + }, // Reset the modal to allow a new user to be invited. reset() { this.set('emailOrUsername', null); this.set('hasCustomMessage', false); this.set('customMessage', null); + this.set('invitingExistingUserToTopic', false); this.get('model').setProperties({ groupNames: null, error: false, @@ -189,6 +199,7 @@ export default Ember.Controller.extend(ModalFunctionality, { inviteLink: null }); }, + actions: { createInvite() { @@ -230,6 +241,8 @@ export default Ember.Controller.extend(ModalFunctionality, { } else if (this.get('isMessage') && result && result.user) { this.get('model.details.allowed_users').pushObject(Ember.Object.create(result.user)); this.appEvents.trigger('post-stream:refresh'); + } else if (this.get('invitingToTopic') && emailValid(this.get('emailOrUsername').trim()) && result && result.user) { + this.set('invitingExistingUserToTopic', true); } }).catch(onerror); } diff --git a/app/models/topic.rb b/app/models/topic.rb index f9fcfadf52e..495459d4fd2 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -744,9 +744,10 @@ SQL # Invite a user to the topic by username or email. Returns success/failure def invite(invited_by, username_or_email, group_ids=nil, custom_message=nil) + user = User.find_by_username_or_email(username_or_email) + if private_message? # If the user exists, add them to the message. - user = User.find_by_username_or_email(username_or_email) raise UserExists.new I18n.t("topic_invite.user_exists") if user.present? && topic_allowed_users.where(user_id: user.id).exists? if user && topic_allowed_users.create!(user_id: user.id) @@ -764,18 +765,27 @@ SQL end if username_or_email =~ /^.+@.+$/ && Guardian.new(invited_by).can_invite_via_email?(self) - # rate limit topic invite RateLimiter.new(invited_by, "topic-invitations-per-day", SiteSetting.max_topic_invitations_per_day, 1.day.to_i).performed! - # NOTE callers expect an invite object if an invite was sent via email - invite_by_email(invited_by, username_or_email, group_ids, custom_message) + if user.present? + # add existing users + Invite.extend_permissions(self, user, invited_by) + + # Notify the user they've been invited + user.notifications.create(notification_type: Notification.types[:invited_to_topic], + topic_id: id, + post_number: 1, + data: { topic_title: title, + display_username: invited_by.username }.to_json) + return true + else + # NOTE callers expect an invite object if an invite was sent via email + invite_by_email(invited_by, username_or_email, group_ids, custom_message) + end else - # invite existing member to a topic - user = User.find_by_username(username_or_email) raise UserExists.new I18n.t("topic_invite.user_exists") if user.present? && topic_allowed_users.where(user_id: user.id).exists? if user && topic_allowed_users.create!(user_id: user.id) - # rate limit topic invite RateLimiter.new(invited_by, "topic-invitations-per-day", SiteSetting.max_topic_invitations_per_day, 1.day.to_i).performed! # Notify the user they've been invited diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 1bb210eb6ca..6c80ba69c20 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1724,6 +1724,7 @@ en: success_email: "We mailed out an invitation to {{emailOrUsername}}. We'll notify you when the invitation is redeemed. Check the invitations tab on your user page to keep track of your invites." success_username: "We've invited that user to participate in this topic." error: "Sorry, we couldn't invite that person. Perhaps they have already been invited? (Invites are rate limited)" + success_existing_email: "A user with email {{emailOrUsername}} already exists. We've invited that user to participate in this topic." login_reply: 'Log In to Reply' diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 5325b9f89dd..be8fef69227 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -1748,7 +1748,7 @@ describe Topic do it "should add user to the group" do expect(Guardian.new(walter).can_see?(group_private_topic)).to be_falsey - expect { group_private_topic.invite(group_manager, walter.email) }.to raise_error(Invite::UserExists) + group_private_topic.invite(group_manager, walter.email) expect(walter.groups).to include(group) expect(Guardian.new(walter).can_see?(group_private_topic)).to be_truthy end