-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add initial support for durable invitation links. #2056
Conversation
0e05677
to
a928420
Compare
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 believe users can access invitation IDs from the raw hunt data, even if not made visible, since they are part
of the hunt schema and are not filtered from the client. Is this resolvable without a broader change?
Yes, that's accurate. We're overdue to reconsider the Hunt publishing model. In the shorter term, I think it might be easier to just create a new model for invite codes? It means that we can create a new record when the invite code is regenerated and see old codes easily
imports/lib/models/Hunts.ts
Outdated
@@ -68,6 +71,7 @@ export const HuntPattern = { | |||
puzzleHooksDiscordChannel: Match.Optional(SavedDiscordObjectPattern), | |||
firehoseDiscordChannel: Match.Optional(SavedDiscordObjectPattern), | |||
memberDiscordRole: Match.Optional(SavedDiscordObjectPattern), | |||
invitationCode: Match.Optional(String), |
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.
HuntPattern
is used by the createHunt
and updateHunt
methods to determine what parameters users are allowed to set. I think we want users going through the new methods, rather than setting it directly, so I think we can drop this
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.
Ack, removed as part of move to a dedicated model for invitation codes.
useEffect(() => { | ||
acceptHuntInvitationCode.call({ invitationCode }, (error, huntId) => { | ||
if (error) { | ||
setStatus(error.reason ?? "Unknown error"); | ||
} else { | ||
navigate(`/hunts/${huntId}`); | ||
} | ||
}); | ||
}, [invitationCode, navigate]); |
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.
@zarvox Since you did the StrictMode
migration work, is this going to result in calling the method more than once?
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.
AIUI - yes, in strict mode, this method will be invoked twice (ref). But it is more or less idempotent - if the user is already a member of the given hunt, it will log a message (at info) but otherwise return successfully. LMK if I should handle this in some other way though.
<a href={`/join/${hunt.invitationCode}`}> | ||
{`${window.location.origin}/join/${hunt.invitationCode}`} | ||
</a> |
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.
Should this be a copy-to-clipboard button instead of a link?
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.
Changed to plain text with a copy-to-clipboard button (cribbed from the guess queue page).
cba64d9
to
37abb80
Compare
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.
Thanks for the review!
Moved invitation codes to a separate model. The publication for that model only returns the code if the user viewing the hunt has permission to add users, so I think this resolves the permission issues, if I understand this all correctly.
useEffect(() => { | ||
acceptHuntInvitationCode.call({ invitationCode }, (error, huntId) => { | ||
if (error) { | ||
setStatus(error.reason ?? "Unknown error"); | ||
} else { | ||
navigate(`/hunts/${huntId}`); | ||
} | ||
}); | ||
}, [invitationCode, navigate]); |
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.
AIUI - yes, in strict mode, this method will be invoked twice (ref). But it is more or less idempotent - if the user is already a member of the given hunt, it will log a message (at info) but otherwise return successfully. LMK if I should handle this in some other way though.
<a href={`/join/${hunt.invitationCode}`}> | ||
{`${window.location.origin}/join/${hunt.invitationCode}`} | ||
</a> |
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.
Changed to plain text with a copy-to-clipboard button (cribbed from the guess queue page).
imports/lib/models/Hunts.ts
Outdated
@@ -68,6 +71,7 @@ export const HuntPattern = { | |||
puzzleHooksDiscordChannel: Match.Optional(SavedDiscordObjectPattern), | |||
firehoseDiscordChannel: Match.Optional(SavedDiscordObjectPattern), | |||
memberDiscordRole: Match.Optional(SavedDiscordObjectPattern), | |||
invitationCode: Match.Optional(String), |
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.
Ack, removed as part of move to a dedicated model for invitation codes.
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.
Sorry - again - that this has taken a while to get back to. I'm going to make an effort to get caught up this weekend, at least.
Overall, the implementation looks good to me. Thanks for reworking the model situation. I think this is a lot better. I noticed two things looking through this revision, but otherwise the guts look good.
In terms of UI tweaks, I'd definitely support a confirmation dialog for regenerating and clearing an invitation link (basically, anything that will make currently in-flight links invalid). It feels a little less important to me from a destructiveness perspective for the first time you generate a link, although maybe people would find it surprising.
(You also mentioned invitation code UI; I'm not sure what I would do there, but open if you have suggestions. I looked at Discord for inspiration and it basically just spits out a full URL with a copy link, so I'm not sure this is significantly different/worse)
|
||
const copyTooltip = <Tooltip>Copy to clipboard</Tooltip>; | ||
|
||
const invitationUrl = `${window.location.origin}/join/${invitationCode}`; |
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.
Small nit: I think this is slightly better expressed as
Meteor.absoluteUrl(`/join/${invitationCode}`)
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.
Done, thanks!
const generateInvitationLink = useCallback(() => { | ||
if (!hunt) { | ||
return; | ||
} | ||
|
||
generateHuntInvitationCode.call({ huntId: hunt._id }); | ||
}, [hunt]); | ||
|
||
const clearInvitationLink = useCallback(() => { | ||
if (!hunt) { | ||
return; | ||
} | ||
|
||
clearHuntInvitationCode.call({ huntId: hunt._id }); | ||
}, [hunt]); | ||
|
||
const invitationLinkManagementButtons = useMemo(() => { | ||
if (!hunt || !canUpdateHuntInvitationCode) { | ||
return null; | ||
} | ||
|
||
return ( | ||
<FormGroup className="mb-3"> | ||
<Button variant="info" onClick={generateInvitationLink}> | ||
{invitationCode | ||
? "Regenerate invitation link" | ||
: "Generate invitation link"} | ||
</Button> | ||
{invitationCode && ( | ||
<Button variant="info" className="ms-1" onClick={clearInvitationLink}> | ||
Disable invitation link | ||
</Button> | ||
)} | ||
<FormText> | ||
Manage the public invitation link that can be used by anyone to join | ||
this hunt | ||
</FormText> | ||
</FormGroup> | ||
); | ||
}, [ | ||
hunt, | ||
canUpdateHuntInvitationCode, | ||
clearInvitationLink, | ||
generateInvitationLink, | ||
invitationCode, | ||
]); |
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'm going to channel @zarvox for a sec and comment that there's no user feedback when you click on these buttons. At minimum, they should probably be disabled while the API call is in flight. Drew would probably ask that errors get presented if there is one, although I'm generally a little less picky about that. (I think in this case we probably don't need an explicit indicator of success, since that's reflected in the button/link content visually changing)
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.
Makes sense. I went ahead and added confirmation dialogs for all of the operations, repurposing the existing dialogs for promoting/demoting operators. Those dialogs already handle in-flight button disabling and showing errors, so this should now be covered.
- Add invitation codes to the hunt schema. Codes are randomly generated IDs. - Allow admins and hunt operators to (re)generate and clear invitation links from the hunt profile list page. - Allow any user with invitation permissions to see the current invitation link (if any) from the hunt profile list page. - Add an authenticated /join/:invitationCode endpoint which adds the current user to the hunt with that code and redirects to that hunt page. (If a user is unauthenticated when they open this link, they will be redirected here after signing in.) See deathandmayhem#2047
- Move to a dedicated model. This prevents invitation codes from leaking unintentionally as part of the hunt, and also lets us track old code generation, who generated codes, etc. - Change invitation view from a link to plain text with a Copy to Clipboard button.
Extract existing dialogs to a common helper and repurpose for (re)generating and disabling invitation links. Also fix a small bug where an error will remain visible after dismissing and reopening a dialog for a failed operation, and use a simpler method of calculating an absolute URL when showing the invitation link.
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.
LGTM. Thanks again for tolerating our sluggishness on review!
To be considered for this PR:
I believe users can access invitation IDs from the raw hunt data, even if not made visible, since they are partResolved by separating invitations out of hunts and into their own modelof the hunt schema and are not filtered from the client. Is this resolvable without a broader change?
To be handled in a future PR:
create an account. This can be detected by checking the history state since it is stored on redirect. If the invitation link is verified upon form submission, create the new user and add it to the hunt.
See #2047