Skip to content

update ypp suspension status#44

Merged
zeeshanakram3 merged 12 commits intoJoystream:mainfrom
zeeshanakram3:ypp_suspension
Nov 19, 2022
Merged

update ypp suspension status#44
zeeshanakram3 merged 12 commits intoJoystream:mainfrom
zeeshanakram3:ypp_suspension

Conversation

@zeeshanakram3
Copy link
Copy Markdown
Contributor

implements #34

}

@Put(':joystreamChannelId')
@Put('/yt-syncing/:joystreamChannelId')
Copy link
Copy Markdown
Contributor

@kdembler kdembler Oct 17, 2022

Choose a reason for hiding this comment

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

yt-syncing is not a resource, so it shouldn't be in the path. I suggest we just keep it /channels/:channelId

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, I also initially thought of keeping the endpoint like /channels/:channelId. However, we actually need two different endpoints for changing two different fields/values (shouldBeIngested & isSuspended) of the same resource (which is the channel record) by different actors, having different privileges.

So maybe two endpoints like the following make sense?

  • /channels/ingest/:channelId -> callable by channel owners for changing channel's ingestion status, would change shouldBeIngested field
  • /channels/suspend/:channelId -> callable by YPP infra owner to suspend channel's YPP status, would change isSuspended field

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure sounds good but I would change the order to /channels/:channelId/ingest and same for suspend

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 2aad464

description: `Authenticated endpoint to suspend given channel/s from YPP program`,
})
async suspendChannels(
@Headers('ypp_owner_key') yppOwnerKey: string,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should use standard Authorization header for this, it may get special treatment from some clients

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 2aad464

this.channelsService.update({ ...channel, isSuspended, shouldBeIngested: false })
} else {
// if channel suspension is revoked then its YT ingestion/syncing should be resumed
this.channelsService.update({ ...channel, isSuspended, shouldBeIngested: true })
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one's a bit more tricky - if the user has opt out from YT sync and then got suspended, once they get unsuspended, we probably shouldn't enable YT sync back. WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, so I think irrespective of the previous YT sync status, whether enabled or disabled, if a channel gets unsuspended, its YT sync should always be in the disabled state. And it should be the responsibility of the channel owner to turn it back on?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds good

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 2aad464

@zeeshanakram3 zeeshanakram3 merged commit ccfdaac into Joystream:main Nov 19, 2022
@dmtrjsg dmtrjsg mentioned this pull request Dec 15, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants