ceo_common/interfaces/IClubWebHostingService.py

@@ -15,7 +15,7 @@ class IClubWebHostingService(Interface):
         This must be called BEFORE any of the methods below, using a context
         expression like so:
 
-        with club_site_mgr.open_config_dir():
+        with club_site_mgr.begin_transaction():
             club_site_mgr.disable_club_site('club1')
             ...
             club_site_mgr.commit()

ceod/model/ClubWebHostingService.py

@@ -5,6 +5,7 @@ import os
 import re
 import subprocess
 from threading import Lock
+import traceback
 from typing import List
 
 from augeas import Augeas
@@ -22,6 +23,15 @@ from ceo_common.model import Term
 # FIXME: don't assume this
 APACHE_USERDIR_RE = re.compile(r'^/users/(?P<club_name>[0-9a-z-]+)/www/?$')
 
+# This is where all the <Directory> directives for disabled clubs are stored.
+APACHE_DISABLED_CLUBS_FILE = 'conf-available/disable-club.conf'
+# This is the file which contains the snippet to disable a single club.
+APACHE_DISABLING_SNIPPET_FILE = 'snippets/disable-club.conf'
+# This is the maximum number of consecutive terms for which a club is allowed
+# to have no active club reps. After this many terms have passed, the club's
+# website may be disabled.
+MAX_TERMS_WITH_NO_ACTIVE_CLUB_REPS = 3
+
 logger = logger_factory(__name__)
 
 
@@ -39,7 +49,7 @@ class ClubWebHostingService:
         self.conf_available_dir = os.path.join(self.apache_dir, 'conf-available')
         self.clubs_home = cfg.get('clubs_home')
         self.aug = None
-        self.clubs = defaultdict(lambda: {'disabled': False, 'email': None})
+        self.clubs = None
         self.made_at_least_one_change = False
         self.lock = Lock()
 
@@ -56,6 +66,7 @@ class ClubWebHostingService:
         with self._hold_lock():
             try:
                 self.aug = Augeas(self.aug_root)
+                self.clubs = defaultdict(lambda: {'disabled': False, 'email': None})
                 self._get_club_emails()
                 self._get_disabled_sites()
                 yield
@@ -63,7 +74,7 @@ class ClubWebHostingService:
                 if self.aug is not None:
                     self.aug.close()
                 self.aug = None
-                self.clubs.clear()
+                self.clubs = None
                 self.made_at_least_one_change = False
 
     def _run(self, args: List[str], **kwargs):
@@ -81,7 +92,7 @@ class ClubWebHostingService:
             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)
+        self._run(['git', 'add', APACHE_DISABLED_CLUBS_FILE], cwd=self.apache_dir)
         self._run(['git', 'commit', '-m', '[ceo] disable club websites'], cwd=self.apache_dir)
 
     def commit(self):
@@ -132,13 +143,14 @@ class ClubWebHostingService:
 
     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'
+        directory_path = f'/files/etc/apache2/{APACHE_DISABLED_CLUBS_FILE}/Directory'
         num_directories = len(self.aug.match(directory_path))
         # Create a new <Directory> section
         directory_path += '[%d]' % (num_directories + 1)
+        # FIXME: use self.clubs_home here instead (need to update unit tests)
         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')
+        self.aug.set(directory_path + '/directive/arg', APACHE_DISABLING_SNIPPET_FILE)
 
         self.clubs[club_name]['disabled'] = True
         self.made_at_least_one_change = True
@@ -153,6 +165,7 @@ class ClubWebHostingService:
                     return True
         return False
 
+    # This method needs to be called from within a transaction (uses self.clubs)
     def _need_to_disable_inactive_club_site(self, club_name: str) -> bool:
         if self.clubs[club_name]['disabled']:
             # already disabled - nothing to do
@@ -170,16 +183,16 @@ class ClubWebHostingService:
     ) -> List[str]:
         ldap_srv = component.getUtility(ILDAPService)
         mail_srv = component.getUtility(IMailService)
-        clubs = ldap_srv.get_clubs()
+        all_clubs = ldap_srv.get_clubs()
         club_rep_uids = list({
             member
-            for club in clubs
+            for club in all_clubs
             for member in club.members
         })
         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
+        # If a club rep's last non-member term is before cutoff_term, then
         # they are considered inactive
-        cutoff_term = Term.current() - 3
+        cutoff_term = Term.current() - MAX_TERMS_WITH_NO_ACTIVE_CLUB_REPS
         active_club_reps = {
             club_rep
             for club_rep, non_member_terms in club_reps.items()
@@ -188,31 +201,39 @@ class ClubWebHostingService:
         # a club is inactive if it does not have at least one active club rep
         inactive_clubs = [
             club
-            for club in clubs
+            for club in all_clubs
             if not any(map(lambda member: member in active_club_reps, club.members))
         ]
-        clubs_to_disable = [
-            club.cn
-            for club in inactive_clubs
-            if self._need_to_disable_inactive_club_site(club.cn)
-        ]
-        if dry_run:
-            return clubs_to_disable
         with self.begin_transaction():
+            clubs_to_disable = [
+                club.cn
+                for club in inactive_clubs
+                if self._need_to_disable_inactive_club_site(club.cn)
+            ]
+            if dry_run:
+                return clubs_to_disable
+
             for club_name in clubs_to_disable:
                 self.disable_club_site(club_name)
             self.commit()
-            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)
-            if remove_inactive_club_reps:
-                for club in clubs:
-                    # club.members gets modified after calling club.remove_member(),
-                    # so we need to make a copy
-                    members = club.members.copy()
-                    for member in members:
-                        if member not in active_club_reps:
-                            logger.info(f'Removing {member} from {club.cn}')
-                            club.remove_member(member)
+            # self.clubs is set to None once the transaction closes, so make a copy now
+            clubs_info = self.clubs.copy()
+        for club_name in clubs_to_disable:
+            address = clubs_info['email']
+            if address is None:
+                continue
+            try:
+                mail_srv.send_club_website_has_been_disabled_message(club_name, address)
+            except Exception:
+                trace = traceback.format_exc()
+                logger.error(f'Failed to send email to {address}:\n{trace}')
+        if remove_inactive_club_reps:
+            for club in all_clubs:
+                # club.members gets modified after calling club.remove_member(),
+                # so we need to make a copy
+                members = club.members.copy()
+                for member in members:
+                    if member not in active_club_reps:
+                        logger.info(f'Removing {member} from {club.cn}')
+                        club.remove_member(member)
         return clubs_to_disable

tests/ceod/model/test_webhosting.py

@@ -125,6 +125,10 @@ def test_disable_inactive_club_sites(
                 )
                 assert pat.search(disable_club_conf_content) is not None
 
+            with g_admin_ctx():
+                # Club sites should only be disabled once
+                assert webhosting_srv.disable_sites_for_inactive_clubs() == []
+
     mock_mail_server.messages.clear()