Old user suspension reasons have gone missing

bug
21

Yeah, just 3 users, so no issues with the quick fix. Still wondering what you are referring to as a “migration artifact”?

22

Yeah if you have lots of these permanently suspended users there is no quick fix. But if it is a few, should not be a problem to just unsuspend, re-suspend.

A migration artifact is a mistake in upgrading the database between Discourse versions.

Anyway now that @techapj has that information, perhaps he can dig a bit in the raw database to see if the data is still there on older instances where users were suspended. BBS is a good example and definitely exhibits this problem as well, all the old suspension reasons are gone.

5 Likes
24

It may take a few days or a week, but we will look into it and see what we can do.

1 Like
25

This commit by @techAPJ broke it:

https://github.com/discourse/discourse/commit/4c967d11b46133861b8c9495eb65f727425f7dd5#diff-0a5ad2d838fbffeb0848dda1be5dbd12

     @actions ||= Enum.new(:delete_user,
                           :change_trust_level,
                            :change_site_setting,
                            :change_site_customization,
                            :delete_site_customization,
 +                          :change_site_text,  <<<====
                            :checked_for_custom_avatar, # not used anymore
                            :notified_about_avatar,
                            :notified_about_sequential_replies,
 @@ -33,26 +34,27 @@
                           :auto_trust_level_change,
                           :check_email,
                           :delete_post,
                           :delete_topic,
                           :impersonate,
                           :roll_up,
                           :change_username,
                           :custom,
                           :custom_staff,
                           :anonymize_user,
                           :reviewed_post,
                           :change_category_settings,
                           :delete_category,
                           :create_category)

Our enums go from 1 upwards, if you insert anything into the list like that (as opposed to appending at the end) you break everything.

I fixed the issue with

https://github.com/discourse/discourse/commit/3b45cd0fcb4d48f3166e05e3501cbbcb5805d59f

But anyone that updated in the last 21 days is going to have out-of-whack actions in user history, it is pretty much impossible to fix the broken window, cause I would have to join against a table that has version history.

9 Likes
26

Can you add comments in the code indicating the danger in inserting here? Feels like there should be a big WARNING there so this mistake does not happen again in the future.

5 Likes
27

Thanks for hunting this down @sam. Will this fix restore the missing suspension reasons, or just prevent this from occurring again in the future?

28

As stated above, any log table data changed (or created) in the last 20 days is broken forever because the enums were shifted down by the insertion. Data that was not changed in the last 20 days will be unaffected.

29

The fix will restore all the old ones, but the new suspensions that happened ever since the bug was introduced are impossible to recover. (21 day window)

3 Likes
30

Is it possible to provide a fixing query… something like “update WHATEVER from (select WHATEVER from history table) … match on some ID”?

31

Thanks for clearing that up. We have no suspensions in the last 21 days, so all is good! Much appreciated :smile:.

32

No it is not possible, we do not store the (git version + date it started running) in a table, if we had that information we could use a query to fix.

We will correct the underlying problem asap

https://meta.discourse.org/t/enums-that-are-used-in-tables-need-to-be-stable/37622

33

I’ve updated and can confirm that:

Thanks for the fix!

Is there any weirdness we should expect or need to keep an eye out for?

You know - because of the “broken window”:

960×720 115 KB

2 Likes
34

Give me a bit, coming up with a migration that will fix the majority of the pain.

4 Likes
35

This will fix the vast majority of problems this generated

https://github.com/discourse/discourse/commit/956e3ad2088a9ebe09338b94689f91d5975753e9

It looks for “auto change trust level” something that happens quite a lot being mislabeled as “check email” and for “impersonate” that was mislabeled as “check email”. Once it finds the bounds it takes one off the action number.

It logs what changes it makes in case we have to revert them. We can add a few more conditions but it should vastly improve the data quality.

6 Likes
36

Would it be a good idea to drop a new beta with this in, as the bug seems to be present in the current beta7? (I’m thinking as people are still installing and upgrading to beta7 out there.)

4 Likes
37

Agree @neil can you do a new release tomorrow?

38

I apologize for introducing this bug. It was a stupid mistake. :disappointed:

As per @sam’s recommendation I have created a PR to change Enums to also accept a hash, and used it almost everywhere where stability is required along with test cases:

https://github.com/discourse/discourse/pull/3950

More details here:

https://meta.discourse.org/t/enums-that-are-used-in-tables-need-to-be-stable/37622

8 Likes
39

Just wanted to say thanks again…

… and after updating I can confirm that both the sample suspended users have correct reasons marked against them now - yay!

5 Likes