Left a few minor comments/questions/suggestions. Will approve once they have been addressed. Nice work!
Diff comments: > diff --git a/lib/lp/registry/browser/person.py > b/lib/lp/registry/browser/person.py > index 8f044fd..139b687 100644 > --- a/lib/lp/registry/browser/person.py > +++ b/lib/lp/registry/browser/person.py > @@ -889,6 +894,12 @@ class PersonOverviewMenu( > return Link(target, text, icon="edit", summary=text) > > @enabled_with_permission("launchpad.Edit") > + def editmatrixaccounts(self): > + target = "+editmatrixaccounts" > + text = "Update Matrix accounts" Wouldn't it be better to call this "Edit Matrix accounts" since the URL segment and the current function all use "Edit" instead of "Update"? I understand that the similar function for the corresponding Jabber ID link uses "Update" as well, but if we want consistency, we can fix that too, right? :) > + return Link(target, text, icon="edit", summary=text) > + > + @enabled_with_permission("launchpad.Edit") > def editjabberids(self): > target = "+editjabberids" > text = "Update Jabber IDs" > @@ -2417,6 +2425,84 @@ class PersonEditIRCNicknamesView(LaunchpadFormView): > self.request.response.addErrorNotification( > "Neither Nickname nor Network can be empty." > ) > + return > + > + # If we there were no errors, return user to profile page > + self.next_url = canonical_url(self.context) > + > + > +class PersonEditMatrixAccountsView(LaunchpadFormView): > + # TODO: have a look into generalising this view and the relevant template > + # (`person-editmatrixaccounts.pt`) for any social platform > + > + schema = Interface > + platform = MatrixPlatform > + > + @property > + def page_title(self): > + return smartquote( > + f"{self.context.displayname}'s {self.platform.title} accounts" > + ) > + > + label = page_title > + next_url = None > + > + @property > + def cancel_url(self): > + return canonical_url(self.context) > + > + @property > + def existing_accounts(self): > + return self.context.getSocialAccounts( > + platform=self.platform.platform_type > + ) > + > + @action(_("Save Changes"), name="save") > + def save(self, action, data): > + """Process the social accounts form.""" > + form = self.request.form > + > + # Update or remove existing accounts > + for social_account in self.existing_accounts: > + if form.get(f"remove_{social_account.id}"): > + social_account.destroySelf() > + > + else: > + updated_identity = { > + field: form.get(f"{field}_{social_account.id}") > + for field in self.platform.identity_fields > + } > + if not all(updated_identity.values()): > + self.request.response.addErrorNotification( > + "Fields cannot be left empty." > + ) > + return > + > + social_account.identity = updated_identity > + > + # Add new account > + new_account_identity = { > + field_name: form.get(f"new_{field_name}") > + for field_name in self.platform.identity_fields > + } > + > + if all(new_account_identity.values()): > + getUtility(ISocialAccountSet).new( > + person=self.context, > + platform=self.platform.platform_type, > + identity=new_account_identity, > + ) This may have to be wrapped in a try-except block depending on the behaviour of validation done in the lower layers via Simone's MP. > + > + elif any(new_account_identity.values()): > + for field_key, field_value in new_account_identity.items(): > + self.__setattr__(f"new_{field_key}", field_value) > + self.request.response.addErrorNotification( > + "All fields are required to add a new account." > + ) > + return > + > + # If we there were no errors, return user to profile page > + self.next_url = canonical_url(self.context) It will be nice to present a notification to the user that the form submission succeeded. But since the form could have contained a mix of new social accounts, updates to existing ones, and deletion of existing accounts, I am not sure what an appropriate notification message should say. > > > class PersonEditJabberIDsView(LaunchpadFormView): > diff --git a/lib/lp/registry/templates/person-editmatrixaccounts.pt > b/lib/lp/registry/templates/person-editmatrixaccounts.pt > new file mode 100644 > index 0000000..38ab8ed > --- /dev/null > +++ b/lib/lp/registry/templates/person-editmatrixaccounts.pt > @@ -0,0 +1,72 @@ > +<html > + xmlns="http://www.w3.org/1999/xhtml" > + xmlns:tal="http://xml.zope.org/namespaces/tal" > + xmlns:metal="http://xml.zope.org/namespaces/metal" > + xmlns:i18n="http://xml.zope.org/namespaces/i18n" > + metal:use-macro="view/macro:page/main_only" > + i18n:domain="launchpad" > +> > +<body> > + > +<div metal:fill-slot="main"> > +<div metal:use-macro="context/@@launchpad_form/form"> > + <div metal:fill-slot="widgets"> > + > + <p tal:condition="view/error_message" > + tal:content="structure view/error_message/escapedtext" class="error > message" /> > + > + <table> > + > + <tr> > + <td><label>Homeserver</label></td> > + <td><label>Username</label></td> > + </tr> > + > + <tr tal:repeat="social_account view/existing_accounts"> > + <td> > + <input tal:attributes="name string:homeserver_${social_account/id}; > + value social_account/identity/homeserver" Mismatched indent? > + type="text" style="margin-bottom: 0.5em;"/> Is there a reason for specifying an inline style for this element instead of using the stylesheets? > + </td> > + <td> > + <input type="text" > + tal:attributes="name string:username_${social_account/id}; > + value social_account/identity/username" /> > + </td> > + > + <td> > + <label> > + <input type="checkbox" > + value="Remove" > + tal:attributes="name string:remove_${social_account/id}" > /> > + Remove > + </label> > + </td> > + </tr> > + > + <tr> > + <td> > + <input name="new_homeserver" > + type="text" > + placeholder="Enter new homeserver" > + tal:attributes="value view/new_homeserver|nothing" /> > + </td> > + <td> > + <input name="new_username" > + type="text" > + placeholder="Enter new username" > + tal:attributes="value view/new_username|nothing" /> > + </td> > + </tr> > + > + <tr> > + <td class="formHelp">Example: ubuntu.com</td> > + <td class="formHelp">Example: mark</td> > + </tr> > + </table> > + </div> > +</div> > +</div> > + > +</body> > +</html> -- https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/458537 Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:social-accounts-edit-view into launchpad:master. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp