Nodebb: There's apparently zero QA or code review on this project

[blakeyrat-dailywtf]

Our forum at TheDailyWTF was recently updated with this commit:

https://github.com/NodeBB/NodeBB/commit/eb0aea6390cd38c3802d070262068c676d796aaf

Which not only broke notifications, but also deleted all users' notifications previous to the update. Call me crazy, but I think deleting a user's data with no warning or possibility for undo is a _pretty big deal._

Out of curiosity I went to look for the PR this commit was a part of, to find out how well-reviewed the code was, only to find that there was no code review done. (I already knew there was no QA process, from previous updates of similar quality to our forum.)

I'm not asking for perfection, but maybe-- just maybe-- _one_ person could look at the commit before it's merged and permanently deletes user data.

[julianlam]

Hey there,

A couple thoughts... if this commit removed existing notifications, that's unfortunate, but you do have to realise that notifications are automatically pruned after a set number of days anyhow.

I'm not justifying it, but simply saying it isn't nearly as catastrophic as it sounds :smile:

Secondly, this was a commit made very recently, and so it is not included in any released version of NodeBB. Tracking the latest changes is a great way to get the latest fixes and features, but does also mean things like this happen.

That's why our community reporting these bugs and regressions to us is very important.

[blakeyrat-dailywtf]

Deleting user data is more an issue of trust and respect. When NodeBB does something like this, a little bit of that trust is tossed in the garbage, and believe me it's really hard to earn that trust back. It's not merely "unfortunate", it's critical. Software needs to be rock-solid to be liked and trusted by its users. Right now NodeBB is at best quicksand, and it doesn't appear (to us at our particular forum at least) to be solidifying anytime soon.

But you don't consider it that serious; fine. I vehemently disagree, but fine.

That doesn't explain why you're not doing code reviews for every change, nor does it explain why this "loose" commit (outside of a pull request) wasn't tested before being pushed to the master branch. And I don't mean "we did a 5-day-long regression" tested, I mean "I didn't even click the notification _once ever_" tested.

(Also I would also add that your development process seems to be kind of a conundrum: you only know about regressions if your community reports them; your community should only be using released versions. Catch-22.)

If the real problem here is that @BenLubar is updating our forum before it should be, then please have a chat with him. Because he's dragging _your_ reputation through the dirt.

[jarey]

To be honest i'm not a fan of the way the QA is managed at the nodebb project, but that said, you can't blame the project.

You updated your production instance, with a non-stable version, and apparently without testing that changes on a Test environment on your side, with your data backep-up etc. I don't know if you manage backups of the database either, but it is also a good practice to have on production, and a solution to the problem you are referring here, would have be to simply get back to the previous version of nodebb you had, with your data back up.

I think you can't blame the QA on the NodeBB project, when you don't seem to have any decent QA on your project/site.

Just my opinion. Regards.

[blakeyrat-dailywtf]

I 100% agree that our site needs a staging server and a lot more thought put into updates; I've brought this up multiple times and have been ignored. Users of the forum are the victims here, regardless.

But again, let me be clear: we're talking about an update to the notifications system where the developer who pushed-up the commit _didn't once click an notification before doing so_. This bug is impossible to miss with even the most cursory testing. As far as I can tell, there is literally _no_ QA.

(And, BTW, I should add that our forum is far too active to simply revert the database after a problem like this is noticed-- that would also result in loss of user data, the posts made inbetween the update being pushed and the problem being noticed.)

[julianlam]

Actually, I'd like to point out that the database schema didn't change, so resetting the repository to the parent commit would have been just fine.

[blakeyrat-dailywtf]

If the database schema was not changed, why was it necessary to delete older notifications?

[julianlam]

They are not deleted, just filtered out since the method to parse them no longer exists.

[blakeyrat-dailywtf]

So the answer is: it was not necessary; it was done anyway.

[thecartman82]

Julian: Of course we have a QA team. Here it is.

(brings out a mirror)

Blakeyrat: Wait... this seems familiar...
Oh... Oh no! It's... us! IT WAS US ALL ALONG!
NOOOOOOOOOOOO!

Julian: Mwhahahahaha!

[julianlam]

In any case, the side effects from the bug here have been resolved.

[blakeyrat-dailywtf]

So a process change has been introduced that will prevent code that wasn't reviewed or QAed from being introduced in the future?

What's the basis for closing this bug?