Code review comment for lp:~jpds/launchpad/fix_450262

Revision history for this message
Michael Nelson (michael.nelson) wrote :

Hi Jonathan! Wow, you're really getting stuck into some great bugs :)

Your change here looks great... I've only got a few small thoughts.

I was about to mention that the field needs a description after looking at your screenshot, but on merging I see you've already done that! :)

I'm not an English expert, but "Do not send notification to anyone regarding this membership change." reads more clearly to me than "Do not send notification to anyone of this membership change."? (I was confused for half a second by of until I realised the context). But up to you, I think they're both fine.

The label 'Silent' on it's own could still be a bit more descriptive - I know the description is there, but the label itself should make sense on it's own too. Maybe 'Change silently'? That would still be suitably complemented by the description?

It is a shame that editing membership doesn't add a normal notification on the screen after submitting, something like "Foo Bar's membership has been updated successfully. No one was notified via email.", but that's not part of this branch. It was because of this that I was initially uncertain whether deactivation was also silent - I saw checking your diff that it is, but that then left me wondering why reactivation cannot be silent?

To reproduce:
1. Visit https://launchpad.dev/~ubuntu-team/+members
2. Silently deactivate a member.
3. Edit their membership again under Former members.
I can then reactivate their membership but without any option to do it silently?

Again, not a big issue at this point, but it's worth an XXX if you're not keen to tackle it now - unless it's an intentional decision?

So I'd be happy for you to land this as long as you've considering the above points.

review: Approve (ui)

« Back to merge proposal