-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Wiki deletion Reason DB table #777
Conversation
459e099
to
cc20fee
Compare
f38bd95
to
847fb20
Compare
Considering we never really delete wikis and just use soft deletion on them, have you considered making the deletion reason a property on the In case you have considered this, what were the reasons you went for a dedicated table? To me it currently seems a bit heavy to create a new table to store a single line about a single entity. |
Isn´t the soft delete a temporary decision? |
I wouldn't think so. Not sure if we ever check this, but we ought to check the domain of newly created wikis against all wikis, including deleted one in order to prevent Namesquatting. Even if we ever decide to delete resources like ES indices at some point, we will likely always ever keep the Wiki models around. |
Yeah, it seems we currently also check against deleted wikis when validating domains here: api/app/Helper/DomainValidator.php Line 35 in cc274b9
|
that makes sense. I spoke with Tom few days ago told him my plan and he sounded agreeing with the idea of a new table. I am now only wondering what were Tom's reasons. https://mattermost.wikimedia.de/swe/pl/5417g1esotdp3rq9ftm5ygndha |
I can hardly tell, and it's probably also a bit subjective, but I have to admit that I feel having single and primitive one-to-one properties in an external table is a bit of an antipattern for me. We don't have a If we were to create a new table, we'd benefit from making it more flexible so that it can store more than just one thing (kind of like we do with |
We definitely did talk about it and I can't remember what my own reasoning was; I don't think it was a strongly held belief though; you probably did a good job of convincing me; putting it in the If you were to do either of these things then you'd add another field there with the deletion reason. I think it was aligned to what you also mentioned (i.e. would it be inconvenient to access these soft-deleted Wiki objects) but I can't remember my own arguments :D. I think I might also not feel that adding tables is that "expensive" (or "heavy-weight") as Frederik. I think there is some argument that we don't want to just stick everything in the wiki model but where to draw that line is probably subjective. In any case I don't think I'd be "blocking" or standing in the way of any of these solutions :) |
I might be getting myself in a muddle but I think we do. And I think the original intention was so that these Domain objects would persist even after deleting the Wiki objects (see: 06fb4bb). I don't think this completely invalidates the idea that this information could also go in the |
TIL. This was so obvious to me, I didn't even check. Then why does the I'd argue we shall never hard delete Wiki models because we need to keep a record of what was there once. The Wiki model is a good place for that. |
I wouldn't think of it as expensive, but as clutter and confusing. As I said, adding extra tables for a one-to-one primitive value is something I'd definitely put in my "Database Antipatterns" blog post I'll never write. |
I do like the idea of using the |
Legacy, it's supposed to disappear one day I guess.
I don't disagree but it's also slightly less clear cut to me. It's the trade off between fragmentation and single responsibility. To play devil's advocate I could imagine we allow "un-deleting" and then we would lose this 1:1 mapping.
Please write it! FWIW I feel like my DB skills are super weak and I may have poor judgement in this area |
At least for me,
We could get out of this with a migration at that point. In any case I feel this discussion - while interesting - is not helping in moving this PR forward. I have a strong favor for sticking the deletion reason on the wiki model itself for the reasons I stated multiple times, but I realise it's subjective. That being said, let's move on in whatever way @rosalieper seems fit I guess? |
I will move ahead with the idea of adding the deletion reason on the wiki class and therefore close this PR. |
Bug: T347865