-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
[WIP] #2341 Allow re-generation of anyone-can-join invite link #2351
Conversation
…yone-can-join-invite-issue
…yone-can-join-invite-issue
action: { | ||
type: String, | ||
default: 'addMember', | ||
validator: (value) => ['addMember'].includes(value) |
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.
as the validator here shows, only available value for this prop is 'addMember' which is also set as the default. So just removed it to clean it up.
const MIL = 1000 | ||
const MIL_MIN = 60 * MIL | ||
const MIL_HR = MIL_MIN * 60 | ||
const MIL_DAY = 24 * MIL_HR |
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.
These constants are already in time.js
of the project. so replaced these with them.
group-income Run #3169
Run Properties:
|
Project |
group-income
|
Branch Review |
sebin/task/#2341-anyone-can-join-invite-issue
|
Run status |
Passed #3169
|
Run duration | 09m 15s |
Commit |
b746711df7 ℹ️: Merge b31dd3e6fa4e1254cabc056e87148ba4639a26c1 into 9fd5e1643cf0396ad0dfb58b6d45...
|
Committer | Sebin Song |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
10
|
Skipped |
0
|
Passing |
111
|
View all changes introduced in this branch ↗︎ |
…yone-can-join-invite-issue
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.
Nice job @SebinSong, this is a good start. 👍
I think to really nail this it would be useful for me to send you the data
folder from the debug instance we have so that you can test this locally with a group that had its invite expired. See your DMs on GIG!
Currently, you'll notice that when you login as u1
, the Turtles (4)
has one member and when running with this build, clicking + Add
to add members will bring up this dialog:
There are two issues here:
- The invite link that is shown is expired. We need to modify this PR to detect when these original anyone-can-join links are expired and replace them with new ones. I will comment down below with the logic for this in a separate section.
- The expiry date doesn't show the year here, it should though because now we have invite links expiring 20 years into the future.
Logic on how to upgrade expired anyone-can-join links
What we want to do is detect when the group has an anyone-can-join link that meets certain criteria for replacement. These can be arbitrary, but for now, let's make it simple:
- If the anyone-can-join link is expired, and it has invites remaining, then we will create another anyone-can-join link in its place that has
150 - (previous used invites count)
invites remaining (EDIT: I changed this from 60 to 150)
Then there is the question of where to do this?
We want to make sure that when the user goes to invite someone, that they have this new link available to them.
So where is "when the user goes to invite someone" triggered?
It's triggered in 2 places as far as I can tell:
- When the user clicks the "+ Add" button
- When the user switchings to the
Group Settings
page (e.g. themounted ()
call there)
So we need to be prepared and already have the new link ready for them in those two places, and we need to keep the code DRY.
So I propose creating an action in gi.app
namespace that gets called from both those places. E.g. gi.app/fixAnyoneCanJoinLink
or something like that.
You should be able to call it multiple times safely (e.g. in case when they first call it they're offline, and then they need to call it again).
Hope that makes sense, let me know if you have questions!
@taoeffect Thanks, received the data for debugging in GIG. will update this PR once done with investigating it. |
@@ -92,7 +92,7 @@ export const MESSAGE_TYPES = { | |||
} | |||
|
|||
export const INVITE_EXPIRES_IN_DAYS = { | |||
ON_BOARDING: 30, | |||
ON_BOARDING: 365 * 20, // 20years - we shouldn't let the anyone-can-join invite expire. (reference: https://github.com/okTurtles/group-income/issues/2341) |
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.
20 years is a long time, but if the intention is for the invite not to expire at all, why not just have it not expire?
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.
@corrideat how do we do that? I don't remember how the invite stuff you implemented works. Does it support non-expiring invites? If so, how?
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.
We support that, though not in the UI apparently.
In actions/group.js
, if you skip the 'expires', the invite will not have an expiry time (although the modal will show 'This invite link expires on Invalid Date.' and the group settings will show it as active but 'expired').
/* expires: Math.min(
Date.now() + 1 * 60 * 1000,
Date.now() + DAYS_MILLIS * INVITE_EXPIRES_IN_DAYS.ON_BOARDING
), // TODO: revert this after development
*/
We support this 'feature' already for the group creator to join the group (who joins using the CSK instead of an invite link). This way, the invite link doesn't show as 'used' which would be confusing.
…yone-can-join-invite-issue
@taoeffect
Still need to be implemented in |
@taoeffect I've worked on your last change request which is creating To summarize what it does, (let me know if you have better idea for above though.) This seems to work well (auto-updated to 20yrs) except when I use this invite in another browser/container, it still says it's expired. And apparently it's because the group state loaded by group-income/frontend/views/pages/Join.vue Line 125 in 9fd5e16
Q. What do you think is the missing piece in this flow here which makes sure Hope my explanation here makes sense btw. |
@@ -455,7 +455,7 @@ const getters = { | |||
currentWelcomeInvite (state, getters) { | |||
const invites = getters.currentGroupState.invites | |||
const inviteId = Object.keys(invites).find(invite => invites[invite].creatorID === INVITE_INITIAL_CREATOR) | |||
const expires = getters.currentGroupState._vm.authorizedKeys[inviteId].meta.expires | |||
const expires = getters.currentGroupState._vm.invites[inviteId].expires |
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.
@corrideat Can you review 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.
Let me know if this should be reverted.
But FYI, I made this change for two reasons
1) I noticed there is some other places where group's invites are used this way.
eg1.
group-income/frontend/model/contracts/shared/getters/group.js
Lines 133 to 136 in 9fd5e16
groupMembersPending (state, getters) { | |
const invites = getters.currentGroupState.invites | |
const vmInvites = getters.currentGroupState._vm.invites | |
const pendingMembers = Object.create(null) |
eg2.
group-income/frontend/views/containers/group-settings/InvitationLinkModal.vue
Lines 48 to 54 in 9fd5e16
welcomeInviteSecret () { | |
const key = this.currentGroupState._vm.invites?.[this.currentWelcomeInvite.inviteId]?.inviteSecret | |
if (!key) { | |
console.error(`undefined key for welcomeInviteId: ${this.currentWelcomeInvite.inviteId}`) | |
return undefined | |
} | |
return key |
eg3.
group-income/frontend/views/containers/proposals/ProposalItem.vue
Lines 308 to 313 in 9fd5e16
if ( | |
this.currentGroupState._vm.invites?.[inviteKeyId]?.status === INVITE_STATUS.VALID && | |
this.currentGroupState._vm.authorizedKeys?.[inviteKeyId] && | |
this.currentGroupState._vm.authorizedKeys[inviteKeyId]._notAfterHeight == null && | |
this.currentGroupState._vm.invites[inviteKeyId].inviteSecret | |
) { |
2) It's hard to guess/know that getters.currentGroupState._vm.authorizedKeys
has groups inivite, especially when it's the first time we look at this. getters.currentGroupState._vm.invites[inviteId]
helps better in that it has invites
in there and it feels less complex.
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 think this change should be fine, provided that the key name starts with #inviteKey
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.
To expand on this, some of the meta
attributes are copied to the .invites
field. I think it makes more sense to check under the dedicated .invites
property, unless there's some reason not to. For example, if one wants access to the original attributes as they key was created, one would use _vm.authorizedKeys
.
@@ -173,7 +173,10 @@ export default (sbp('sbp/selectors/register', { | |||
permissions: [GIMessage.OP_KEY_REQUEST], | |||
meta: { | |||
quantity: 60, | |||
expires: Date.now() + DAYS_MILLIS * INVITE_EXPIRES_IN_DAYS.ON_BOARDING, |
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.
See my comment for how not to set an expiry date (i.e., leave this blank). While you're at it, I'd also use the server time where possible (sbp('chelonia/time')
)
@taoeffect
I think it's better to discard this PR and write a new one, as it would require large amount of logic updates here. Given the nature of how the logic related to group-invite has evolved, I think it's better for @corrideat to work on this issue. (I can focus on other tasks like image viewer) what would you think? |
You can put this PR on pause if you'd like, but Ricardo's plate is too full to take this issue on right now as he's focusing exclusively on push notifications and related work. |
@taoeffect OK, let me close this PR for now and use it for a reference for a new PR I will write later then. |
I'm not sure using a 'never expires' should take so many logic changes (I honestly haven't checked), but if that's the case, mind that I only suggested it because 'never expires' is semantically closer to the intent. Setting it to some large value, like 20 years (or 100 years) is a valid (but less ideal) alternative. In other words, my review wasn't meant to say exactly how it should be implemented. |
@corrideat I think it does require redoing large portion of what I've done in this PR man.. Eg) NOTE 1] in #2351 (comment) has to be re-written and also we would need to search through the source code to find all places where something like below exists and makes sure the UI don't break. We also wouldn't need to have 1. and 2. I've done in #2351 (comment) either, as there would be no case where app needs to update So I think it's fair for me to decide to close it and it's better to be re-written from scratch (and no headache of figuring out which part to leave as is and which part needs to be updated again in this PR) Greg said above you have a full table of TODOs now but go ahead and feel free to assign yourself to this issue and work on it if you want, while I'm focusing on other task at my hand too. |
Yeah, sure. I only wanted to point out that my suggestion was just that, in case it didn't come across like that. If you feel it'd be better to re-work it from scratch, then let's do that. It's unlikely I'll have time to work on this issue myself in the near future, but let's see. |
closes #2341
[NOTE 1] As in the screenshot below, if the invite's life is > year, the human-readable expression says 'X yrs Y mth Z d' instead of displaying the hours and minutes too. But let me know if this should be updated otherwise.
[NOTE 2] If it's the case where the group-invite should be generated straight away, the button displays 'Create invitation' instead of 'Send invitation'. But let me know if this should be reverted.