launchpad-reviewers team mailing list archive

[Ines Almeida <mp+457527@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/registry/interfaces/socialaccount.py b/lib/lp/registry/interfaces/socialaccount.py
> new file mode 100644
> index 0000000..30294ab
> --- /dev/null
> +++ b/lib/lp/registry/interfaces/socialaccount.py
> @@ -0,0 +1,89 @@
> +# Copyright 2023 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""SocialAccount interfaces."""
> +
> +__all__ = ["ISocialAccount", "ISocialAccountSet", "PlatformType"]
> +
> +from lazr.enum import DBEnumeratedType, DBItem
> +from lazr.restful.declarations import exported, exported_as_webservice_entry
> +from lazr.restful.fields import Reference
> +from zope.interface import Interface
> +from zope.schema import Choice, Dict, Int, TextLine
> +
> +from lp import _
> +from lp.registry.interfaces.role import IHasOwner
> +
> +
> +class PlatformType(DBEnumeratedType):
> +    """Platform Type

Would maybe write "Social Media Platform Type" here, to make it clearer for someone reading

> +
> +    Social Account is associated to a PlatformType.
> +    """
> +
> +    MATRIX = DBItem(
> +        1,
> +        """
> +        Matrix platform
> +
> +        The Social Account will hold Matrix account info.
> +        """,
> +    )
> +
> +    JABBER = DBItem(

I think we will want to add jabber to this list, but maybe we should create the whole thing only for Matrix for now, and only then migrate the remaining. Having Jabber in 2 places might be confusing, otherwise. Thoughts?

> +        2,
> +        """
> +        Jabber platform
> +
> +        The Social Account will hold Jabber account info.
> +        """,
> +    )
> +
> +
> +@exported_as_webservice_entry(as_of="beta")
> +class ISocialAccount(IHasOwner):
> +    """Social Account"""
> +
> +    id = Int(title=_("Database ID"), required=True, readonly=True)
> +    # schema=Interface will be overridden in person.py because of circular

This is kind of annoying. But I see it's done in the other social account interfaces

> +    # dependencies.
> +    person = exported(
> +        Reference(
> +            title=_("Owner"), required=True, schema=Interface, readonly=True
> +        )
> +    )
> +
> +    platform = exported(
> +        Choice(
> +            title=_("Platform type"),

Perhaps "Social media platform" instead?

> +            required=True,
> +            vocabulary=PlatformType,
> +            default=PlatformType.MATRIX,

Hmmm, not sure about having a default value. Doesn't bother me that much, but seems odd to have a default platform

> +        )
> +    )
> +
> +    identity = exported(
> +        Dict(
> +            title=_("SocialAccount identity"),

Maybe just "Identity"?

> +            key_type=TextLine(),
> +            required=True,
> +            readonly=False,
> +            description=_(
> +                "A dictionary mapping attributes for the social media account "
> +                "(JSON format specific per social media platform). "
> +            ),
> +        )
> +    )
> +
> +    def destroySelf():
> +        """Delete this SocialAccount from the database."""
> +
> +
> +class ISocialAccountSet(Interface):
> +    """The set of SocialAccounts."""
> +
> +    def new(self, person, platform, identity):
> +        """Create a new SocialAccount pointing to the given Person."""
> +
> +    def getByPerson(person):
> +        """Return all SocialAccounts for the given person."""


-- 
https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/457527
Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad:social-account-interface-and-implementation into launchpad:master.