Files within a group aren't deleted after deleting the group

Bug #1354222 reported by Moises Burgos
20
This bug affects 4 people
Affects Status Importance Assigned to Milestone
Mahara
Fix Released
Medium
Unassigned
1.10
Won't Fix
Medium
Unassigned
1.9
Won't Fix
Medium
Unassigned
15.04
Fix Released
Medium
Unassigned
15.10
Fix Released
Medium
Unassigned
16.04
Fix Released
Medium
Unassigned
16.10
Fix Released
Medium
Unassigned

Bug Description

If you create a group that contains any files (e.g. image, file) and then delete that group you should see a warning message. In addition, the files are not deleted although the group is (or marked as deleted)

To reproduce (Mahara master):

1.- Click on the 'Group' tab and create a group
2.- Click on the 'Files' tab and add a file or image
3.- Delete the group and you should see the warning. To check that the added files/images weren't deleted you can check out the 'artefact_file_files' table.

Is this a bug or is it an intended behaviour? If it is a bug I could provide a patch for review.

Cheers,
Moises

Revision history for this message
Robert Lyon (robertl-9) wrote :

Hi Moises,

One thing to test would be if a group has some files and the group creates a page and uses those files on the page (for example has a 'files to download' block) and then shares that page with users not in group and allows those users to copy page. Do those users need to have the group files to exist? or do they make copies of the files?

If they copy the files to their own content -> files area then it might be fine to delete the group files once the group is deleted.

Also something to check is if the group files are used in the group's forum discussions - do those files need to hang around after group is deleted?

I don't have the answer to these questions at the moment so some investigation will need to be done

Robert

Revision history for this message
Moises Burgos (moises-burgos) wrote :

Hi Robert,

Thanks for replying.

I hadn't thought about those cases you mentioned. So, I'll look further into this and get back to you with any news.

Moises

Revision history for this message
Christian Stickel (christian-stickel) wrote :

Hi Robert,

this bug also works if you do nothing with the files:

1. create a group
2. upload a file to the group
3. find the file in /maharadata
4. delete the group

The file remains in the system even after some manual reloads of the cron. It appears in 'artefact_file_files'. Is this table meant to collect the garbage files?

best regards,
Christian

Changed in mahara:
status: New → Confirmed
Lars (lars-mogwitz)
information type: Public → Public Security
Lars (lars-mogwitz)
information type: Public Security → Public
Revision history for this message
Lars (lars-mogwitz) wrote :

I reproduced this using Christian's method.
These files seem to stay in the depths of maharadata forever. Cron doesnt clean anything.
We are planning on using mahara for all of our students meaning tons of GB added each semester.

I'd highly appreciate if the files were deleted, too when a group is deleted.

Kind regards
Lars

Revision history for this message
Aaron Wells (u-aaronw) wrote :

We should:

1. Fix the problem of the files not getting deleted.

2. Make a script that checks for orphaned files in the dataroot directory. We could schedule this as a cron task (that runs, like, once a month), or make it a script that admins have to run on demand.

Changed in mahara:
importance: Undecided → Medium
milestone: none → 15.04.0
tags: added: fileupload groups
Revision history for this message
Lars (lars-mogwitz) wrote :

Aaron.

1) We are working on this as a 2) part of the cronjob.

We decided not to assign the files uploaded in a group to a certain user, instead we will make the cron delete them, IF and only IF they aren't used in a view any longer.

The reason we don't want to assign the file to a user is that if there is more than one user who uses the group-file in their view and the 'owner' deletes it the other user will 'lose' the file in their view.

In the future we might consider assigning the file to a user if he is the only one using the file and the file added doesn't exceed the their quota.

I'll keep you updated

Revision history for this message
Kristina Hoeppner (kris-hoeppner) wrote :

Hi Lars,

Thank you for looking into fixing the issue. Is it OK with you if I assign you to it or would it be somebody else?

Thank you
Kristina

Revision history for this message
Christian Stickel (christian-stickel) wrote :

Lars,

I suggest you have a look at the group_delete() method to initially delete the groupfiles and at this point log the files which are still in use and also log the views where they are in use.

This lookuptable could then be used in a second step to implement a method for the cron, thus reducing the amount of queries needed.

Imho this can become a usability issue. It would be appropriate to inform the user at this point that he will be using orphaned files and that he will not be able to use the files when the view is deleted. Thus I suggest a dialog or a message that asks the user if he wants to copy these files to his own repository. If his quota is exceeded this will give the user the chance to download the files and reupload them again when needed.

cheers,
Christian

Revision history for this message
Lars (lars-mogwitz) wrote :

Kristina.
You are very welcome to assign me to this. I'd really love to contribute to Mahara.
Though I am a programmer for a decent amount of time now I was never working on an open source project.
Are there any guidelines on how to submit your changes/files and where can I find them?
Thanks in advance you for your help.

Christian.
I agree with you in deleting all files which aren't used in any view and are part of the deleted group.
As you suggested I'd put that into the delete_group() method.

Rather than creating a lookup table for the files, which are still in use I would add kind of the same mechanism to the cron script which searches for files in ALL deleted groups that aren't used in views no more.

I feel like the query I worked on today is fast enough to be executed every time the cron runs with no significant penalty for the cron script's execution time.

I would add the user-messaging when they have a file in one of their views which is part of a deleted group when everything is working fine.

Right now I am working on a test-environment with a huge database of 10.000.000 artefacts, 200,000 views and 200.000 groups to find out two things:
a) Is it necessary to implement this orphan-deleter as a part of the update process when updating to a new version of Mahara or can it be done through the cron
b) Is my query fast enough to be used in huge systems

I think I'll have an answer by the end of next week. Until then I'll play around with my test scenario's settings and let you know.

Kind regards
Lars

Revision history for this message
Aaron Wells (u-aaronw) wrote :

Hi Lars,

There's a lot of developer information up on our Wiki (though it's pretty disorganized) -- https://wiki.mahara.org/index.php/Developer_Area

We use a gerrit code review system (at https://reviews.mahara.org ) to put new code into the Mahara. This page describes how to use that: https://wiki.mahara.org/index.php/Developer_Area/Contributing_Code

And if you have any questions you can ask for help on the https://mahara.org developer forum, or in the #mahara-dev channel on irc.freenode.net.

Cheers,
Aaron

Lars (lars-mogwitz)
Changed in mahara:
assignee: nobody → Lars (lars-mogwitz)
status: Confirmed → In Progress
Revision history for this message
Kristina Hoeppner (kris-hoeppner) wrote :

Hi Lars,

Fantastic that you want to contribute. :-)

Just a few highlights from the developer documentation to get you started quickly:

1. https://wiki.mahara.org/index.php/Developer_Area/Developer_Environment - here you find how to set up a developer environment on your computer to check out the code and make changes.

2. Aaron already pointed to the section on contributing code through Gerrit. The part "Committing in some easy condensed steps" gives you the steps without all the explanations.

Look forward to seeing your code.

Cheers
Kristina

Robert Lyon (robertl-9)
Changed in mahara:
milestone: 15.04.0 → 15.04.1
Aaron Wells (u-aaronw)
Changed in mahara:
milestone: 15.04.1 → 15.10.0
Revision history for this message
Mahara Bot (dev-mahara) wrote : A patch has been submitted for review

Patch for "master" branch: https://reviews.mahara.org/6446

Revision history for this message
Robert Lyon (robertl-9) wrote :

On latest master codebase (pre 16.10) the issue with deletion of group files was due to artefact/file/lib.php not being called during the artefact delete_by_artefacttype() function during the deletion of a group.

I added safe_require('artefact', 'file'); to that function and for my tests the deletion of group files was done as expected

See: https://reviews.mahara.org/#/c/6446/1

Revision history for this message
Robert Lyon (robertl-9) wrote :

The part about needing a cron to clean up orphan files is still needed

Revision history for this message
Mahara Bot (dev-mahara) wrote : A change has been merged

Reviewed: https://reviews.mahara.org/6446
Committed: https://git.mahara.org/mahara/mahara/commit/6933370d76a63d677690607a3631aefdcfec9013
Submitter: Son Nguyen (<email address hidden>)
Branch: master

commit 6933370d76a63d677690607a3631aefdcfec9013
Author: Robert Lyon <email address hidden>
Date: Mon May 2 09:41:48 2016 +1200

Bug 1354222: Allowing the deletion of group files

Was failing due to not being able to fine artefact files library

behatnotneeded

Change-Id: Ie28d697ffc922d70d86457a8ca4a978dfddaec55
Signed-off-by: Robert Lyon <email address hidden>

Revision history for this message
Mahara Bot (dev-mahara) wrote : A patch has been submitted for review

Patch for "16.04_STABLE" branch: https://reviews.mahara.org/6475

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Patch for "15.10_STABLE" branch: https://reviews.mahara.org/6476

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Patch for "15.04_STABLE" branch: https://reviews.mahara.org/6477

Revision history for this message
Mahara Bot (dev-mahara) wrote : A change has been merged

Reviewed: https://reviews.mahara.org/6475
Committed: https://git.mahara.org/mahara/mahara/commit/18a9195145d0266e2905329819f5cea52fbc222e
Submitter: Son Nguyen (<email address hidden>)
Branch: 16.04_STABLE

commit 18a9195145d0266e2905329819f5cea52fbc222e
Author: Robert Lyon <email address hidden>
Date: Mon May 2 09:41:48 2016 +1200

Bug 1354222: Allowing the deletion of group files

Was failing due to not being able to fine artefact files library

behatnotneeded

Change-Id: Ie28d697ffc922d70d86457a8ca4a978dfddaec55
Signed-off-by: Robert Lyon <email address hidden>
(cherry picked from commit 6933370d76a63d677690607a3631aefdcfec9013)

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6476
Committed: https://git.mahara.org/mahara/mahara/commit/5b8cd0fba878b707387de70d994badef649976f6
Submitter: Robert Lyon (<email address hidden>)
Branch: 15.10_STABLE

commit 5b8cd0fba878b707387de70d994badef649976f6
Author: Robert Lyon <email address hidden>
Date: Mon May 2 09:41:48 2016 +1200

Bug 1354222: Allowing the deletion of group files

Was failing due to not being able to fine artefact files library

behatnotneeded

Change-Id: Ie28d697ffc922d70d86457a8ca4a978dfddaec55
Signed-off-by: Robert Lyon <email address hidden>
(cherry picked from commit 6933370d76a63d677690607a3631aefdcfec9013)

Revision history for this message
Mahara Bot (dev-mahara) wrote :

Reviewed: https://reviews.mahara.org/6477
Committed: https://git.mahara.org/mahara/mahara/commit/88456e494e01a2ebaa7121fcc6fa7ee883476190
Submitter: Robert Lyon (<email address hidden>)
Branch: 15.04_STABLE

commit 88456e494e01a2ebaa7121fcc6fa7ee883476190
Author: Robert Lyon <email address hidden>
Date: Mon May 2 09:41:48 2016 +1200

Bug 1354222: Allowing the deletion of group files

Was failing due to not being able to fine artefact files library

behatnotneeded

Change-Id: Ie28d697ffc922d70d86457a8ca4a978dfddaec55
Signed-off-by: Robert Lyon <email address hidden>
(cherry picked from commit 6933370d76a63d677690607a3631aefdcfec9013)

Robert Lyon (robertl-9)
Changed in mahara:
milestone: 16.04.1 → none
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.