Skip to content

Add command line interface #7

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

james-cnz
Copy link
Contributor

This pull request removes safety checking on the purge method, on the assumption that this checking will be performed in the user interface, and adds a basic CLI that allows directly calling the method. #5 is needed first, to prevent possible data loss that may result from removing the safety checking.

// Safety check.
return;
}
if (!in_array($component, local_purgeoldassignments_components())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey James - I'm not a big fan of the removal of this check - it feels too easy for us to be able to trigger a deletion (even from the front end) of non-assignment files with this change. What's the use-case for allowing users to delete other files in the site with this script?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @danmarsden,
Sorry, I've apparently misunderstood something you said.
In Catalyst WR #459151 you wrote "feel free to add a PR with a CLI script that allows an admin - who hopefully knows what they are doing! :-) to run a CLI clean up for any time frame on the fileareas/components/course required."
I took that to mean remove the safety checks, and allow the admin to do anything.
I don't actually have a use case for this.

@danmarsden
Copy link
Member

Cool, let's leave that check in for now (only supporting assignment fileareas) - I think removing the check there still opens up a POST in the ui passing a non supported component and could allow someone to delete stuff they should not be deleting in this manner

@james-cnz
Copy link
Contributor Author

OK, that check's back in.
The minimum age check is still removed, though, and I think it is important that #5 is done first. I think the user interface currently can unintentionally request deletion of files with a minimum age of 0 years, so applying this change first, which removes that check, could lead to real data loss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants