okTurtles / group-income

A decentralized and private (end-to-end encrypted) financial safety net for you and your friends.
https://groupincome.org
GNU Affero General Public License v3.0
331 stars 44 forks source link

[WIP] #2341 Allow re-generation of anyone-can-join invite link #2351

Closed SebinSong closed 1 month ago

SebinSong commented 2 months ago

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.

create-invite-modal
cypress[bot] commented 2 months ago

group-income    Run #3169

Run Properties:  status check passed Passed #3169  •  git commit b746711df7 ℹ️: Merge b31dd3e6fa4e1254cabc056e87148ba4639a26c1 into 9fd5e1643cf0396ad0dfb58b6d45...
Project group-income
Branch Review sebin/task/#2341-anyone-can-join-invite-issue
Run status status check passed Passed #3169
Run duration 09m 15s
Commit git commit b746711df7 ℹ️: Merge b31dd3e6fa4e1254cabc056e87148ba4639a26c1 into 9fd5e1643cf0396ad0dfb58b6d45...
Committer Sebin Song
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 10
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 111
View all changes introduced in this branch ↗︎
SebinSong commented 2 months ago

@taoeffect Thanks, received the data for debugging in GIG. will update this PR once done with investigating it.

SebinSong commented 1 month ago

@taoeffect I just investigated with the debug data you shared and clicking on + add button actually spawns this AddMember proposal modal instead. (when trying it on master FYI)



But the solution you proposed which is,

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)

Still need to be implemented in Group Settings. So let me work on adding this logic in Group settings page as an additional update.

SebinSong commented 1 month ago

@taoeffect I've worked on your last change request which is creating gi.app/group/fixAnyoneCanJoinLink action in this commit.

To summarize what it does, 1. Created gi.contracts/group/updateGroupInviteExpiry which allows us to update the expires attribute of a group's invite (here). 2. gi.app/group/fixAnyoneCanJoinLink will call thisgi.contracts/group/updateGroupInviteExpiry to update the anyone-can-join invite to have 20 years as its expires.

(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 sbp('chelonia/latestContractState') in Join.vue still has the old state before 1. above.

https://github.com/okTurtles/group-income/blob/9fd5e1643cf0396ad0dfb58b6d453ddb941a892c/frontend/views/pages/Join.vue#L125

Q. What do you think is the missing piece in this flow here which makes sure sbp('chelonia/latestContractState') loads the group state with the latest update made in 1.?

Hope my explanation here makes sense btw. Cheers,

SebinSong commented 1 month ago

@taoeffect If we are going with what @corrideat suggested above,

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').

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.

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?

taoeffect commented 1 month ago

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.

SebinSong commented 1 month ago

@taoeffect OK, let me close this PR for now and use it for a reference for a new PR I will write later then.

corrideat commented 1 month ago

@SebinSong I think it's better to discard this PR and write a new one, as it would require large amount of logic updates here.

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.

SebinSong commented 1 month ago

@corrideat I think it does require redoing large portion of what I've done in this PR man..

Eg) NOTE 1] in https://github.com/okTurtles/group-income/pull/2351#issue-2527560608 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 https://github.com/okTurtles/group-income/pull/2351#issuecomment-2370393831 either, as there would be no case where app needs to update expires property of a group-invite, if anyone-can-join invite won't have this value in the first place.

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.

corrideat commented 1 month ago

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.