Celebrity Detection by BlairBeverly · Pull Request #23 · wearenewpublic/np-platform

BlairBeverly · 2024-10-31T15:10:23Z

DO NOT SUBMIT - just looking for feedback
demo video: https://drive.google.com/file/d/1n8r3pTqSvKp34a0Umk-FbQMLOatM3S6I/view?usp=sharing

Adds admin screen to verify users
Does gpt check of first time login users to determine if they're a celebrity
If they are a celebrity, checks verified users list to determine whether to verify
Adds verification_status to persona and persona preview

Need feedback:
This is my first time managing data across instances:

data from login structure is changing data in profile structure
in the case of admins, you have users changing the persona preview data of other users when there isn't necessarily even an account created yet for the email provided. in particular, please let me know if the approach i'm taking in setVerficationStatusOnPersonaAsync is OK where I'm getUserByEmailAsync and updating objects remotely.

robennals

Here are some comments.

Things mostly look sensible. The main issue is that we need to change the server API so that names and verification status are secure and can't just get updated by untrusted client code.

robennals · 2024-10-31T16:53:12Z

client/feature/ProfilePhotoAndName.js

        <Pad size={8} />
        <RadioGroup value={nameMode} onChange={onChangeNameMode}>
            <RadioOption radioKey='full' text={fbUser.displayName} />
+            {personaPreview.verificationStatus === 'unverified' && <View style={{backgroundColor: colorLightBlueBackground, borderRadius: 8}}>


As a minor readability thing, can we push the verification status into it's own component?

as discussed, will do this later.

robennals · 2024-10-31T16:54:06Z

client/feature/UserVerificationFeature.js

+        <Pad />
+        <AddVerifiedUser onUsersAdded={() => setRefreshKey(refreshKey + 1)}/>
+        <Pad />
+        <Heading level={3} label='Already Verified Users' />


Do we also want the ability to remove verified users, in case the admin made a mistake?

as discussed, will do this later.

robennals · 2024-10-31T16:55:04Z

client/feature/UserVerificationFeature.js

+        <Pad />
+        <Heading level={3} label='Already Verified Users' />
+        <VerifiedUserList verifiedUsers={verifiedUsers} />
+    </ConversationScreen>


Should this screen also show a list of recent people who have triggered celebrity detection but weren't marked as verified?

as discussed, will do this later.

robennals · 2024-10-31T17:02:25Z

client/structure/login.js

            'instance', personaKey, 'global', 'preview']);
+        const fbUser = datastore.getFirebaseUser();            
        if (!personaPreview?.name) {
+            const isCelebrity = await datastore.callServerAsync(


I think we probably actually need to do the celebrity check as part of getOrCreateUser whenever a user is created, as well as in whatever server side call updates the name.

A user who is banging the server API could potentially bypass the "set up your profile" screen, causing their name to default to whatever the SSO provider set.

as discussed, will do this later.

robennals · 2024-10-31T17:03:57Z

client/structure/login.js

+            const isCelebrity = await datastore.callServerAsync(
+                'verify', 'getIsCelebrity', {name: fbUser.displayName});
+            if (isCelebrity) {
+                await datastore.callServerAsync('verify', 'setVerficationStatusOnPersona', {email: fbUser.email})


Email passed from the client isn't secure. Better for the server side to grab the email of the account.

robennals · 2024-10-31T17:06:16Z

server/component/verify.js

+    const encodedEmail = stringToFbKey(email.toLowerCase());
+    const verificationStatus = verifiedUsers.includes(encodedEmail) ? "verified" : "unverified";
+
+    const userRecord = await getUserByEmailAsync(email);    


Rather that setting these fields directly, we should probably call updateProfileAsync.

That also does the work up updating the previews on all the instances, which this seems to be missing.

robennals · 2024-10-31T17:07:26Z

server/component/verify.js

+    const oldPreview = await serverstore.getRemoteGlobalAsync({...path, key: 'preview', });
+    serverstore.setRemoteGlobal(
+        {...path, key: 'preview', value: { ...oldPreview, verificationStatus}});
+    serverstore.updateRemoteObject(


You also need to update the preview in all the instances that the user has participated in.
But best to do that by calling updateProfileAsync rather than duplicating here.

robennals · 2024-10-31T17:08:35Z

server/component/verify.js

+exports.getIsCelebrityAsync = getIsCelebrityAsync;
+
+async function setVerficationStatusOnPersonaAsync({serverstore, email}) {
+    const verifiedUsers = await serverstore.getModulePrivateAsync('verify', ['verifiedUsers']) || [];


We should also make updateProfileAsync no longer accessible by the client, since that would allow an untrusted client to change the verification status of a user. Let's make a change to drop that method and just have a dedicated method for each field we care about, where all of those server methods do whatever check is needed, and either return an error message (to show to the user) or call updateProfileAsync (which would now be an internal-only method).

as discussed, will do this later.

robennals · 2024-10-31T17:09:07Z

server/prompts/celebrity_name_check.txt

@@ -0,0 +1,4 @@
+Is "{{name}}" commonly recongized as a celebrity, even a minor one, in any country?


Not blocking, but we should have an email dataset for this prompt.

as discussed, will do this later.

robennals · 2024-10-31T17:17:23Z

server/util/firebaseutil.js

+function getUserByEmailAsync(email) {
+    return admin.auth().getUserByEmail(email);
+}
+


We probably also need to modify addCurrentUser from datastore.js so that can't just set fake instance data. That shouldn't be hard because we already call linkInstance - so the code can go in there. We just need to replace setObject with setLocalObjectView (doesn't exist yet), and update the database access rules in psi-product

as discussed, will do this later.

robennals

Mostly looking good, but a few issues:

It seems that the badge is always showing the verification status for the logged in user, not the user whose profile photo is being shown. Unit tests should also cover this case.
Can you use profilePhotoLayers rather than modifying people.js
Can you put the verification badge into a feature module. This will better isolate code, and make it easier for us to play around with different UI options.

robennals · 2024-11-04T19:02:32Z

client/component/people.js


    return <View style={{ width: size, height: size, borderRadius: size / 2, backgroundColor: color, justifyContent: 'center', alignItems: 'center' }}>
        <Text style={[{ fontSize: size / 2 }, s.letter]}>{letter}</Text>
+        <View style={{ position: 'absolute', right: -badgePad, bottom: 0 }}>


Is there a reason you aren't doing this with profilePhotoLayers?

That config slot was added specifically to allow enhancements like this.

robennals · 2024-11-04T19:03:07Z

client/component/people.js

+    },
+});
+
+function Badge({type, name}) {


Can we put this in a feature module, rather than in the core "people" component?

You can use profilePhotoLayers to add the badge.

robennals · 2024-11-04T19:14:09Z

client/demo/login-demo.js

+        personaPreview: {name:'Tom Cruise', verificationStatus: 'verified'},
+        stories: [
+            {label: "Don't show message for verified name", actions: [
+                CHECK_TEXT_ABSENCE(verificationMessage)


We can keep this if you like, but in general I prefer to rely on the storybook snapshots to check that the right stuff is shown. Otherwise one can quickly end up with a lot of checks.

Agree there's the possibility for lots of checks, but I can be disciplined about not doing that. So far I've had trouble using the storybook snapshots effectively-- its often many hundreds of lines of html output to sift through and its not trivial to distinguish which diffs are accidental or on purpose. I feel a lot better just having an explicit check for the things that matter.

robennals · 2024-11-04T20:02:22Z

client/component/people.js

+    const datastore = useDatastore();
+    const fbUser = datastore.getFirebaseUser();
+    const personaPreview = usePersonaPreview();
+    const verificationStatus = personaPreview?.verificationStatus ?? null;


As written, it seems the badge will always be showing the verification status of the user who is logged in. Don't we actually want it to be showing the verification status of the user who we are showing a profile photo for?

thanks for catching this. fixed.

robennals · 2024-11-04T20:03:06Z

client/component/people.js

-            <IconCircleCheck />
-        </View>}
+        <View style={{ position: 'absolute', right: -badgePad, bottom: 0 }}>
+            <Badge type={type} name={name} />


This call isn't saying what user the badge should be shown for. The badge should be the status of the user whose face is being shown, not of the logged in user.

robennals · 2024-11-04T20:05:53Z

client/component/people.js

+        return null;
+    }
+
+    if (verificationStatus === 'verified') {


Note that the verification status is the status of their name, so we only want to show the verification badge if we are also showing their real name, rather than a pseudonym.

Right now the photo editing UI doesn't record whether the 'name' is a pseudonym. We could either have a field to check that, or we could look at the text and see if it contains any uppercase letters or spaces.

i was solving for this case by checking (fbUser?.displayName !== persona?.name), assuming if the name is different from the firebase user name then they've set a psuedonym. does that not work?

fbUser is the fbUser record of the logged in user. But we are showing the badge for a different user. So this check won't work.

robennals · 2024-11-04T20:07:45Z

client/demo/designsystem-demo.js

                </Datastore>
            </DemoSection>
+            <DemoSection label='Verified Profile Photo'>
+            <Datastore personaPreview={{ verificationStatus: 'verified' }}>


This seems to only be testing the case where we show the badge of the current logged in user for an instance that they aren't attached to.

We should test that use case, but the most important use case is the case where you are seeing the profile photo for a different user - and in that case, their persona info will be in the persona collection.

robennals · 2024-11-04T20:11:11Z

client/component/people.js

            if (profilePhotoLayers?.length > 0) {                
                return <> 
-                    <FaceImage face={face} photoUrl={photo ?? persona?.photoUrl} type={type} border={border} faint={faint} check={check} />
+                    <FaceImage face={face} photoUrl={photo ?? persona?.photoUrl} type={type} border={border} faint={faint} />


You've added a "name" property to FaceImage, but aren't passing it in these cases. Is that intentional?

added the name so i could do the pseudonym check. i'm now just getting the name from the persona object so not passing this anymore.

robennals · 2024-11-04T20:15:03Z

client/component/people.js

                </>
            } else {
-                return <FaceImage face={face} photoUrl={photo ?? persona?.photoUrl} type={type} border={border} faint={faint} check={check} />
+                return <FaceImage face={face} photoUrl={photo ?? persona?.photoUrl} type={type} border={border} faint={faint} />


This reminds me that we probably need to show profilePhotoLayers for LetterFace and AnonymousFace (which I don't think we are actually using right now) too - since a person with a celebrity name may not be showing a profile photo.

If we do that, then we probably want to turn the photo layers into a component that can easily wrap any photo, so we can easily re-use the same code in all these locations - and also in the login screen.

BlairBeverly · 2024-11-06T01:45:25Z

Fixed the issue where it was always checking the logged in users profile for verification status
Created a ProfileWithLayers that wraps around all the face components (LetterFace, FaceImage, etc)
Am using the profilePhotoLayers config
Setting verification badging as a feature

I am still having a couple small issues, but I thought I'd just get this out at a reasonable stopping point so I can move on to the higher priority things. Some outstanding issues:

I was not able to figure out how to show the verification badge in the component demo for the logged in user in the top bar. Note in the real psi-product, the badge shows correctly. It's just in the component demo for some reason
I was not able to figure out how to show the verification badges during the FirstLoginSetup. For some reason the profilePhotoLayers config is coming up undefined in this context and I don't understand why.

robennals

Making progress, but it seems this still has a few issues:

The name check to see if this is a real name or pseduonym is comparing to the name of the logged in user, not the name of the user whose badge we are showing
This is being done with the PhotoAndName module, which is only supposed to be applied to the profile structure. The verification badge should be it's own module.

robennals · 2024-11-06T18:44:22Z

client/component/people.js

+        return null;
+    }
+
+    if (verificationStatus === 'verified') {


fbUser is the fbUser record of the logged in user. But we are showing the badge for a different user. So this check won't work.

robennals · 2024-11-06T18:50:58Z

client/component/people.js

-                return <FaceImage face={face} photoUrl={photo ?? persona?.photoUrl} type={type} border={border} faint={faint} check={check} />
-            }
+        if (face || photo || persona?.photoUrl) {      
+            return <ProfileWithLayers faceComponent={FaceImage} face={face} persona={persona}


Not a blocker, but I'd have implemented this using {children} rather that faceComponent.

E.g.

<ProfileOverlay personaKey={personaKey} type={type}> <FaceImage ... /> </ProfileOverlay>

robennals · 2024-11-06T18:55:03Z

client/demo/designsystem-demo.js

 import { CLICK, DemoSection, POPUP, SpacedArray } from "../system/demo";
 import { Image, View } from "react-native";
 import { FeatureMenuScreen } from "./featuremenu-demo";
+import { VerificationBadge } from "../feature/ProfilePhotoAndName";


Not a blocker, but this feels like a separate feature from ProfilePhotoAndName - which is about giving you the ability to edit your profile photo and name from a profile. Might be better in a separate module file.

robennals · 2024-11-06T18:58:26Z

client/feature/ProfilePhotoAndName.js

    }
 }

+export function VerificationBadge({persona, size}) {


Not a blocker, but as a general principle, it's usually better to pass a key (e.g. personaKey) rather than an object (e.g. persona).

When doing react re-renders, keys are strings and are compared by value, while objects are compared by reference. Thus using keys makes it easier to avoid unwanted re-renders.

That said, this is somewhat moot right now because all data access hooks currently refresh all components for every data change - though I'm planning to do that nicer in the future.

robennals · 2024-11-06T18:58:56Z

client/feature/ProfilePhotoAndName.js


+export function VerificationBadge({persona, size}) {
+    const datastore = useDatastore();
+    const fbUser = datastore.getFirebaseUser();


getFirebaseUser is getting the user record for the logged in user, not the user the badge is being shown for.

robennals · 2024-11-06T19:00:20Z

client/feature/ProfilePhotoAndName.js

+    const fbUser = datastore.getFirebaseUser();
+    const verificationStatus = persona?.verificationStatus ?? null;
+
+    if (fbUser?.displayName !== persona?.name) {


This will always be true, except for the badge of the currently logged in user. I don't think that's what you want.

I think you probably want to check if the name starts with a capital letter - and maybe also enforce that for real names in the setup screen.

robennals · 2024-11-06T19:03:02Z

client/feature/ProfilePhotoAndName.js

            makePreview: previewPhotoAndName,
-        }]
+        }],
+        profilePhotoLayers: [VerificationBadge]


I think this should be a different feature.

ProfilePhotoAndName is a feature applied to profiles, that makes the profile names and photos editable.

The feature you are writing is applied to conversations, and shows a badge on people's photos.
These should probably be different features.

We currently don't have a feature list what structures they apply to, since that would get awkward features that add functionality to things like bylines that apply to multiple structures, but it's possible it would make sense to have some kind of convention for documenting what we think a feature is supposed to be applied to.

robennals · 2024-11-06T19:04:26Z

client/structure/profile.js

    defaultConfig: {
        profileWidgets: [],
        profileModules: [],
+        profilePhotoLayers: [VerificationBadge],


Why is this here? Wouldn't we just turn on the verification feature on profiles by default, rather than having this hard-coded into the structure?

We do need to add profilePhotoLayers to the structure, so it's accessible as a config slot, but I think it should default to empty.

BlairBeverly added 2 commits October 31, 2024 08:03

Celebrity Detection

3bd29ed

Merge branch 'main' of https://github.com/BlairBeverly/np-platform

053b2c5

robennals requested changes Oct 31, 2024

View reviewed changes

BlairBeverly and others added 9 commits November 1, 2024 18:40

Merge branch 'wearenewpublic:main' into main

d05d1db

get checkmark working, unit tests

33ebb6e

Merge branch 'main' of https://github.com/BlairBeverly/np-platform

c234aea

remove consoles

e47dcdf

uncomment checktext

7638780

update setVerificationStatusAsync test

555c0ed

update tests

d02215e

Merge branch 'wearenewpublic:main' into main

045b709

Merge branch 'main' of https://github.com/BlairBeverly/np-platform

9395dae

robennals requested changes Nov 4, 2024

View reviewed changes

BlairBeverly and others added 3 commits November 5, 2024 17:32

updates

16e8f70

Merge branch 'wearenewpublic:main' into main

9cbd71d

Merge branch 'main' of https://github.com/BlairBeverly/np-platform

3b94f0b

robennals requested changes Nov 6, 2024

View reviewed changes

Merge branch 'wearenewpublic:main' into main

2fa3bb8

		@@ -0,0 +1,4 @@
		Is "{{name}}" commonly recongized as a celebrity, even a minor one, in any country?

Conversation

BlairBeverly commented Oct 31, 2024

Uh oh!

robennals left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robennals left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment