public
/
pyceo
27
1
Fork 3
Code Issues 25 Pull Requests 5 Projects Releases 1 Wiki Activity

Disable inactive club sites #68

Merged
r389li merged 8 commits from disable-inactive-club-sites into master 2022-07-22 23:52:01 -04:00
Conversation 0 Commits 8 Files Changed 35 +835 -33
merenber commented 2022-07-11 23:48:26 -04:00
Owner
Copy Link

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 2022-07-11 23:48:27 -04:00
merenber added 1 commit 2022-07-11 23:49:47 -04:00
continuous-integration/drone/pr Build is failing Details
e31fa2a88c fix lint error
merenber requested review from r389li 2022-07-11 23:50:36 -04:00
merenber requested review from a268wang 2022-07-11 23:50:36 -04:00
merenber requested review from r345liu 2022-07-11 23:50:36 -04:00
merenber added 1 commit 2022-07-12 00:59:28 -04:00
continuous-integration/drone/pr Build is failing Details
2dfa87c9b0 fix requirements.txt
merenber added 1 commit 2022-07-12 01:09:15 -04:00
continuous-integration/drone/pr Build is passing Details
c6218bf389 install libaugeas0 before python-augeas in .drone.yml
merenber added 1 commit 2022-07-12 01:32:07 -04:00
continuous-integration/drone/pr Build is passing Details
de02171caa use contextmanager for mutex
r345liu approved these changes 2022-07-12 21:26:28 -04:00
r389li requested changes 2022-07-17 21:58:13 -04:00
ceo_common/interfaces/IClubWebHostingService.py Outdated
@ -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():
r389li commented 2022-07-13 15:46:51 -04:00
Owner
Copy Link

Should this be

with club_site_mgr.begin_transaction():

instead?

Should this be ```py with club_site_mgr.begin_transaction(): ``` instead?
merenber commented 2022-07-20 20:22:42 -04:00
Author
Owner
Copy Link

Yes.

Yes.
r389li marked this conversation as resolved
ceo_common/interfaces/ILDAPService.py
@ -103,0 +106,4 @@
Retrieves all clubs.
"""
# couldn't import the Term class from ceo_common.model due to some
r389li commented 2022-07-17 21:56:37 -04:00
Owner
Copy Link

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?
merenber commented 2022-07-20 20:23:31 -04:00
Author
Owner
Copy Link

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).
👍 1
r389li marked this conversation as resolved
ceod/model/ClubWebHostingService.py
@ -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)
r389li commented 2022-07-17 21:52:51 -04:00
Owner
Copy Link

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.
merenber commented 2022-07-20 20:26:43 -04:00
Author
Owner
Copy Link

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.
r389li commented 2022-07-21 19:30:22 -04:00
Owner
Copy Link

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
ceod/model/ClubWebHostingService.py
@ -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')
r389li commented 2022-07-17 21:53:14 -04:00
Owner
Copy Link

Hardcode -> constant

Hardcode -> constant
merenber commented 2022-07-20 20:27:05 -04:00
Author
Owner
Copy Link

See comment above about Augeas.

See comment above about Augeas.
r389li marked this conversation as resolved
ceod/model/ClubWebHostingService.py
@ -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')
r389li commented 2022-07-17 21:53:18 -04:00
Owner
Copy Link

Hardcode -> constant

Hardcode -> constant
merenber commented 2022-07-20 20:27:19 -04:00
Author
Owner
Copy Link

Likewise.

Likewise.
r389li marked this conversation as resolved
ceod/model/ClubWebHostingService.py
@ -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'
r389li commented 2022-07-17 21:53:24 -04:00
Owner
Copy Link

Hardcode -> constant

Hardcode -> constant
r389li marked this conversation as resolved
ceod/model/ClubWebHostingService.py
@ -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')
r389li commented 2022-07-17 21:53:38 -04:00
Owner
Copy Link

Hardcode -> constant

Hardcode -> constant
r389li marked this conversation as resolved
ceod/model/ClubWebHostingService.py
@ -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
r389li commented 2022-07-17 21:22:28 -04:00
Owner
Copy Link

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?
merenber commented 2022-07-20 20:40:32 -04:00
Author
Owner
Copy Link

Sure, we can make this a constant.

Sure, we can make this a constant.
r389li marked this conversation as resolved
ceod/model/ClubWebHostingService.py Outdated
@ -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)
r389li commented 2022-07-17 21:23:35 -04:00
Owner
Copy Link

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?
merenber commented 2022-07-20 20:55:25 -04:00
Author
Owner
Copy Link

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?
r389li commented 2022-07-21 19:55:13 -04:00
Owner
Copy Link

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?
merenber commented 2022-07-21 21:59:11 -04:00
Author
Owner
Copy Link

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.
r389li commented 2022-07-22 23:48:24 -04:00
Owner
Copy Link

Sure.

Sure.
r389li marked this conversation as resolved
r389li requested review from r345liu 2022-07-17 21:58:18 -04:00
merenber added 1 commit 2022-07-20 21:05:42 -04:00
continuous-integration/drone/pr Build is passing Details
c7d7e021fd add suggestions from r389li
merenber requested review from r389li 2022-07-20 21:05:50 -04:00
r389li requested changes 2022-07-21 19:56:17 -04:00
r389li left a comment
Owner
Copy Link

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)
ceod/model/ClubWebHostingService.py
@ -0,0 +224,4 @@
continue
try:
mail_srv.send_club_website_has_been_disabled_message(club_name, address)
except Exception:
r389li commented 2022-07-21 19:54:05 -04:00
Owner
Copy Link

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.
merenber commented 2022-07-21 22:04:04 -04:00
Author
Owner
Copy Link

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.
👍 1
r389li marked this conversation as resolved
ceod/model/MailService.py
@ -138,2 +138,4 @@
body,
)
def send_club_website_has_been_disabled_message(self, club: str, address: str):
r389li commented 2022-07-21 19:53:48 -04:00
Owner
Copy Link

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

Does this actually raise an exception? If so, which one?
merenber commented 2022-07-21 22:05:01 -04:00
Author
Owner
Copy Link

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.
👍 1
r389li marked this conversation as resolved
merenber added 1 commit 2022-07-21 22:07:41 -04:00
continuous-integration/drone/pr Build is passing Details
2a145cd83a don't remove club reps if email failed
merenber requested review from r389li 2022-07-21 22:51:22 -04:00
r389li approved these changes 2022-07-22 23:51:45 -04:00
r389li left a comment
Owner
Copy Link

LGTM

LGTM
r389li merged commit cfb5f77711 into master 2022-07-22 23:52:01 -04:00
r389li deleted branch disable-inactive-club-sites 2022-07-22 23:52:10 -04:00
Sign in to join this conversation.
Reviewers
No reviewers
a268wang
r345liu
r389li
Labels
Clear labels   
priority
high
High priority

  
priority
low
Low priority

  
priority
medium
Medium priority

  
priority
very high
Very high priority

  
BUG

   
Feature

   
High Priority

   
Low Priority

   
Medium Priority
No Label
priority
high
priority
low
priority
medium
priority
very high
BUG
Feature
High Priority
Low Priority
Medium Priority
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: public/pyceo#68
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?