185(possible) & 190

[StephenMurphy]

I was unable to reproduce 185, but any broadcast has to pass through here and wont yet be published so should be caught. Working to show exactly how this is getting called improperly, but you can, at least, stop the publicly visible symptoms for the time being by catching unpublished posts trying to broadcast and putting them back into the queue or, in the case of 190, delete them.

[ssmathias]

Just one question so far on this pull request. What if the user removes a trashed post from the trash? How do you think it should be handled?

[StephenMurphy]

As the change stands, the broadcast post is deleted entirely, so the user must choose to broadcast again which, to me, makes the most sense; treat it as though they've written a new post. This keeps it consistent regardless of whether they've restored the post before or after the original post time.

As commented, there is the additional option of removing line 635 so that the broadcast post persists in the aggregate queue and is restored with the post -- as long as the user restores it before the post was originally scheduled to be displayed. If, however, they restore it too late, the broadcast is treated like a new post (from user's perspective) -- that is, they'd have to edit broadcast settings and fill out the form as if they've just made a new post. The original broadcast post hangs around until the next 15 minute interval when the queue decrements and it's off the end where it's left as garbage until its queue position is filled, near as I can tell. This is not, to me, ideal -- the user has an inconsistent experience, and we've made a mess. I'd keep line 635.

[StephenMurphy]

190 has been solved. Throwing away a post was handled oddly resulting in the broadcast being sent out. As per my recommendation above I lumped it in with 'private' posts.

[ssmathias]

Your reasoning appears logical to me. Code is relatively clean. I'd give this a thumbs-up at least as a stopgap measure for these issues.

[StephenMurphy]

I'd absolutely agree modifying the broadcast file is a stopgap for #185 but I've since updated to reflect the actual cause of #190 and that should be fixed unless I'm much mistaken.

[ssmathias]

Just a couple pieces of feedback after doing a bit of internal review (this is based on our code standards):

Change "and" to "&&" in conditionals Change "or" to "||" in conditionals Change "else if" to "elseif" (one word syntax is supported in this style of block, and required in colon-style blocks which we use in templates) Change the comments to be less personal, and express how the code changes resolve the issue. No need to note yourself, as it will come through on the commit history.

[StephenMurphy]

No problem. Would you like me to push those changes through?

[ssmathias]

Absolutely. Thank you.

[StephenMurphy]

I assume also change or -> ||?

[ssmathias]

Correct.

[ssmathias]

Looking much better. Thank you.

[mospaw]

These changes were manually merged into a working feature branch.