FEATURE: Various improvements to invite system (#12298)

* FIX: Do not show expired invites under Pending tab

* DEV: Controller action was renamed in previous commit

* FEATURE: Add 'Expired' tab to invites

* FEATURE: Refresh model after removing expired invites

* FEATURE: Do not immediately add invite to the list

Opening the 'create-invite' modal used to automatically generate an
invite to reserve an invite link. If the user did not save it and
closed the modal, the invite would be destroyed. This operations caused
the invite list to change in the background and confuse users.

* FEATURE: Sort redeemed users by creation time

* UX: Improve show / hide advanced options link

* FIX: Show redeemed users even if invites were trashed

* UX: Change modal title when editing invite

* UX: Remove Get Link button

Users can get it from the edit modal

* FEATURE: Add limit for invite links generated by regular users

* FEATURE: Add option to skip email

* UX: Show better error messages

* FIX: Show "Invited by" even if invite was trashed

Follow up to 1fdfa13a099d8e46edd0c481b3aaaafe40455ced.

* FEATURE: Add button to save without sending email

Follow up to c86379a465f28a3cc64a4a8c939cf32cf2931659.

* DEV: Use a buffer to hold all changed data

* FEATURE: Close modal after save

* FEATURE: Rate limit resend invite email

* FEATURE: Make the save buttons smarter

* FEATURE: Do not always send email even for new invites
This commit is contained in:
Dan Ungureanu 2021-03-06 13:29:35 +02:00 committed by GitHub
parent fae2fc0b5e
commit 7f3240ea31
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
19 changed files with 244 additions and 99 deletions

View File

@ -7,7 +7,6 @@ import { bufferedProperty } from "discourse/mixins/buffered-content";
import ModalFunctionality from "discourse/mixins/modal-functionality";
import Group from "discourse/models/group";
import Invite from "discourse/models/invite";
import I18n from "I18n";
export default Controller.extend(
ModalFunctionality,
@ -20,14 +19,9 @@ export default Controller.extend(
autogenerated: false,
showAdvanced: false,
showOnly: false,
type: "link",
topicId: null,
topicTitle: null,
groupIds: null,
onShow() {
Group.findAll().then((groups) => {
this.set("allGroups", groups.filterBy("automatic", false));
@ -36,7 +30,6 @@ export default Controller.extend(
this.setProperties({
autogenerated: false,
showAdvanced: false,
showOnly: false,
});
this.setInvite(Invite.create());
@ -54,38 +47,45 @@ export default Controller.extend(
this.setProperties({
invite,
type: invite.email ? "email" : "link",
groupIds: invite.groups ? invite.groups.map((g) => g.id) : null,
});
if (invite.topics && invite.topics.length > 0) {
this.setProperties({
topicId: invite.topics[0].id,
topicTitle: invite.topics[0].title,
});
} else {
this.setProperties({ topicId: null, topicTitle: null });
}
},
save(autogenerated) {
this.set("autogenerated", autogenerated);
save(opts) {
const newRecord =
(this.autogenerated || !this.invite.id) && !opts.autogenerated;
const data = {
group_ids: this.groupIds,
topic_id: this.topicId,
expires_at: this.buffered.get("expires_at"),
};
this.set("autogenerated", opts.autogenerated);
if (this.type === "link") {
data.max_redemptions_allowed = this.buffered.get(
"max_redemptions_allowed"
);
} else if (this.type === "email") {
data.email = this.buffered.get("email");
data.custom_message = this.buffered.get("custom_message");
const data = { ...this.buffered.buffer };
if (data.groupIds !== undefined) {
data.group_ids = data.groupIds;
delete data.groupIds;
}
if (data.topicId !== undefined) {
data.topic_id = data.topicId;
delete data.topicId;
delete data.topicTitle;
}
if (this.type === "link") {
if (this.buffered.get("email")) {
data.email = "";
data.custom_message = "";
}
} else if (this.type === "email") {
if (this.buffered.get("max_redemptions_allowed") > 1) {
data.max_redemptions_allowed = 1;
}
if (opts.sendEmail) {
data.send_email = true;
} else {
data.skip_email = true;
}
}
const newRecord = !this.invite.id;
return this.invite
.save(data)
.then(() => {
@ -96,10 +96,7 @@ export default Controller.extend(
}
if (!this.autogenerated) {
this.appEvents.trigger("modal-body:flash", {
text: I18n.t("user.invited.invite.invite_saved"),
messageClass: "success",
});
this.send("closeModal");
}
})
.catch((e) =>
@ -113,6 +110,15 @@ export default Controller.extend(
isLink: equal("type", "link"),
isEmail: equal("type", "email"),
@discourseComputed(
"currentUser.staff",
"siteSettings.invite_link_max_redemptions_limit",
"siteSettings.invite_link_max_redemptions_limit_users"
)
maxRedemptionsAllowedLimit(staff, staffLimit, usersLimit) {
return staff ? staffLimit : usersLimit;
},
@discourseComputed("buffered.expires_at")
expiresAtRelative(expires_at) {
return moment.duration(moment(expires_at) - moment()).humanize();
@ -127,18 +133,16 @@ export default Controller.extend(
return false;
},
@discourseComputed("type", "invite.email", "buffered.email")
saveLabel(type, email, bufferedEmail) {
return type === "email" && email !== bufferedEmail
? "user.invited.invite.send_invite_email"
: "user.invited.invite.save_invite";
@discourseComputed("buffered.hasBufferedChanges", "invite.email", "type")
changed(hasBufferedChanges, inviteEmail, type) {
return hasBufferedChanges || (inviteEmail ? "email" : "link") !== type;
},
@action
saveInvite() {
saveInvite(sendEmail) {
this.appEvents.trigger("modal-body:clearFlash");
this.save();
this.save({ sendEmail });
},
}
);

View File

@ -42,6 +42,7 @@ export default Controller.extend({
},
inviteRedeemed: equal("filter", "redeemed"),
inviteExpired: equal("filter", "expired"),
invitePending: equal("filter", "pending"),
@discourseComputed("filter")
@ -72,6 +73,17 @@ export default Controller.extend({
}
},
@discourseComputed("invitesCount.total", "invitesCount.expired")
expiredLabel(invitesCountTotal, invitesCountExpired) {
if (invitesCountTotal > 0) {
return I18n.t("user.invited.expired_tab_with_count", {
count: invitesCountExpired,
});
} else {
return I18n.t("user.invited.expired_tab");
}
},
@discourseComputed("invitesCount.total", "invitesCount.redeemed")
redeemedLabel(invitesCountTotal, invitesCountRedeemed) {
if (invitesCountTotal > 0) {
@ -87,7 +99,7 @@ export default Controller.extend({
createInvite() {
const controller = showModal("create-invite");
controller.set("invites", this.model.invites);
controller.save(true);
controller.save({ autogenerated: true });
},
@action
@ -102,13 +114,6 @@ export default Controller.extend({
controller.setInvite(invite);
},
@action
showInvite(invite) {
const controller = showModal("create-invite");
controller.set("showOnly", true);
controller.setInvite(invite);
},
@action
destroyInvite(invite) {
invite.destroy();
@ -122,6 +127,7 @@ export default Controller.extend({
Invite.destroyAllExpired()
.then(() => {
this.set("removedAll", true);
this.send("triggerRefresh");
})
.catch(popupAjaxError);
}

View File

@ -1,5 +1,7 @@
import EmberObject from "@ember/object";
import { alias } from "@ember/object/computed";
import { Promise } from "rsvp";
import discourseComputed from "discourse-common/utils/decorators";
import User from "discourse/models/user";
import { ajax } from "discourse/lib/ajax";
import { isNone } from "@ember/utils";
@ -30,6 +32,14 @@ const Invite = EmberObject.extend({
.then(() => this.set("reinvited", true))
.catch(popupAjaxError);
},
@discourseComputed("groups")
groupIds(groups) {
return groups ? groups.map((group) => group.id) : [];
},
topicId: alias("topics.firstObject.id"),
topicTitle: alias("topics.firstObject.title"),
});
Invite.reopenClass({

View File

@ -22,4 +22,10 @@ export default DiscourseRoute.extend({
searchTerm: "",
});
},
actions: {
triggerRefresh() {
this.refresh();
},
},
});

View File

@ -1,4 +1,4 @@
{{#d-modal-body title=(if showOnly "user.invited.invite.show_link" (if inviteId "user.invited.invite.edit_title" "user.invited.invite.new_title"))}}
{{#d-modal-body title=(if invite.id "user.invited.invite.edit_title" "user.invited.invite.new_title")}}
<div class="input-group">
<label for="invite_link">{{i18n "user.invited.invite.instructions"}}</label>
{{input name="invite_link"
@ -10,17 +10,13 @@
<p>{{i18n "user.invited.invite.expires_at_time" time=expiresAtRelative}}</p>
{{#unless showOnly}}
<p>
{{#if showAdvanced}}
{{d-icon "caret-down"}}
<a href {{action (mut showAdvanced) false}}>{{i18n "user.invited.invite.hide_advanced"}}</a>
<a href {{action (mut showAdvanced) false}}>{{d-icon "caret-down"}} {{i18n "user.invited.invite.hide_advanced"}}</a>
{{else}}
{{d-icon "caret-right"}}
<a href {{action (mut showAdvanced) true}}>{{i18n "user.invited.invite.show_advanced"}}</a>
<a href {{action (mut showAdvanced) true}}>{{d-icon "caret-right"}} {{i18n "user.invited.invite.show_advanced"}}</a>
{{/if}}
</p>
{{/unless}}
{{#if showAdvanced}}
<div class="input-group">
@ -43,7 +39,7 @@
type="number"
value=buffered.max_redemptions_allowed
min="1"
max=siteSettings.invite_link_max_redemptions_limit
max=maxRedemptionsAllowedLimit
}}
</div>
{{/if}}
@ -64,16 +60,16 @@
<label>{{i18n "user.invited.invite.add_to_groups"}}</label>
{{group-chooser
content=allGroups
value=groupIds
value=buffered.groupIds
labelProperty="name"
onChange=(action (mut groupIds))
onChange=(action (mut buffered.groupIds))
}}
</div>
<div class="input-group">
{{choose-topic
selectedTopicId=topicId
topicTitle=topicTitle
selectedTopicId=buffered.topicId
topicTitle=buffered.topicTitle
additionalFilters="status:public"
label="user.invited.invite.invite_to_topic"
}}
@ -100,19 +96,27 @@
{{/d-modal-body}}
<div class="modal-footer">
{{#unless showOnly}}
{{#if isEmail}}
{{d-button
icon=(if isEmail "envelope" "link")
label=saveLabel
icon="envelope"
label=(if changed "user.invited.invite.send_invite_email" "user.invited.reinvite")
class="btn-primary"
action=(action "saveInvite" true)
disabled=disabled
}}
{{/if}}
{{d-button
icon="link"
label="user.invited.invite.save_invite"
class="btn-primary"
action=(action "saveInvite")
disabled=disabled
}}
{{/unless}}
{{d-button
label="close"
class="btn-primary"
label="cancel"
class="btn-flat"
action=(route-action "closeModal")
}}
</div>

View File

@ -7,6 +7,7 @@
<nav>
<ul class="nav nav-pills">
{{nav-item route="userInvited.show" routeParam="pending" i18nLabel=pendingLabel}}
{{nav-item route="userInvited.show" routeParam="expired" i18nLabel=expiredLabel}}
{{nav-item route="userInvited.show" routeParam="redeemed" i18nLabel=redeemedLabel}}
</ul>
</nav>
@ -79,6 +80,35 @@
{{/each}}
</tbody>
</table>
{{else if inviteExpired}}
<table class="table user-invite-list">
<thead>
<tr>
<th>{{i18n "user.invited.invited_via"}}</th>
<th>{{i18n "user.invited.sent"}}</th>
<th>{{i18n "user.invited.expires_at"}}</th>
<th></th>
</tr>
</thead>
<tbody>
{{#each model.invites as |invite|}}
<tr>
<td>
{{#if invite.email}}
{{invite.email}}
{{else}}
{{i18n "user.invited.invited_via_link" count=invite.redemption_count max=invite.max_redemptions_allowed}}
{{/if}}
</td>
<td>{{raw-date invite.updated_at}}</td>
<td>{{raw-date invite.expires_at}}</td>
<td class="actions">
{{d-button icon="trash-alt" class="cancel" action=(action "destroyInvite" invite) title=(if invite.destroyed "user.invited.removed" "user.invited.remove")}}
</td>
</tr>
{{/each}}
</tbody>
</table>
{{else}}
<table class="table user-invite-list">
<thead>
@ -122,10 +152,6 @@
<td class="actions">
{{d-button icon="pencil-alt" action=(action "editInvite" invite) title="user.invited.edit"}}
{{d-button icon="trash-alt" class="cancel" action=(action "destroyInvite" invite) title=(if invite.destroyed "user.invited.removed" "user.invited.remove")}}
{{d-button icon="link" action=(action "showInvite" invite) title="user.invited.copy_link"}}
{{#if invite.email}}
{{d-button icon="sync" action=(action "reinvite" invite) disabled=invite.reinvited label=(if invite.reinvited "user.invited.reinvited" "user.invited.reinvite")}}
{{/if}}
</td>
</tr>
{{/each}}

View File

@ -843,7 +843,7 @@
.radio-group {
input[type="radio"] {
display: table-cell;
display: inline;
vertical-align: middle;
margin-top: -1px;
}

View File

@ -2,7 +2,7 @@
class InvitesController < ApplicationController
requires_login only: [:create, :destroy, :destroy_all, :resend_invite, :resend_all_invites, :upload_csv]
requires_login only: [:create, :destroy, :destroy_all_expired, :resend_invite, :resend_all_invites, :upload_csv]
skip_before_action :check_xhr, except: [:perform_accept_invitation]
skip_before_action :preload_json, except: [:show]
@ -73,7 +73,7 @@ class InvitesController < ApplicationController
render json: failed_json, status: 422
end
rescue Invite::UserExists, ActiveRecord::RecordInvalid => e
render json: { errors: [e.message] }, status: 422
render_json_error(e.message)
end
end
@ -109,13 +109,31 @@ class InvitesController < ApplicationController
new_email = params[:email].presence
if old_email != new_email
invite.emailed_status = Invite.emailed_status_types[new_email ? :pending : :not_required]
invite.emailed_status = if new_email && !params[:skip_email]
Invite.emailed_status_types[:pending]
else
Invite.emailed_status_types[:not_required]
end
end
end
invite.email = new_email
if params[:send_email]
if invite.emailed_status != Invite.emailed_status_types[:pending]
begin
RateLimiter.new(current_user, "resend-invite-per-hour", 10, 1.hour).performed!
rescue RateLimiter::LimitExceeded
return render_json_error(I18n.t("rate_limiter.slow_down"))
end
end
invite.update!(params.permit(:custom_message, :max_redemptions_allowed, :expires_at))
invite.emailed_status = Invite.emailed_status_types[:pending]
end
begin
invite.update!(params.permit(:email, :custom_message, :max_redemptions_allowed, :expires_at))
rescue ActiveRecord::RecordInvalid => e
return render_json_error(e.message)
end
end
if invite.emailed_status == Invite.emailed_status_types[:pending]

View File

@ -407,6 +407,8 @@ class UsersController < ApplicationController
invites = if filter == "pending" && guardian.can_see_invite_details?(inviter)
Invite.includes(:topics, :groups).pending(inviter)
elsif filter == "expired"
Invite.expired(inviter)
elsif filter == "redeemed"
Invite.redeemed_users(inviter)
else
@ -423,6 +425,7 @@ class UsersController < ApplicationController
end
pending_count = Invite.pending(inviter).reorder(nil).count.to_i
expired_count = Invite.expired(inviter).reorder(nil).count.to_i
redeemed_count = Invite.redeemed_users(inviter).reorder(nil).count.to_i
render json: MultiJson.dump(InvitedSerializer.new(
@ -433,8 +436,9 @@ class UsersController < ApplicationController
type: filter,
counts: {
pending: pending_count,
expired: expired_count,
redeemed: redeemed_count,
total: pending_count + redeemed_count
total: pending_count + expired_count
}
),
scope: guardian,

View File

@ -55,7 +55,12 @@ class Invite < ActiveRecord::Base
if user && user.id != self.invited_users&.first&.user_id
@email_already_exists = true
errors.add(:email)
errors.add(:email, I18n.t(
"invite.user_exists",
email: email,
username: user.username,
base_path: Discourse.base_path
))
end
end
@ -181,16 +186,26 @@ class Invite < ActiveRecord::Base
.joins("LEFT JOIN users ON invited_users.user_id = users.id")
.where(invited_by_id: inviter.id)
.where('redemption_count < max_redemptions_allowed')
.where('expires_at > ?', Time.zone.now)
.order('invites.updated_at DESC')
end
def self.expired(inviter)
Invite.distinct
.joins("LEFT JOIN invited_users ON invites.id = invited_users.invite_id")
.joins("LEFT JOIN users ON invited_users.user_id = users.id")
.where(invited_by_id: inviter.id)
.where('redemption_count > max_redemptions_allowed OR expires_at < ?', Time.zone.now)
.order('invites.expires_at ASC')
end
def self.redeemed_users(inviter)
InvitedUser
.includes(:invite)
.joins("LEFT JOIN invites ON invites.id = invited_users.invite_id")
.includes(user: :user_stat)
.where('invited_users.user_id IS NOT NULL')
.where('invites.invited_by_id = ?', inviter.id)
.order('user_stats.time_read DESC, invited_users.redeemed_at DESC')
.order('invited_users.redeemed_at DESC')
.references('invite')
.references('user')
.references('user_stat')
@ -221,8 +236,13 @@ class Invite < ActiveRecord::Base
def ensure_max_redemptions_allowed
if self.max_redemptions_allowed.nil?
self.max_redemptions_allowed = 1
elsif !self.max_redemptions_allowed.between?(1, SiteSetting.invite_link_max_redemptions_limit)
errors.add(:max_redemptions_allowed, I18n.t("invite_link.max_redemptions_limit", max_limit: SiteSetting.invite_link_max_redemptions_limit))
else
limit = invited_by&.staff? ? SiteSetting.invite_link_max_redemptions_limit
: SiteSetting.invite_link_max_redemptions_limit_users
if !self.max_redemptions_allowed.between?(1, limit)
errors.add(:max_redemptions_allowed, I18n.t("invite_link.max_redemptions_limit", max_limit: limit))
end
end
end

View File

@ -2,7 +2,7 @@
class InvitedUser < ActiveRecord::Base
belongs_to :user
belongs_to :invite
belongs_to :invite, -> { unscope(where: :deleted_at) }
validates_presence_of :invite_id
validates_uniqueness_of :invite_id, scope: :user_id, conditions: -> { where.not(user_id: nil) }

View File

@ -446,7 +446,7 @@ class User < ActiveRecord::Base
end
def invited_by
used_invite = Invite.joins(:invited_users).where("invited_users.user_id = ?", self.id).first
used_invite = Invite.with_deleted.joins(:invited_users).where("invited_users.user_id = ?", self.id).first
used_invite.try(:invited_by)
end

View File

@ -6,7 +6,7 @@ class InvitedSerializer < ApplicationSerializer
def invites
ActiveModel::ArraySerializer.new(
object.invite_list,
each_serializer: object.type == "pending" ? InviteSerializer : InvitedUserSerializer,
each_serializer: object.type == "pending" || object.type == "expired" ? InviteSerializer : InvitedUserSerializer,
scope: scope,
root: false,
show_emails: object.show_emails

View File

@ -1442,6 +1442,8 @@ en:
title: "Invites"
pending_tab: "Pending"
pending_tab_with_count: "Pending (%{count})"
expired_tab: "Expired"
expired_tab_with_count: "Expired (%{count})"
redeemed_tab: "Redeemed"
redeemed_tab_with_count: "Redeemed (%{count})"
invited_via: "Invitation"
@ -1492,7 +1494,6 @@ en:
invite:
new_title: "Create Invite"
edit_title: "Edit Invite"
show_link: "Invite Link"
instructions: "Share this link to instantly grant access to this site:"
copy_link: "copy link"
@ -1501,8 +1502,8 @@ en:
show_advanced: "Show Advanced Options"
hide_advanced: "Hide Advanced Options"
type_email: "Automatically send invitation link via email"
type_link: "Manually share an invite link to people"
type_email: "Invite just one email address"
type_link: "Invite one or more people with a link"
email: "Limit to email address:"
max_redemptions_allowed: "Max number of uses:"
@ -1512,7 +1513,7 @@ en:
expires_at: "Expire after:"
custom_message: "Send email:"
send_invite_email: "Send Invite Email"
send_invite_email: "Save and Send Email"
save_invite: "Save Invite"
invite_saved: "Invite was saved."

View File

@ -1758,6 +1758,7 @@ en:
max_post_deletions_per_day: "Maximum number of posts a user can delete per day. Set to 0 to disable post deletions."
invite_link_max_redemptions_limit: "Maximum redemptions allowed for invite links can't be more than this value."
invite_link_max_redemptions_limit_users: "Maximum redemptions allowed for invite links generated by regular users can't be more than this value."
alert_admins_if_errors_per_minute: "Number of errors per minute in order to trigger an admin alert. A value of 0 disables this feature. NOTE: requires restart."
alert_admins_if_errors_per_hour: "Number of errors per hour in order to trigger an admin alert. A value of 0 disables this feature. NOTE: requires restart."

View File

@ -1744,6 +1744,11 @@ rate_limits:
max: 1000000
default: 5000
client: true
invite_link_max_redemptions_limit_users:
min: 2
max: 1000000
default: 10
client: true
developer:
force_hostname:

View File

@ -449,6 +449,8 @@ describe Invite do
pending_invite = Fabricate(:invite, invited_by: inviter, email: 'pending@example.com')
pending_link_invite = Fabricate(:invite, invited_by: inviter, max_redemptions_allowed: 5)
expired_invite = Fabricate(:invite, invited_by: inviter, email: 'expired@example.com', expires_at: 1.day.ago)
expect(Invite.pending(inviter)).to contain_exactly(pending_invite, pending_link_invite)
end
end
@ -465,9 +467,18 @@ describe Invite do
expect(Invite.redeemed_users(inviter)).to contain_exactly(redeemed_invite.invited_users.first)
end
it 'returns redeemed invites even if trashed' do
inviter = Fabricate(:user)
redeemed_invite = Fabricate(:invite, invited_by: inviter, email: 'redeemed@example.com')
Fabricate(:invited_user, invite: redeemed_invite, user: Fabricate(:user))
redeemed_invite.trash!
expect(Invite.redeemed_users(inviter)).to contain_exactly(redeemed_invite.invited_users.first)
end
it 'returns redeemed invites for invite links' do
inviter = Fabricate(:user)
invite_link = Fabricate(:invite, invited_by: inviter, max_redemptions_allowed: 50)
invite_link = Fabricate(:invite, invited_by: inviter, max_redemptions_allowed: 5)
redeemed = [
Fabricate(:invited_user, invite: invite_link, user: Fabricate(:user)),

View File

@ -2554,4 +2554,14 @@ describe User do
expect(user.do_not_disturb?).to eq(false)
end
end
describe "#invited_by" do
it 'returns even if invites was trashed' do
invite = Fabricate(:invite, invited_by: Fabricate(:user))
Fabricate(:invited_user, invite: invite, user: user)
invite.trash!
expect(user.invited_by).to eq(invite.invited_by)
end
end
end

View File

@ -277,6 +277,11 @@ describe InvitesController do
before do
sign_in(admin)
RateLimiter.enable
end
after do
RateLimiter.disable
end
it 'updating email address resends invite email' do
@ -293,6 +298,20 @@ describe InvitesController do
expect(invite.reload.custom_message).to eq("new message")
expect(Jobs::InviteEmail.jobs.size).to eq(0)
end
it 'can send invite email' do
user = Fabricate(:user, trust_level: SiteSetting.min_trust_level_to_allow_invite)
invite = Fabricate(:invite, invited_by: user, email: 'test@example.com')
sign_in(user)
RateLimiter.enable
expect { put "/invites/#{invite.id}", params: { send_email: true } }
.to change { RateLimiter.new(user, "resend-invite-per-hour", 10, 1.hour).remaining }.by(-1)
expect(response.status).to eq(200)
expect(Jobs::InviteEmail.jobs.size).to eq(1)
end
end
context '#perform_accept_invitation' do