Corrideat/task/#2341 anyone can join invite issue #2366

corrideat · 2024-09-30T18:23:22Z

No description provided.

…yone-can-join-invite-issue

frontend/views/containers/group-settings/InvitationsTable.vue

cypress · 2024-09-30T18:35:03Z

group-income Run #3258

Run Properties: Passed #3258 • dd61181395 ℹ️: Merge d0ec792a690c74cbb02f151bc5819de95b32e27f into b64ecda81d1b1c9574b1b4490945...

Project	`group-income`
Run status	`Passed #3258`
Run duration	`09m 20s`
Commit	`dd61181395 ℹ️: Merge d0ec792a690c74cbb02f151bc5819de95b32e27f into b64ecda81d1b1c9574b1b4490945...`
Committer	`Ricardo Iván Vieitez Parra`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`10`
Skipped	`0`
Passing	`111`

corrideat · 2024-10-02T18:21:17Z

@greg Looks like this should be ready!

taoeffect

Prelim

frontend/model/contracts/shared/time.js

…e-issue

taoeffect

Running "exec:flow" (exec) task
Error -------------------------------------------------------------------- frontend/model/contracts/shared/time.js:177:4

Cannot expect string as the return type of function because string [1] is incompatible with implicitly-returned
undefined. [incompatible-return]

   177| ): string {
           ^^^^^^ [1]


Error ------------------------------------------------------------------- frontend/model/contracts/shared/time.js:263:46

Cannot build a typed interface for this module. You should annotate the exports of this module with types. Missing type
annotation at function return: [signature-verification-failure]

   263| export function timeLeft (expiryTime: number) {

taoeffect

A few weird errors after visiting http://localhost:3000/app/group-settings using GI_PERSIST=sqlite grunt dev with the data I sent to you on GIG DM.

First there was this:

Not sure if that's a big deal or not.

But then 3 different invites were created, when there should only be one:

And they are showing 0/60, when they should show 0/150.

It also took a while for the new invites to appear...

corrideat · 2024-10-04T19:09:51Z

The lastLoggedIn appears to be unrelated
3 different invites, I'll see what you're doing.
And they are showing 0/60, when they should show 0/150. Not sure where you're getting the 150 number (maybe you mean 180?), but it's correct that each has 60, or whatever the number is.

frontend/controller/actions/group.js

frontend/views/utils/time.js

taoeffect

Nice improvements @corrideat!

I left a few comments (see above), and also tried testing it again.

I wasn't successful though with the test data that I sent you. Logged in as u1 and switched to Turtles (4) group.

Old invite still there, showing wrong expiry too:

Using that invite shows it's expired.

This PR needs to also update the UI to say that this invite doesn't expire (if it doesn't have an expiry).

corrideat · 2024-10-10T22:08:42Z

@taoeffect I think this should be ready now

frontend/model/contracts/shared/time.js

taoeffect · 2024-10-11T17:24:55Z

frontend/model/state.js

+// Wrapper function for performing contract upgrades and migrations
+const contractUpdate = (updateFn: (contractIDHints: ?string[]) => any) => {
+  const loginErrorHandler = () => {
+    sbp('okTurtles.events/off', CONTRACTS_MODIFIED, modifiedHandled)
+    sbp('okTurtles.events/off', LOGIN_COMPLETE, loginCompleteHandler)
+    sbp('okTurtles.events/off', LOGOUT, logoutHandler)
+  }
+  const logoutHandler = () => {
+    sbp('okTurtles.events/off', CONTRACTS_MODIFIED, modifiedHandled)
+  }
+  const loginCompleteHandler = () => {
+    sbp('okTurtles.events/off', LOGIN_ERROR, loginErrorHandler)
+  }
+  // This function is called when the set of subscribed contracts is modified
+  const modifiedHandled = (_, { added }) => {
+    // Wait for the added contracts to be ready, then call the update function
+    sbp('chelonia/contract/wait', added).then(() => {
+      updateFn(added)
+    })
+  }
+
+  // Add event listeners for CONTRACTS_MODIFIED, LOGOUT, LOGIN_COMPLETE and
+  // LOGIN_ERROR
+  sbp('okTurtles.events/on', CONTRACTS_MODIFIED, modifiedHandled)
+  sbp('okTurtles.events/once', LOGOUT, logoutHandler)
+  sbp('okTurtles.events/once', LOGIN_COMPLETE, loginCompleteHandler)
+  sbp('okTurtles.events/once', LOGIN_ERROR, loginErrorHandler)
+
+  // Call the update function in the next tick
+  setTimeout(updateFn, 0)
+}


So we're just going to keep calling the postUpgrade functions every time the contracts are modified?

That seems ugly.

Is there someway to make it so that after the upgrade gets run, it's the modifiedHandled is no longer called?

This seems like a potentially useful utility function. Can we import it from some other location instead of here? (Is there a way to make this available as a Chelonia dev utility)?

Is there someway to make it so that after the upgrade gets run, it's the modifiedHandled is no longer called?

One idea that occurred to me, is that updateFn can pass in a function that will unregister the listener that can be called once a successful upgrade is done.

Is there someway to make it so that after the upgrade gets run, it's the modifiedHandled is no longer called?

Well, no, because that's the entire point of this change not to have to refresh the page. The contractIDHints argument is there to allow returning early.

But some of these functions only need to be run once, right? No need to call them every single time a contract is added or removed.

Did you see this?

One idea that occurred to me, is that updateFn can pass in a function that will unregister the listener that can be called once a successful upgrade is done.

This seems like a potentially useful utility function. Can we import it from some other location instead of here? (Is there a way to make this available as a Chelonia dev utility)?

Useful for what, other than upgrades? Like, I'm all for moving it, but I don't see it as being particularly useful as a generic function outside it being used here.

Is there a way to make this available as a Chelonia dev utility

One issue with that is that all of the events other than CONTRACTS_MODIFIED are GI events, not Chelonia events.

Useful for what, other than upgrades? Like, I'm all for moving it, but I don't see it as being particularly useful as a generic function outside it being used here.

Useful for anyone writing Chelonia-based apps. This is one of 2 ways we've got for doing upgrades (and the only way that exists currently). Seems like every dev would need it.

One issue with that is that all of the events other than CONTRACTS_MODIFIED are GI events, not Chelonia events.

Honestly, with how complicated logging in and logging out is, I think we will be forced to move that code into Chelonia.

with how complicated logging in and logging out is, I think we will be forced to move that code into Chelonia.

I'm not sure that feasible nor a good idea, and it's not really that logging in or out is complicated: it is, in a way, but that's because there are things that need to be done in specific places (the SW and the app), and because of specific things related to saving and loading the state, which can also need encryption and decryption.

In this case, the LOGIN_COMPLETE and LOGIN_ERROR events are also only on the application / browsing-context side (implementing this function in the actions / SW side wouldn't need these events, maybe it'd need LOGIN, but you asked to use the existing selector we have).

Moving logging in and out into Chelonia means that certain types of contracts need to be defined, rather than having a generic system.

In another type of application, you only really need to listen for the CONTRACTS_MODIFIED modified event.

Can we leave this without major changes and improve it later? Like I said, I'm happy to put it in a different file (in fact, I think it doesn't belong in this already complicated file). My question is more about the 'moving it to Chelonia' part, as this is feeling like an ever-growing PR.

frontend/model/state.js

taoeffect

OK, think I've finished the review, sorry it took so long!

frontend/model/state.js

taoeffect · 2024-10-13T18:50:45Z

frontend/model/state.js

+          if (!contractIDHints.reduce((acc, contractID) => {
+            return (acc || state.contracts[contractID]?.type === contractType)
+          }, false)) return


This code seems to me, unnecessarily confusing. Could you please replace this with something simpler, e.g.:

if (!contractIDHints.some(contractID => state.contracts[contractID]?.type === contractType)) { return }

taoeffect · 2024-10-13T18:52:19Z

frontend/model/state.js

+
+// Wrapper function for performing contract upgrades and migrations
+// TODO: Consider moving this function into a different file
+const contractUpdate = (initialState: Object, updateFn: (state: Object, contractIDHints: ?string[]) => any, contractType: ?string) => {


Could you please move contractType to be the 2nd or 1st parameter?

It wouldn't work that well because it's meant to be an optional parameter, as you may want to update contracts regardless of type.

taoeffect · 2024-10-13T18:54:52Z

frontend/model/state.js

+  const resetHandler = () => {
+    sbp('okTurtles.events/off', CONTRACTS_MODIFIED, modifiedHandler)
+  }


Since this is 1 line and it is only used once with a /once listener, it can be inlined as an anonymous function below.

I thought that you preferred named functions over inlined ones (and it's also more consistent in this case)

I do, usually, it especially helps when there's a large or confusing chunk of logic to simplify, or to DRY things. In this case it's simple enough to inline and there's no DRY violation. However, it's not a big to leave as-is too of you prefer.

frontend/model/state.js

taoeffect · 2024-10-13T18:57:07Z

frontend/model/state.js

@@ -101,6 +169,7 @@ sbp('sbp/selectors/register', {
      if (!ourIdentityContractId || !state[ourIdentityContractId]?.groups) return
      Object.entries(state[ourIdentityContractId].groups).map(([groupID, { hasLeft }]: [string, Object]) => {
        if (hasLeft || !state[groupID]?.chatRooms) return undefined
+        if (Array.isArray(contractIDHints) && !contractIDHints.includes(groupID)) return undefined


Isn't the point of the wrappedUpdateFn to handle this?

If this line is needed, can you add a 1-line comment above it explaining why wrappedUpdateFn is insufficient?

I'll add a comment. It's not sufficient because wrappedUpdateFn is filtering contracts by type, while this is filtering out contracts (groups) that haven't been recently added.

taoeffect · 2024-10-13T19:02:23Z

frontend/model/state.js

@@ -90,7 +158,7 @@ sbp('sbp/selectors/register', {
      // $FlowFixMe[incompatible-call]
      Vue.set(state, 'reverseNamespaceLookups', Object.fromEntries(Object.entries(state.namespaceLookups).map(([k, v]: [string, string]) => [v, k])))
    }
-    (() => {
+    contractUpdate(state, (state: Object, contractIDHints: ?string[]) => {


I asked Claude if it could make the code from line 170 to 182 more readable, and here's what it suggested:

Object.entries(state[ourIdentityContractId].groups) .filter(([groupID, { hasLeft }]) => { return !hasLeft && state[groupID]?.chatRooms && (!Array.isArray(contractIDHints) || contractIDHints.includes(groupID)) }) .map(([groupID]) => { // $FlowFixMe[incompatible-use] const chatRooms = state[groupID].chatRooms const needsUpgrade = Object.values(chatRooms) .flatMap(({ members }) => Object.values(members)) .some(member => member.status === PROFILE_STATUS.ACTIVE && member.joinedHeight == null ) return needsUpgrade ? groupID : null }) .filter(Boolean)

If this accomplishes the same thing, do you think it's safe to replace it with this?

Maybe, I'd need to see if they do the same thing or not. So far, it seems like it might do the same thing.

frontend/model/state.js

taoeffect · 2024-10-13T19:10:24Z

frontend/views/containers/group-settings/InvitationsTable.vue

-      const minutes = Math.ceil(remainder / MIL_MIN)
+      if (expiryTime == null) return L("Doesn't expire")
+      const { expired, years, months, days, hours, minutes } = timeLeft(expiryTime)
+      if (expired) L('Expired')


Missing return here

taoeffect · 2024-10-13T19:11:38Z

Gruntfile.js

@@ -223,7 +224,8 @@ module.exports = (grunt) => {
        'process.env.MAX_EVENTS_AFTER': `'${MAX_EVENTS_AFTER}'`,
        'process.env.NODE_ENV': `'${NODE_ENV}'`,
        'process.env.EXPOSE_SBP': `'${EXPOSE_SBP}'`,
-        'process.env.ENABLE_UNSAFE_NULL_CRYPTO': `'${ENABLE_UNSAFE_NULL_CRYPTO}'`
+        'process.env.ENABLE_UNSAFE_NULL_CRYPTO': `'${ENABLE_UNSAFE_NULL_CRYPTO}'`,
+        'process.env.UNSAFE_TRUST_ALL_MANIFEST_SIGNING_KEYS': UNSAFE_TRUST_ALL_MANIFEST_SIGNING_KEYS


Inconsistent handling of this environment variable, could you please wrap it the same way as the others?

but this is a boolean, not a string

So is ENABLE_UNSAFE_NULL_CRYPTO, I don't think it matters

It does matter (although I can see the argument for consistency). See:

if (process.env.ENABLE_UNSAFE_NULL_CRYPTO === 'true' && ...

If you make it a string, then it's a string. If you don't quote it, it's whatever the type of that expression is.

I see. Well, can we make it consistent anyway, for consistency's sake?

taoeffect

Another round of review ready!

SebinSong and others added 15 commits September 11, 2024 10:15

increase the life of anyone-invite-link

96e6592

update readableExpiryInfo method (incomplete)

abe1666

some refactoring in GroupMembers.vue and ProposalTemplate.vue

655bbfc

add generateInviteImmediately() method / make sure the invite works

c1f86a4

Merge remote-tracking branch 'origin/master' into sebin/task/#2341-an…

4c72344

…yone-can-join-invite-issue

Merge remote-tracking branch 'origin/master' into sebin/task/#2341-an…

f638fb7

…yone-can-join-invite-issue

fix the enter issue / update submit-btn text

2621b53

revert the invite life

f9ec6c5

update readable invite-life logic

95f1edc

update the period display expression again

43e557e

Merge remote-tracking branch 'origin/master' into sebin/task/#2341-an…

8a437e4

…yone-can-join-invite-issue

update comment

c80ddeb

Merge remote-tracking branch 'origin/master' into sebin/task/#2341-an…

709daac

…yone-can-join-invite-issue

implement gi.app/group/fixAnyoneCanJoinLink and use it

b31dd3e

Invite expires. TODO: Update exisiting invites

eed98aa

taoeffect reviewed Sep 30, 2024

View reviewed changes

frontend/views/containers/group-settings/InvitationsTable.vue Outdated Show resolved Hide resolved

corrideat marked this pull request as ready for review October 2, 2024 17:47

corrideat force-pushed the corrideat/task/#2341-anyone-can-join-invite-issue branch from 1d7a207 to 196a7b9 Compare October 2, 2024 17:47

corrideat requested a review from SebinSong October 2, 2024 17:49

Recreate anyone can join

1459e8a

corrideat force-pushed the corrideat/task/#2341-anyone-can-join-invite-issue branch from 196a7b9 to 1459e8a Compare October 2, 2024 18:09

taoeffect reviewed Oct 3, 2024

View reviewed changes

frontend/model/contracts/shared/time.js Outdated Show resolved Hide resolved

frontend/model/contracts/shared/time.js Outdated Show resolved Hide resolved

corrideat added 2 commits October 4, 2024 13:09

Fields for readable date

bd56e10

Merge branch 'master' into corrideat/task/#2341-anyone-can-join-invit…

ba54c9b

…e-issue

taoeffect requested changes Oct 4, 2024

View reviewed changes

Fix types

c93c63b

taoeffect reviewed Oct 9, 2024

View reviewed changes

frontend/controller/actions/group.js Outdated Show resolved Hide resolved

taoeffect reviewed Oct 9, 2024

View reviewed changes

frontend/views/utils/time.js Show resolved Hide resolved

taoeffect requested changes Oct 9, 2024

View reviewed changes

corrideat force-pushed the corrideat/task/#2341-anyone-can-join-invite-issue branch from e8ba426 to 7900236 Compare October 10, 2024 19:22

postUpgradeVerification improvements

330624d

corrideat force-pushed the corrideat/task/#2341-anyone-can-join-invite-issue branch from 7900236 to 330624d Compare October 10, 2024 19:33

Filter for valid invites

1011aac

taoeffect reviewed Oct 11, 2024

View reviewed changes

frontend/model/contracts/shared/time.js Outdated Show resolved Hide resolved

Revert type changes

74b69d1

taoeffect reviewed Oct 11, 2024

View reviewed changes