ausocean / cloud

GNU General Public License v3.0
1 stars 1 forks source link

model: Site entity should include YouTubeEmail #188

Open scruzin opened 6 days ago

scruzin commented 6 days ago

At present, the YouTube account email is associated with the broadcast config, which means it is handled every time a configuration is updated, usually unnecessarily. The principle of data minimization suggests that we rethink this approach.

On the reasonable assumption that the given Site is only ever associated with one YouTube account, the YouTube account email should move to the Site entity, along with the OwnerEmail (the Google account associated with the site).

saxon-milton commented 5 days ago

In what way is the account handled every time the configuration is updated ? I don't necessarily think this is the case, but maybe to some level it's unavoidable with how we're doing things.

I think there might still be circumstances where we want an AusOcean managed site, but have one of the cameras going to someone elses channel ?

scruzin commented 5 days ago

The account is part of the broadcast config but, AFAIK, is only required to generate a YT access token. In contrast, all other properties of the broadcast config are inherent properties of the broadcast.

Further, a given premium customer, although they may have multiple broadcasts, will in all likelihood use the same YT account for all of them (just as we do).

We can still have an AusOcean-managed site, but specify a YT account email for that site that is different to our standard AusOcean YT channel.

saxon-milton commented 5 days ago

The account field in the BroadcastConfig is also used to retrieve the token from a bucket object, which happens on the OceanTV side of things.

I'm thinking about the (maybe rare) case that we have a site with most broadcasts going to AusOcean's channel, but then one of them going somewhere else.

scruzin commented 5 days ago

We could handle that case by creating an extra site for that purpose. Seems like a rare use case, though.

saxon-milton commented 5 days ago

Oops, that close was unintentional...

scruzin commented 5 days ago

Just to clarify my earlier comment about data minimization, the general idea with schema design is factor out common data to avoid duplication. For example, Device entities include neither the email address of the device's owner, nor the name of the site. Instead, this common data is part of the Site entity, which is shared by multiple devices. Similarly, there will typically be multiple broadcasts per site, all using the same account (enforced by policy, if not technically).

As a bonus, moving the YT account email to the Site means it comes accessible to usage outside of just the broadcast manager. For example, we could send emails to YT account owners to advise them of YT policy changes, etc.

saxon-milton commented 5 days ago

Just to clarify my earlier comment about data minimization, the general idea with schema design is factor out common data to avoid duplication. For example, Device entities include neither the email address of the device's owner, nor the name of the site. Instead, this common data is part of the Site entity, which is shared by multiple devices. Similarly, there will typically be multiple broadcasts per site, all using the same account (enforced by policy, if not technically).

As a bonus, moving the YT account email to the Site means it comes accessible to usage outside of just the broadcast manager. For example, we could send emails to YT account owners to advise them of YT policy changes, etc.

I think this all makes sense, and that we should move towards it. We'll have to consider that whenever we use the YouTube API we'll need to retrieve the site entity using the site key, and then get the account so that we can get the token from the corresponding bucket object. This is a little bit more involved (and uses more datastore operations) than just using the account stored in the config, but maybe that doesn't matter.

The proportion of value vs work to do this is probably in line with P2, but maybe possibly P3 ?

scruzin commented 5 days ago

Implementing Issue #185 would certainly be easy with this change.

Basically, we'd just need to call notifier.Init with the new WithRecipientLookup option, passing in a function similar to the following:

func(skey int64, kind Kind) []string {
        site, err := mode.GetSite(skey)
        switch kind {
        case "broadcast-generic", "broadcast-hardware", "broadcast-nework": //etc.
                return []{ site.OpsEmail, site.YouTubeEmail }
        default:
                return []{ site.OpsEmail }}
        }
}

Or we can wait until @ao-david implements his proposal to add a token generation endpoint to Ocean TV.

PS OpsEmail is a new property I envision adding to the Site, rather than depending on env vars.

scruzin commented 5 days ago

Actually, at the cost of temporary duplication, we can add YouTubeEmail (and OpsEmail) to Site and manually populate those fields for now, while we transition the broadcast manager to use the former.

saxon-milton commented 5 days ago

Implementing Issue #185 would certainly be easy with this change.

Basically, we'd just need to call notifier.Init with the new WithRecipientLookup option, passing in a function similar to the following:

func(skey int64, kind Kind) []string {
        site, err := mode.GetSite(skey)
        switch kind {
        case "broadcast-generic", "broadcast-hardware", "broadcast-nework": //etc.
                return []{ site.OpsEmail, site.YouTubeEmail }
        default:
                return []{ site.OpsEmail }}
        }
}

Or we can wait until @ao-david implements his proposal to add a token generation endpoint to Ocean TV.

PS OpsEmail is a new property I envision adding to the Site, rather than depending on env vars.

Yeah true, with that in mind the change probably makes even more sense; in line with P2 or even P1.