Code review comment for lp:~wallyworld/lazr.restful/propogate-notifications

Revision history for this message
Ian Booth (wallyworld) wrote :

>
>> Since the notification text values may be escaped html and totally
>> free form text, it's not possible I think to do what is suggested
>> above. The only way I can see at this point is to encode the
>> notification text values as a json string and send as one header
>> value.
>
> Do we really want to allow arbitrary HTML? For example, what if I have
> a GUI client and want to display these messages? I would have to use an
> HTML renderer to be able to show them sanely. We could allow a small
> subset of HTML like just anchor tags, but even that places a pretty big
> burden on non-browser clients.
>
> I propose that we start with a type (DEBUG, WARNING, etc.) and a
> plain-text string. That may mean that we have to audit/rework the
> existing notifications to make them compatible.

Trouble is, all the launchpad usages of notifications - well very many -
stuff html in there using structured(). The assumption on launchpad's
part is that these notifications are being displayed on an html client.
The lazr restful stuff is making no assumptions about the content of the
notifications. It just runs the content through json and sends it along.
It's up to client and server to ensure that stuff sent is acceptable. In
launchpad's case, client.js grabs the html and displays it. If launchpad
currently assumes it's to send notifications as html, that's ok. It can
be changed later and lazr restful will continue to function unchanged.
Non browser clients won't be supported right off the bat but they will
just ignore the notification header value anyway. The initial goal is to
fix the issue in launchpad where inline editing of stuff works different
to non ajax editing in that it doesn't display the same messages to the
user.

« Back to merge proposal