Code review comment for lp:~wallyworld/launchpad/delete-bugtask-ui-878909

Hi

Thanks for the excellent review.

>
> This is dangerous. Both title and displayname may *always* contain
> markup. We use structured() to build safe interpolated messages.
>

I originally tried using structured() but the legit html in the message
got escaped. I suspect I may have messed up using the api and had meant
to come back to look at it again but forgot. I'll take another look.

> Would direct language be more effective at conveying the consequences of the
> action:
> Deletion is permanent, it cannot be undone.
>

Sure.

>> + user_can_delete=self.user_can_delete_bugtask,
>> + delete_link=link + '/+delete',
>
> While I am certain this link will work, but it is a crafted link we need to
> maintain with extra testing.
> canonical_url(self.context.target, view_name='+delete')
> will raise an exception if the named view is unregistered.
>

Yes, much better, thanks.

>>
>> +class TestBugTaskDeleteLinks(TestCaseWithFactory):
>> + """ Test that the delete icons/links for each relevant bug task are
>> + correctly rendered.
>> +
>> + Bug task deletion is protected by a feature flag.
>> + """
>
> The first line may not wrap according to PEP 256.
>

Yeah, sometimes it is hard to limit the text to only one line and still
have it make sense. I'll truncate.

>
> The TestBrowser is an indirect means to test rendering. The view does that.
> you can render any view (that had the principal argument passed) by calling
> it; view(). Launchpad views add striping rules so __call__ calls render().
> You can render a view's templates using view() or view.render(). It is much
> faster than TestBrowser.
>

I only resorted to using TestBrowser because I could get view.render()
to work for this case. It failed with various errors (eg None broken the
chain etc) depending on what I tried. I see you suggest:

 > view = create_initialized_view(
 > bug, name='+bugtasks-and-nominations-table'
 > principal=bugtask.owner)

I tried things like eg:

  view = create_initialized_view(bugtask, name='+index',
     rootsite='bugs', principal=bugtask.owner)

  view = create_initialized_view(bug, name='+index',
     rootsite='bugs', principal=bugtask.owner)

I'd like to understand why view.render() failed.

>
> And I see you did you view.render() here. I do not care for exact testing
> of user text because it tends to change often and get tested more than once.
> I put ids on the text fragment, id="deletion-waring-message", and just test
> for the presence. This permits non technical users to make trivial changes
> to the text while the code ensures that the intent of the message is still
> in the content.
>

Yes, good idea. I had doubts how far we needed to go here, hence the use
of the regex to test the gist of the message.

>
> This code is repeating the mistakes of Bugs past. The code embeds roles
> and celebrities into model code. This is the primary reason bug permissions
> are fucked up. I believe this logic belongs in lp.bugs.security, where I
> think this came from. userCanDelete() could do this:
> def userCanDelete(self):
> return check_permission('launchpad.Delete', self)
>
> The user is gotten from the interaction.
>

Ah, of course. I actually had the code in the security adaptor in the
first place and only moved it because I didn't recognise the ability to
use check_permission to access it for this use case.

>> +
>> +<div metal:fill-slot="main">
>> +<div metal:use-macro="context/@@launchpad_form/form">
>> +<div metal:fill-slot="extra_info">
>> +<p tal:content="structure view/confirmation_message">
>> + Are you sure?
>> +</p>

> Why did you choose to create HTML in BugTaskDeletionView.confirmation_message
> rather than using it in the template where escaping is easier to manage?
>

The idea was that the message is content provided by the model to be
rendered. It's the view's job simply to splat that message onto the
screen. Like how request notifications are done. I'll look to rework it.

> <a tal:condition="data/user_can_delete"
> tal:attributes="
> id string:bugtask-delete-${data/form_row_id};
> href data/delete_link"
> class="sprite remove bugtask-delete" style="margin-left: 4px"></a>
>
> I believe the style attr is unneeded. We can update style-3.0.css with a rule
> like:
> .bugtask-delete {
> margin-left: 4px;
> }
> .

I was torn here. I thought it better to add a little bit of one off, use
case specific styling to the tales rather than "pollute" the css with
yet another rule which is only used in one place.