Skip to content
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

Templates not removed from DB after deleting Module #97

Closed
Timo-Breumelhof opened this issue Oct 21, 2021 · 8 comments · Fixed by #217
Closed

Templates not removed from DB after deleting Module #97

Timo-Breumelhof opened this issue Oct 21, 2021 · 8 comments · Fixed by #217
Assignees
Labels
bug Something isn't working
Milestone

Comments

@Timo-Breumelhof
Copy link
Contributor

Describe the bug

When you place Active Forums on a page, the templates are copied from \DesktopModules\ActiveForums\config\templates to the database.
But when you delete the module form a page, the Templates remain in the database while they should be removed IMO.

Software Versions

  • DNN: 09.09.01
  • Module: 06.06.00

To Reproduce

Steps to reproduce the behavior:

  1. Place AF on a apge
  2. Initialize the Module
  3. Delete the module from the page
  4. Empty Recycle Bin

Expected behavior

There should not be any template entries for this ModuleId in the activeforums_Templates table

@Timo-Breumelhof Timo-Breumelhof added the bug Something isn't working label Oct 21, 2021
@WillStrohl
Copy link
Member

What if you delete and then restore the module? Those templates need to be there still. Maybe a cascade on the templates table could be added instead?

Is there the possibility of multiple forum instances using the same template?

@WillStrohl WillStrohl added the requires more information More information is required to duplicate or understand the issue. label Oct 21, 2021
@Timo-Breumelhof
Copy link
Contributor Author

This was after I emptied the Recycle Bin. And they are related to a moduleId, so no way these are going to be used ever again IMO

@WillStrohl
Copy link
Member

No, you're correct if this was emptied from the recycle bin. 👍🏽

@Timo-Breumelhof Timo-Breumelhof removed the requires more information More information is required to duplicate or understand the issue. label Dec 2, 2021
@johnhenley johnhenley self-assigned this Nov 15, 2022
@johnhenley
Copy link
Collaborator

I researched and tested this and found that this doesn't just affect templates--no content from any forum-related tables is deleted when a module instance is deleted and recycle bin is emptied. For other modules (like HTML/Text) this is implemented by adding reference and a cascading delete to the Modules table. For forums we would have to do something similar. Since ModuleId is not in every table, we'd either have to 1) put it in a reference where possible which might lead to orphans, 2) add ModuleId to all tables where it doesn't yet exist, and then reference back to Modules, or 3) add the reference from activeforums_forums and then put in a delete trigger that deletes from other tables when a forum is deleted.

Regardless, need some serious digging into table dependencies beforehand.

Thoughts?

@WillStrohl
Copy link
Member

Yeah, it's probably just missing cascades that should be in place.

@johnhenley
Copy link
Collaborator

I have been doing some work on getting the relationships updated and the cascading deletes in-place so that deleting the module instance properly deletes all data contained within the forum tables related to the module instance. (as an aside, why does DNN rely on the cascading delete from the module table anyway? Why not do like IUpgradeable, and have an IDeletingInstance interface?)

Here is how things looked before I started.
image

Here is how things now look:
image

There are some tables (see diagram marker A) that aren't good candidates for the foreign keys to anything because they currently don't have either ModuleId or ForumId or anything in their definition to use for a cascading delete. Some contain transient data (like activeforums_SearchCache and activeforums_Queue) and generally are empty, so removing a module instance doesn't really have any impact. Others (like activeforums UserProfiles) have ModuleId but it is -1 because the user profile rows are used across module instances. I'm fine with leaving these two alone. Some like activeforums_Security probably could have ModuleId added, since they are module-specific and deleting the module instance does leave orphaned records. Let me know what you think about that. Not a high priority.

The ones that concerns me, however, are B & C, which are the relationships between activeforums_ForumTopics/activeforums_Topics/activeforums_Content. The relationships between them are actually kinda backwards, so the cascading delete can't be implemented. Possible solutions are:

  1. Fix the relationship and restructure/renormalize the tables a little bit; this is optimally the best solution but riskiest since it does mean updating existing code and stored procedures.
  2. Implement a DELETE trigger in activeforums_Forums that knows about those three tables and handles the deletes. Not a huge fan of triggers, but this solves the issue but it's not great.
  3. Do nothing, which leaves data in these three tables (activeforums_ForumTopics/activeforums_Topics/activeforums_Content ) and their children tables (activeforums_Topics_Tracking, etc. etc.) when the module instance is deleted.

Lastly, want to note that for the sake of simplicity, where replacing an existing constraint (without CASCADE DELETE) with a new one (with CASCADE DELETE) I'm updating with a simpler name.
For instance,
FK_activeforums_ForumTopics_activeforums_Forums
is now
FK_activeforums_ForumTopics_Forums

@johnhenley
Copy link
Collaborator

For this issue, I've added ModuleId to activeforums_Content, and scripted an UPDATE to populate it.
And have updated the relationships and I'm much more comfortable with this:
image]

@WillStrohl
Copy link
Member

Wow! This is amazing work, John. We're going to be in such a better place soon! :) Thank you so much for your hard work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants