launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #33130
[Merge] ~ines-almeida/launchpad:add-github-social into launchpad:master
Ines Almeida has proposed merging ~ines-almeida/launchpad:add-github-social into launchpad:master.
Commit message:
Add GitHub as social account choice
Users can now add a new GitHub social account via browser same as they do Matrix
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/494263
Unfortunately, the UI for adding social accounts is not the best - it requires its own view per social platform (IRC, Jabber, Matrix, GitHub)
But it's at least clear, and hopefully we get to do a remake of User pages in the future
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:add-github-social into launchpad:master.
diff --git a/lib/canonical/launchpad/images/social-github.png b/lib/canonical/launchpad/images/social-github.png
new file mode 100644
index 0000000..6cb3b70
Binary files /dev/null and b/lib/canonical/launchpad/images/social-github.png differ
diff --git a/lib/lp/app/tests/test_tales.py b/lib/lp/app/tests/test_tales.py
index 52730b8..b056a67 100644
--- a/lib/lp/app/tests/test_tales.py
+++ b/lib/lp/app/tests/test_tales.py
@@ -6,6 +6,7 @@
from datetime import datetime, timedelta, timezone
from lxml import html
+from testscenarios import WithScenarios
from zope.component import getAdapter, getUtility
from zope.traversing.interfaces import IPathAdapter, TraversalError
@@ -389,27 +390,47 @@ class TestIRCNicknameFormatterAPI(TestCaseWithFactory):
)
-class TestSocialAccountFormatterAPI(TestCaseWithFactory):
+class TestSocialAccountFormatterAPI(TestCaseWithFactory, WithScenarios):
"""Tests for SocialAccountFormatterAPI"""
layer = DatabaseFunctionalLayer
+ scenarios = [
+ (
+ "Matrix",
+ {
+ "platform": SocialPlatformType.MATRIX,
+ "identity": {"username": "fred", "homeserver": "ubuntu.com"},
+ "icon": "social-matrix",
+ "href": "https://matrix.to//#/@fred:ubuntu.com";,
+ "display": "@fred:ubuntu.com",
+ },
+ ),
+ (
+ "GitHub",
+ {
+ "platform": SocialPlatformType.GITHUB,
+ "identity": {
+ "username": "fred",
+ },
+ "icon": "social-github",
+ "href": "https://github.com/fred";,
+ "display": "/fred",
+ },
+ ),
+ ]
+
def test_formatted_display(self):
"""Social account is displayed as expected."""
person = self.factory.makePerson()
social_account_set = getUtility(ISocialAccountSet)
- identity = {
- "username": "fred",
- "homeserver": "ubuntu.com",
- }
social_account = social_account_set.new(
- person, SocialPlatformType.MATRIX, identity
+ person, self.platform, self.identity
)
expected_html = (
'<img class="user-social-accounts__icon" alt="Matrix" '
- 'title="Matrix" src="/@@/social-matrix" /> '
- "<a href=https://matrix.to//#/@fred:ubuntu.com "
- 'target="_blank"><strong>@fred:ubuntu.com</strong></a>'
+ 'title="Matrix" src="/@@/{self.icon}" /> <a href={self.href} '
+ 'target="_blank"><strong>{display}</strong></a>'
)
self.assertEqual(
diff --git a/lib/lp/registry/browser/configure.zcml b/lib/lp/registry/browser/configure.zcml
index bff5d4f..fd0eb65 100644
--- a/lib/lp/registry/browser/configure.zcml
+++ b/lib/lp/registry/browser/configure.zcml
@@ -1252,6 +1252,13 @@
template="../templates/person-editsocialaccounts.pt"
/>
<browser:page
+ name="+editgithubaccounts"
+ for="lp.registry.interfaces.person.IPerson"
+ class="lp.registry.browser.person.PersonEditGitHubAccountsView"
+ permission="launchpad.Edit"
+ template="../templates/person-editsocialaccounts.pt"
+ />
+ <browser:page
name="+editjabberids"
for="lp.registry.interfaces.person.IPerson"
class="lp.registry.browser.person.PersonEditJabberIDsView"
diff --git a/lib/lp/registry/browser/person.py b/lib/lp/registry/browser/person.py
index bf3c6fd..d0af80b 100644
--- a/lib/lp/registry/browser/person.py
+++ b/lib/lp/registry/browser/person.py
@@ -20,6 +20,7 @@ __all__ = [
"PersonEditIRCNicknamesView",
"PersonEditJabberIDsView",
"PersonEditMatrixAccountsView",
+ "PersonEditGitHubAccountsView",
"PersonEditTimeZoneView",
"PersonEditSSHKeysView",
"PersonEditView",
@@ -154,6 +155,7 @@ from lp.registry.interfaces.pillar import IPillarNameSet
from lp.registry.interfaces.poll import IPollSubset
from lp.registry.interfaces.product import InvalidProductName, IProduct
from lp.registry.interfaces.socialaccount import (
+ GithubPlatform,
ISocialAccountSet,
MatrixPlatform,
SocialAccountIdentityError,
@@ -833,6 +835,7 @@ class PersonOverviewMenu(
"editircnicknames",
"editjabberids",
"editmatrixaccounts",
+ "editgithubaccounts",
"editsshkeys",
"editpgpkeys",
"editlocation",
@@ -905,6 +908,12 @@ class PersonOverviewMenu(
return Link(target, text, icon="edit", summary=text)
@enabled_with_permission("launchpad.Edit")
+ def editgithubaccounts(self):
+ target = "+editgithubaccounts"
+ text = "Edit GitHub accounts"
+ return Link(target, text, icon="edit", summary=text)
+
+ @enabled_with_permission("launchpad.Edit")
def editjabberids(self):
target = "+editjabberids"
text = "Update Jabber IDs"
@@ -1696,11 +1705,23 @@ class PersonView(LaunchpadView, FeedsMixin, ContactViaWebLinksMixin):
)
@property
+ def should_show_github_accounts_section(self):
+ """Should the github accounts section be shown?
+
+ It's shown when the person has social accounts for the Matrix platform
+ registered or has rights to register new ones.
+ """
+ return bool(self.github_accounts) or (
+ check_permission("launchpad.Edit", self.context)
+ )
+
+ @property
def should_show_socialaccounts_section(self):
return (
self.should_show_ircnicknames_section
or self.should_show_jabberids_section
or self.should_show_matrix_accounts_section
+ or self.should_show_github_accounts_section
)
@property
@@ -1737,6 +1758,12 @@ class PersonView(LaunchpadView, FeedsMixin, ContactViaWebLinksMixin):
)
@cachedproperty
+ def github_accounts(self):
+ return self.context.getSocialAccountsByPlatform(
+ SocialPlatformType.GITHUB
+ )
+
+ @cachedproperty
def is_probationary_or_invalid_user(self):
"""True when the user is not active or does not have karma.
@@ -2550,6 +2577,10 @@ class PersonEditMatrixAccountsView(PersonEditSocialAccountsView):
platform = MatrixPlatform
+class PersonEditGitHubAccountsView(PersonEditSocialAccountsView):
+ platform = GithubPlatform
+
+
class PersonEditJabberIDsView(LaunchpadFormView):
schema = IJabberID
field_names = ["jabberid"]
diff --git a/lib/lp/registry/interfaces/socialaccount.py b/lib/lp/registry/interfaces/socialaccount.py
index 4c8134d..f7f6d9e 100644
--- a/lib/lp/registry/interfaces/socialaccount.py
+++ b/lib/lp/registry/interfaces/socialaccount.py
@@ -6,6 +6,7 @@
__all__ = [
"ISocialAccount",
"ISocialAccountSet",
+ "GithubPlatform",
"MatrixPlatform",
"SocialPlatformType",
"SOCIAL_PLATFORM_TYPES_MAP",
@@ -46,6 +47,15 @@ class SocialPlatformType(DBEnumeratedType):
""",
)
+ GITHUB = DBItem(
+ 2,
+ """
+ GitHub platform
+
+ The Social Account will hold GitHub account info.
+ """,
+ )
+
# XXX pelpsi 2023-12-14 bug=760849: "beta" is a lie to get WADL generation
# working.
@@ -160,7 +170,39 @@ class MatrixPlatform(SocialPlatform):
)
+class GithubPlatform(SocialPlatform):
+ title = "GitHub"
+ identity_fields = ["username"]
+ identity_fields_example = {
+ "username": "mark",
+ "homeserver": "ubuntu.com",
+ }
+ platform_type = SocialPlatformType.GITHUB
+ icon = "social-github"
+ display_format = "<strong>/{username}</strong>"
+ url = "https://github.com/{username}";
+
+ @classmethod
+ def validate_identity(cls, identity):
+ if not all(
+ identity.get(required_field)
+ for required_field in cls.identity_fields
+ ):
+ raise SocialAccountIdentityError(
+ f"You must provide the following fields: "
+ f"{', '.join(cls.identity_fields)}."
+ )
+ if not isinstance(identity["username"], str):
+ raise SocialAccountIdentityError("Username must be a string.")
+ # GitHub username can contain a-z, 0-9, and -
+ # ref: https://docs.github.com/en/enterprise-cloud@latest/admin/managing-iam/iam-configuration-reference/username-considerations-for-external-authentication#about-username-normalization # noqa: E501
+ username_regex = r"^[A-z0-9-]+"
+ if not re.match(username_regex, identity["username"]):
+ raise SocialAccountIdentityError("Username must be valid.")
+
+
SOCIAL_PLATFORM_TYPES_MAP = {
+ SocialPlatformType.GITHUB: GithubPlatform,
SocialPlatformType.MATRIX: MatrixPlatform,
}
diff --git a/lib/lp/registry/templates/person-portlet-contact-details.pt b/lib/lp/registry/templates/person-portlet-contact-details.pt
index 906d22d..1349c71 100644
--- a/lib/lp/registry/templates/person-portlet-contact-details.pt
+++ b/lib/lp/registry/templates/person-portlet-contact-details.pt
@@ -189,6 +189,11 @@
<a tal:replace="structure overview_menu/editmatrixaccounts/fmt:icon" />
</dd>
+ <dd class="user-social-accounts__item github-account" tal:repeat="social_account view/github_accounts">
+ <span><span tal:replace="structure social_account/fmt:formatted_display" /></span>
+ <a tal:replace="structure overview_menu/editgithubaccounts/fmt:icon" />
+ </dd>
+
<tal:irc condition="view/should_show_ircnicknames_section">
<dd class="user-social-accounts__item" tal:condition="not: context/ircnicknames" id="empty-irc">
<img class="user-social-accounts__icon" alt="IRC" title="IRC" src="/@@/social-irc"/>
@@ -213,6 +218,14 @@
</dd>
</tal:matrix>
+ <tal:matrix condition="view/should_show_github_accounts_section">
+ <dd class="user-social-accounts__item" tal:condition="not: view/github_accounts" id="empty-matrix">
+ <img class="user-social-accounts__icon" alt="Matrix" title="Matrix" src="/@@/social-github" />
+ <span>No github accounts registered.</span>
+ <a tal:replace="structure overview_menu/editgithubaccounts/fmt:icon" />
+ </dd>
+ </tal:matrix>
+
</dl>
</div>
</div>
diff --git a/lib/lp/registry/tests/test_socialaccount.py b/lib/lp/registry/tests/test_socialaccount.py
index 10dba86..f9f4dd0 100644
--- a/lib/lp/registry/tests/test_socialaccount.py
+++ b/lib/lp/registry/tests/test_socialaccount.py
@@ -13,7 +13,7 @@ from lp.testing import TestCaseWithFactory, login_person
from lp.testing.layers import DatabaseFunctionalLayer
-class TestSocialAccount(TestCaseWithFactory):
+class TestMatrixSocialAccount(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
def test_social_account(self):
@@ -49,6 +49,22 @@ class TestSocialAccount(TestCaseWithFactory):
self.assertEqual(social_account.identity["homeserver"], "abc.org")
self.assertEqual(social_account.identity["username"], "test-nickname")
+ def test_github_account(self):
+ # GitHub Social Account is created as expected and
+ # associated to the user.
+ user = self.factory.makePerson()
+ attributes = {}
+ attributes["username"] = "test-nickname"
+ social_account = getUtility(ISocialAccountSet).new(
+ user, SocialPlatformType.GITHUB, attributes
+ )
+
+ self.assertEqual(len(user.social_accounts), 1)
+ social_account = user.social_accounts[0]
+
+ self.assertEqual(social_account.platform, SocialPlatformType.GITHUB)
+ self.assertEqual(social_account.identity["username"], "test-nickname")
+
def test_multilevel_domain_matrix_account(self):
# Homeserver with a multi-level domain is allowed
# Matrix username can contain a-z, 0-9, ., _, =, -, and /
@@ -173,6 +189,21 @@ class TestSocialAccount(TestCaseWithFactory):
attributes,
)
+ def test_malformed_github_account_username(self):
+ # Username must be a string
+ user = self.factory.makePerson()
+ attributes = {}
+ attributes["username"] = "@username!(*)"
+ utility = getUtility(ISocialAccountSet)
+
+ self.assertRaises(
+ SocialAccountIdentityError,
+ utility.new,
+ user,
+ SocialPlatformType.GITHUB,
+ attributes,
+ )
+
def test_malformed_matrix_account_homeserver(self):
# Homeserver must be a valid address
user = self.factory.makePerson()
@@ -205,6 +236,21 @@ class TestSocialAccount(TestCaseWithFactory):
attributes,
)
+ def test_empty_fields_github_account(self):
+ # Identity field must be not empty
+ user = self.factory.makePerson()
+ attributes = {}
+ attributes["username"] = ""
+ utility = getUtility(ISocialAccountSet)
+
+ self.assertRaises(
+ SocialAccountIdentityError,
+ utility.new,
+ user,
+ SocialPlatformType.GITHUB,
+ attributes,
+ )
+
def test_multiple_social_accounts(self):
# Users can have multiple social accounts
user = self.factory.makePerson()
@@ -250,16 +296,14 @@ class TestSocialAccount(TestCaseWithFactory):
user_two = self.factory.makePerson()
attributes = {}
- attributes["homeserver"] = "ghi.org"
attributes["username"] = "test-nickname"
getUtility(ISocialAccountSet).new(
- user_two, SocialPlatformType.MATRIX, attributes
+ user_two, SocialPlatformType.GITHUB, attributes
)
attributes = {}
- attributes["homeserver"] = "lmn.org"
- attributes["username"] = "test-nickname"
+ attributes["username"] = "test-nickname2"
getUtility(ISocialAccountSet).new(
- user_two, SocialPlatformType.MATRIX, attributes
+ user_two, SocialPlatformType.GITHUB, attributes
)
self.assertEqual(len(user.social_accounts), 2)
@@ -275,11 +319,9 @@ class TestSocialAccount(TestCaseWithFactory):
self.assertEqual(len(user_two.social_accounts), 2)
social_account = user_two.social_accounts[0]
- self.assertEqual(social_account.platform, SocialPlatformType.MATRIX)
- self.assertEqual(social_account.identity["homeserver"], "ghi.org")
+ self.assertEqual(social_account.platform, SocialPlatformType.GITHUB)
self.assertEqual(social_account.identity["username"], "test-nickname")
social_account = user_two.social_accounts[1]
- self.assertEqual(social_account.platform, SocialPlatformType.MATRIX)
- self.assertEqual(social_account.identity["homeserver"], "lmn.org")
- self.assertEqual(social_account.identity["username"], "test-nickname")
+ self.assertEqual(social_account.platform, SocialPlatformType.GITHUB)
+ self.assertEqual(social_account.identity["username"], "test-nickname2")
References
