Code review comment for lp:~widelands-dev/widelands-website/notifications_cleanup

SirVer (sirver) wrote :

Some comments while testing:

- On http://alpha.widelands.org/notification/, the maps heading says Wlmaps. That should probably just read Maps.
- I sent myself a PM and got an email immediately.
- I subscribed to the forum and created a new topic.
- When uploading a map I got a 504 gateway timeout the first time. When trying again I got an "Internal server error" after a while, but the map was apparently successful uploaded.

After activating the alpha repo and running /var/www/django_projects/alpha/wlwebsite/code/widelands/manage.py emit_notices I got the two emails I was expecting (map upload and new topic). So it works! Great :)

[pickling]
I think the warning is fine and can be ignored here. Pickling is a way to serialize (i.e. convert to a string representation) a living Python object so that it can be stored somewhere. We store the notices in some sort of on-disk queue for later sending as email (in emit_notices). The design seems interesting, I would expect to save the data into the database using djangos normal means, but apparently another approach was taken here.

The potential problem is that if we update django, the models living representation might change, since the code changed. Unpickling from this queue will then give half-correct representations that will cause problems. However, this is easily avoided by making sure that the emit queue is empty before we update anything. This should be the case after emit_notices runs successfully.

Code lgtm - 2 nits inlined.

review: Approve

« Back to merge proposal