Disable inactive club sites #68

Merged
r389li merged 8 commits from disable-inactive-club-sites into master 2 months ago
Owner

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.

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.
merenber added 2 commits 3 months ago
merenber added 1 commit 3 months ago
e31fa2a88c fix lint error
merenber requested review from r389li 3 months ago
merenber requested review from a268wang 3 months ago
merenber requested review from r345liu 3 months ago
merenber added 1 commit 3 months ago
2dfa87c9b0 fix requirements.txt
merenber added 1 commit 3 months ago
merenber added 1 commit 3 months ago
de02171caa use contextmanager for mutex
r345liu approved these changes 3 months ago
r389li requested changes 3 months ago
This must be called BEFORE any of the methods below, using a context
expression like so:
with club_site_mgr.open_config_dir():
Owner

Should this be

with club_site_mgr.begin_transaction():

instead?

Should this be ```py with club_site_mgr.begin_transaction(): ``` instead?
Poster
Owner

Yes.

Yes.
r389li marked this conversation as resolved
Retrieves all clubs.
"""
# couldn't import the Term class from ceo_common.model due to some
Owner

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?

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?
Poster
Owner

It's not duplicated code - I just had to use 'Term' instead of Term in the type annotation (which is optional anyways).

It's not duplicated code - I just had to use `'Term'` instead of `Term` in the type annotation (which is optional anyways).
r389li marked this conversation as resolved
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)
Owner

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.

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.
Poster
Owner

We can create constants for conf-available/disable-club.conf and snippets/disable-club.conf, but all of the standard Apache config paths are hard-coded into Augeas, so they're not configurable.

We can create constants for `conf-available/disable-club.conf` and `snippets/disable-club.conf`, but all of the standard Apache config paths are [hard-coded into Augeas](https://github.com/hercules-team/augeas/blob/master/lenses/httpd.aug), so they're not configurable.
Owner

Sure, just those two then, we can leave Apache config paths.

Sure, just those two then, we can leave Apache config paths.
r389li marked this conversation as resolved
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')
Owner

Hardcode -> constant

Hardcode -> constant
Poster
Owner

See comment above about Augeas.

See comment above about Augeas.
r389li marked this conversation as resolved
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')
Owner

Hardcode -> constant

Hardcode -> constant
Poster
Owner

Likewise.

Likewise.
r389li marked this conversation as resolved
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'
Owner

Hardcode -> constant

Hardcode -> constant
r389li marked this conversation as resolved
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')
Owner

Hardcode -> constant

Hardcode -> constant
r389li marked this conversation as resolved
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
Owner

Can we put the # terms into ceo(d).ini, or some constants file at least?

Can we put the # terms into `ceo(d).ini`, or some constants file at least?
Poster
Owner

Sure, we can make this a constant.

Sure, we can make this a constant.
r389li marked this conversation as resolved
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)
Owner

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?

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?
Poster
Owner

I added some code to log an error and continue - is this what you meant?

I added some code to log an error and continue - is this what you meant?
Owner

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?

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?
Poster
Owner

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.

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.
Owner

Sure.

Sure.
r389li marked this conversation as resolved
r389li requested review from r345liu 3 months ago
merenber added 1 commit 2 months ago
c7d7e021fd add suggestions from r389li
merenber requested review from r389li 2 months ago
r389li requested changes 2 months ago
r389li left a comment
Owner

Just a few things left, good work! (see my reply to the last unresolved comment on the previous review as well)

Just a few things left, good work! (see my reply to the last unresolved comment on the previous review as well)
continue
try:
mail_srv.send_club_website_has_been_disabled_message(club_name, address)
except Exception:
Owner

Let's catch the specific exception that send_club_website_has_been_disabled_message raises.

Let's catch the specific exception that `send_club_website_has_been_disabled_message` raises.
Poster
Owner

Actually, in this case, I think we really do want to catch all exceptions. Here's why:

  1. We don't know exactly which Exceptions this function will raise. Sending an email can fail for a lot of different reasons (host is unreachable, host refuses connection, authentication fails, message is rejected, etc.).
  2. We don't want the failure of a single recipient to affect the other recipients. So no matter how a particular message fails, we want to keep on going.
Actually, in this case, I think we really do want to catch all exceptions. Here's why: 1. We don't know exactly which Exceptions this function will raise. Sending an email can fail for a lot of different reasons (host is unreachable, host refuses connection, authentication fails, message is rejected, etc.). 2. We don't want the failure of a single recipient to affect the other recipients. So no matter how a particular message fails, we want to keep on going.
r389li marked this conversation as resolved
body,
)
def send_club_website_has_been_disabled_message(self, club: str, address: str):
Owner

Does this actually raise an exception? If so, which one?

Does this actually raise an exception? If so, which one?
Poster
Owner

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.

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.
r389li marked this conversation as resolved
merenber added 1 commit 2 months ago
2a145cd83a don't remove club reps if email failed
merenber requested review from r389li 2 months ago
r389li approved these changes 2 months ago
r389li left a comment
Owner

LGTM

LGTM
r389li merged commit cfb5f77711 into master 2 months ago
r389li referenced this issue from a commit 2 months ago
r389li deleted branch disable-inactive-club-sites 2 months ago

Reviewers

a268wang was requested for review 3 months ago
r345liu was requested for review 3 months ago
r389li approved these changes 2 months ago
continuous-integration/drone/pr Build is passing
The pull request has been merged as cfb5f77711.
Sign in to join this conversation.
No Label
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: public/pyceo#68
Loading…
There is no content yet.