Disable inactive club sites #68
Labels
No Label
priority
high
priority
low
priority
medium
priority
very high
BUG
Feature
High Priority
Low Priority
Medium Priority
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: public/pyceo#68
Loading…
Reference in New Issue
No description provided.
Delete Branch "disable-inactive-club-sites"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #51.
An API argument called
remove_inactive_club_reps
was added so that we can dynamically control whether we want to remove inactive club reps or not. The default action is only to disable club websites without changing group membership.@ -0,0 +15,4 @@
This must be called BEFORE any of the methods below, using a context
expression like so:
with club_site_mgr.open_config_dir():
Should this be
instead?
Yes.
@ -103,0 +106,4 @@
Retrieves all clubs.
"""
# couldn't import the Term class from ceo_common.model due to some
Is this duplicated code then? If so can we take out both references and stick it into a new file or something so we still import one thing?
It's not duplicated code - I just had to use
'Term'
instead ofTerm
in the type annotation (which is optional anyways).@ -0,0 +81,4 @@
logger.debug('No git folder found in Apache directory')
return
logger.debug('Committing changes to git repository')
self._run(['git', 'add', 'conf-available/disable-club.conf'], cwd=self.apache_dir)
Can we use constants (top of this file is fine) for all file paths? Seems like a pain to fix if we ever change it (i.e. if Apache updates something) if we hardcode everything.
We can create constants for
conf-available/disable-club.conf
andsnippets/disable-club.conf
, but all of the standard Apache config paths are hard-coded into Augeas, so they're not configurable.Sure, just those two then, we can leave Apache config paths.
@ -0,0 +98,4 @@
club_name = None
club_email = None
filename = os.path.basename(config_file_path)
directive_paths = self.aug.match(f'/files/etc/apache2/sites-available/{filename}/VirtualHost/directive')
Hardcode -> constant
See comment above about Augeas.
@ -0,0 +115,4 @@
config_file_paths = glob.glob(self.conf_available_dir + '/disable-*.conf')
for config_file_path in config_file_paths:
filename = os.path.basename(config_file_path)
directory_paths = self.aug.match(f'/files/etc/apache2/conf-available/{filename}/Directory')
Hardcode -> constant
Likewise.
@ -0,0 +132,4 @@
def disable_club_site(self, club_name: str):
logger.info(f'Disabling website for {club_name}')
directory_path = '/files/etc/apache2/conf-available/disable-club.conf/Directory'
Hardcode -> constant
@ -0,0 +138,4 @@
directory_path += '[%d]' % (num_directories + 1)
self.aug.set(directory_path + '/arg', f'/users/{club_name}/www')
self.aug.set(directory_path + '/directive', 'Include')
self.aug.set(directory_path + '/directive/arg', 'snippets/disable-club.conf')
Hardcode -> constant
@ -0,0 +179,4 @@
club_reps = ldap_srv.get_club_reps_non_member_terms(club_rep_uids)
# If a club rep's last non-member term was more than 3 terms ago, then
# they are considered inactive
cutoff_term = Term.current() - 3
Can we put the # terms into
ceo(d).ini
, or some constants file at least?Sure, we can make this a constant.
@ -0,0 +205,4 @@
for club_name in clubs_to_disable:
address = self.clubs[club_name]['email']
if address is not None:
mail_srv.send_club_website_has_been_disabled_message(club_name, address)
Knowing that mail sometimes fails to send, can we have remaining code short-circuit so we don't lose the old club reps if mail isn't successfully sent?
I added some code to log an error and continue - is this what you meant?
I meant we should stop the rest of the code execution and don't actually remove the reps if the email message isn't sent. Continuing would still mean the reps are removed right?
Ah OK. I still think that we should update the Apache configs (since unmaintained sites are still a security risk), but we can hold off on removing the inactive club reps.
Sure.
Just a few things left, good work! (see my reply to the last unresolved comment on the previous review as well)
@ -0,0 +224,4 @@
continue
try:
mail_srv.send_club_website_has_been_disabled_message(club_name, address)
except Exception:
Let's catch the specific exception that
send_club_website_has_been_disabled_message
raises.Actually, in this case, I think we really do want to catch all exceptions. Here's why:
@ -138,2 +138,4 @@
body,
)
def send_club_website_has_been_disabled_message(self, club: str, address: str):
Does this actually raise an exception? If so, which one?
See the comment above - we'd probably have to dig deep into the smtplib library to figure this out. It's not particularly useful information to us - all we need to know is that the message failed and move on to the next one.
LGTM