Skip to content
This repository has been archived by the owner on Mar 2, 2021. It is now read-only.

WIP: Feat/user channel create delete #68

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

4www
Copy link

@4www 4www commented May 21, 2020

Handle:

  • on register user: create userSettings and associate with user
  • on delete user: delete user settings
  • on delete user: delete user channel(s)
  • on channel create: create channel public and association with channel
  • on channel create: channel.slug = slugify(channel.title)
  • on channel delete: delete channel.channelPublic
  • on channel delete: channel.channelPublic.followers.forEach( delete favorites[channel]
  • on channel edit: channel.slug = slugify(channel.slug || channel.title)

No so easy to test functions locally, and deploy. So many google, gcloud, firebase login things, service accounts. Heavy to maintain and document, have to put CI/CD, but that as well. Having to go through the service accounts and api access interfaces, be sure that everything use the correct profiles, re-generate everything to rotate-keys...

@4www 4www requested a review from oskarrough May 21, 2020 10:02
Copy link
Member

@oskarrough oskarrough left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So many things! Really cool. Still didn't get to test it, but found a few things to comment on anyway.

All of this also assumes the R4 frontend is updated, so it doesn't try to do everything twice?

README.md Outdated Show resolved Hide resolved
@@ -98,7 +98,7 @@
// write: only the user owner can write a channel
// write: only a user with no channel can write a new one to himself
".write": "auth != null && (root.child('users').child(auth.uid).child('channels').child($channelID).exists() || (!data.exists() && !root.child('users').child(auth.uid).child('channels').exists()))",
".validate": "newData.hasChildren(['slug', 'title', 'created']) || !newData.exists()",
".validate": "newData.hasChildren(['title']) || !newData.exists()",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe slug was removed because it's now generated in the backend, but what about created?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should generate it in the Backend as well. My mistake!

functions/channel.js Outdated Show resolved Hide resolved
// create new /users/:newUser
let userRef = admin.database().ref(`/users/${userUid}`)
userRef.set({
settings: userSettingsId,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't createUserSetting be part of this method? So you don't have to call both. Also, if you don't have a userSetting, how can you create a user?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the firebase.function.aut.onRegister = handleUserCreate
is splitted in functions that handle small part of this process

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm confused. So what's the steps to create a new user from a front-end perspective?

var userRef = await database.ref('/users').push()
userRef.set({someProperty: 'helloword'})

And then backend takes care of the usersetting?

Comment on lines +29 to +32
let userRef = admin.database().ref(`/users/${userUid}`)
return await userRef.once('value').then(dataSnapshot => {
return dataSnapshot.val()
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stick to either await or then?

Suggested change
let userRef = admin.database().ref(`/users/${userUid}`)
return await userRef.once('value').then(dataSnapshot => {
return dataSnapshot.val()
})
let userRef = admin.database().ref(`/users/${userUid}`)
let snapshot = await userRef.once('value')
return snapshot.val()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, but here the second await is not needed, as everything happens sequentially in the then

console.log('channels', channels)
if (!channels) {
return
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to delete the channels.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ja!

when a channel is deleted
*/

const handleChannelDelete = async (change, context) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor but move this method to the end of the file? So it reads create, update, delete.

Comment on lines 105 to 107
await userChannelRef.update({
slug: slugify(title)
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inside R4 front-end we validate slug to make sure it's unique. Guess we need to do that here before updating?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is totally correct! Glad you are looking at the file!

.firebaserc Outdated
"staging": "radio4000-staging"
"staging": "radio4000-staging",
"dev": "radio4000-hugurp",
"default": "radio4000-hugurp"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is default used anywhere or can we remove?

functions/user.js Show resolved Hide resolved
functions/user.js Show resolved Hide resolved
functions/user.js Show resolved Hide resolved

let updates = {}
Object.keys(followers).forEach(followerId => {
updates[`/channels/${followerId}/favoriteChannels/${channelId}`] = null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does channelId come from?

@4www 4www changed the title Feat/user channel create delete WIP: Feat/user channel create delete May 25, 2020
Comment on lines +166 to +168
nvm install 10.10.0 // install node 10
nvm use 10.10.0 // use node 10
npm // install npm dependencies with yarn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it need 10.10.0 or just 10.x?

Suggested change
nvm install 10.10.0 // install node 10
nvm use 10.10.0 // use node 10
npm // install npm dependencies with yarn
nvm use 10
npm install

@@ -103,7 +104,9 @@ const handleChannelCreate = async (snapshot, context) => {
// validate slug, or generate it
try {
await userChannelRef.update({
slug: slugify(title)
slug: slugify(title),
created: admin.database.ServerValue.TIMESTAMP,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so nice if we don't have to deal with timestamps in the frontend

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants