launchpad-reviewers team mailing list archive

[Guruprasad <mp+458537@xxxxxxxxxxxxxxxxxx>]

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.