-
Notifications
You must be signed in to change notification settings - Fork 20
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
Configs for user/topic filters and folder/file format #25
base: master
Are you sure you want to change the base?
Conversation
…y exact topic matches are downloaded.
…rachy. Added config to define the recording file name format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking Great! Thanks for your contribution!
Some changes that I think are worth doing before merging
USERS = [ | ||
USERS_INCLUDE = [ | ||
# R"####@####.####", | ||
# R"####@####.####", | ||
] | ||
|
||
# Put here emails of the users you want to exclude from checking for recordings. Optional. | ||
USERS_EXCLUDE = [ | ||
# R"####@####.####", | ||
# R"####@####.####", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here isn't clear to me from the docs, What gets precedence?
END_DAY, END_MONTH, END_YEAR = None , 3, 2022 | ||
END_DAY, END_MONTH, END_YEAR = None, 3, 2022 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls revert the added spaces
if CONFIG.VERBOSE_OUTPUT: | ||
utils.print_dim('Found matching users:') | ||
|
||
for user_email, user_name in users: | ||
if user_email in CONFIG.USERS_EXCLUDE: | ||
users.pop(users.index((user_email, user_name))) | ||
continue | ||
|
||
if CONFIG.VERBOSE_OUTPUT: | ||
utils.print_dim(f'{user_name} <{user_email}>') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just run a filter on the list, then print it as is
topic_lower = str.lower(meeting['topic']) | ||
topic_lower_slug = utils.slugify(meeting['topic']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put outside the if
statement
@@ -208,6 +210,8 @@ def get_meetings(meeting_uuids): | |||
url = f'https://api.zoom.us/v2/meetings/{utils.double_encode(meeting_uuid)}/recordings' | |||
meetings.append(get_with_token(lambda t: requests.get(url=url, headers=get_headers(t))).json()) | |||
|
|||
utils.print_dim(f"Recordings found: {len(meetings)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the number of recordings
it's more complicated than that
GROUP_BY_TOPIC = True | ||
# Group records in a folder hierarchy using the order below. | ||
# Reorder or comment out any of the folder groups below to control the folder hierarchy created to orgainze the downloaded recording files. | ||
GROUP_FOLDERS_BY = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GROUP_BY
or FOLDER_HEIRARCHY
# Recording file name format to use when saving files. Reorder or comment out any file name format pieces below to control the file naming pattern. | ||
# Example: 2023-12-25t143021z__name-of-the-meeting__audio_transcript__ff625374.VTT | ||
FILE_NAME_FORMAT = [ | ||
R"RECORDING_START_DATETIME", # Recording start datetime | ||
R"RECORDING_NAME", # Recording name | ||
R"RECORDING_TYPE", # Recoding type | ||
R"FILE_ID", # Recording unique file ID | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit problematic
I use the file_id in the end to detect already downloaded file. I don't think it's a good idea to let users remove it...
file_name = utils.slugify( | ||
f'{recording_name}__{recording_type_suffix}{file_name_suffix}{file_id[-8:]}' | ||
) + '.' + ext | ||
recording_name = utils.slugify(f'{topic}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utils.slugify(topic)
return file_name | ||
|
||
def download_recording_file(download_url, host_folder, file_name, file_size, topic, recording_name, recording_start, user_email): | ||
folder_path = create_folder_path(host_folder, topic, recording_name, recording_start, user_email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would call this outside this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to pass all these parameters to the download method
folder_path = os.path.join(folder_path, user_email) | ||
if group_by == "TOPIC": | ||
folder_path = os.path.join(folder_path, topic) | ||
|
||
if CONFIG.GROUP_BY_RECORDING: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this still here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be part of your grouping mechanism, no?
Added new configs to: