Update code to pass checks for main.

[schmelz21]

🗣 Description

Specifically, to resolve pre-commit failures in main branch, so that new pull request are successful with GitHub checks.

💭 Motivation and context

Due to the inclusion of command-line updates, and new code pertaining to setup.py install requirements, previous tests developed were failing. The following coincide with resolving issues #7. #8 and Pull Request #31 - will allow for tests to pass.

Primary Updates:

• setuptools requirements
• Exception clauses to handle required assets (FileNotFoundError)
• Add code function load_customers

🧪 Testing

1. python3 setup.py install
2. pytest tests/test_pe_reports.py
3. pre-commit run --all-files
4. pe-reports $(date "+%Y-%m-%d") data/ output/ --db-creds-file=cyhy_connect/

✅ Checklist

• This PR has an informative and human-readable title.
• Changes are limited to a single goal - eschew scope creep!
• All relevant type-of-change labels have been added.
• These code changes follow cisagov code standards.
• All relevant repo and/or project documentation has been updated
  to reflect the changes in this PR.
• Tests have been added and/or modified to cover the changes in this PR.
• All new and existing tests pass.

[schmelz21]

@mcdonnnj Have the changes that were submitted satisfactorily resolve the issues you identified?

Sorry for the delay. This is on my list to finish re-review on today.

Ok.. I was just closing outdated and made the issue. Wasn't sure if we were good to go.
I will hold off then.. Thanks for jumping in with the heads up and your planed re-review.

[mcdonnnj]

Thank you for your continued work. I have some suggestions based on other changes you've made. Additionally there are still unresolved elements of my last review that I have commented on again.

[mcdonnnj]

There is still no check that there is a problem with the returned value from load_customers(). If there is a problem accessing org_names.json you currently output a log message, return a None, and then in

pe-reports/src/pe_reports/report_generator.py

Line 333 in 8f98208

agencies.append(names_obj[folder_name][0])

you immediately try and access the result without verifying it is a non-empty dictionary.

[mcdonnnj]

Those changes need to be made here as well. This logging message still does not accurately reflect the exception you are catching and it should return 1 to indicate an error.

[mcdonnnj]

That does not fix the problem here. This should still return a 1 to indicate an error state.

[cduhn17]

@mcdonnnj , @schmelz-ctr
Per commit 7115dd4 has addressed your comments about making sure agencies are present and a more clearly stated error state for when a directory exists. The thought was that its not exactly an error so the logging has been changed to INFO and that it is acceptable for the directory to exists. Please review and provide guidance for changes.

[schmelz21]

@mcdonnnj , @schmelz-ctr
Per commit 7115dd4 has addressed your comments about making sure agencies are present and a more clearly stated error state for when a directory exists. The thought was that its not exactly an error so the logging has been changed to INFO and that it is acceptable for the directory to exists. Please review and provide guidance for changes.

@cduhn17 if to imply it is not an error but more of a check, can we then remove traceback arg exc_info=True. Alternatively we could possibly use logging.warning too if it satisfies @mcdonnnj concerns.

Code:

    try:
        os.mkdir(f"{args['OUTPUT_DIRECTORY']}")
    except FileExistsError as err:
        logging.info(f"The output directory exists. {err}")
        return 0

[stewartl97]

As a note to all involved, the PRs #31 and #33 are extremely old and only valid for our LookingGlass data pulls for which the contract ends on 9/30 and they will be OBE. Is there any possibility we can have them completed before I have to redirect the team to continue other work?

[schmelz21]

As a note to all involved, the PRs #31 and #33 are extremely old and only valid for our LookingGlass data pulls for which the contract ends on 9/30 and they will be OBE. Is there any possibility we can have them completed before I have to redirect the team to continue other work?

@mcdonnnj, Although we are going to sunset this version, I was hoping we could at least close out this last pr as it will allow for this version to pass checks. It also leaves us with working scripts in the event we need to revisit this code.
Are you able to re-review @cduhn17 's last commit, inputs. Thanks

[mcdonnnj]

As a note to all involved, the PRs #31 and #33 are extremely old and only valid for our LookingGlass data pulls for which the contract ends on 9/30 and they will be OBE. Is there any possibility we can have them completed before I have to redirect the team to continue other work?

@mcdonnnj, Although we are going to sunset this version, I was hoping we could at least close out this last pr as it will allow for this version to pass checks. It also leaves us with working scripts in the event we need to revisit this code. Are you able to re-review @cduhn17 's last commit, inputs. Thanks

@schmelz21 This remains unresolved. As shown below the names_obj dictionary is still accessed without any consideration for whether or not load_customer() has returned an empty dictionary.

pe-reports/src/pe_reports/report_generator.py

Lines 328 to 332 in 7115dd4

	names_obj = load_customers()
	for root, folders, files in contents:
	for folder_name in folders:
	agencies.append(names_obj[folder_name][0])

[schmelz21]

As a note to all involved, the PRs #31 and #33 are extremely old and only valid for our LookingGlass data pulls for which the contract ends on 9/30 and they will be OBE. Is there any possibility we can have them completed before I have to redirect the team to continue other work?

@mcdonnnj, Although we are going to sunset this version, I was hoping we could at least close out this last pr as it will allow for this version to pass checks. It also leaves us with working scripts in the event we need to revisit this code. Are you able to re-review @cduhn17 's last commit, inputs. Thanks

@schmelz21 This remains unresolved. As shown below the names_obj dictionary is still accessed without any consideration for whether or not load_customer() has returned an empty dictionary.

pe-reports/src/pe_reports/report_generator.py

Lines 328 to 332 in 7115dd4

	names_obj = load_customers()
	for root, folders, files in contents:
	for folder_name in folders:
	agencies.append(names_obj[folder_name][0])

As a note to all involved, the PRs #31 and #33 are extremely old and only valid for our LookingGlass data pulls for which the contract ends on 9/30 and they will be OBE. Is there any possibility we can have them completed before I have to redirect the team to continue other work?

@mcdonnnj, Although we are going to sunset this version, I was hoping we could at least close out this last pr as it will allow for this version to pass checks. It also leaves us with working scripts in the event we need to revisit this code. Are you able to re-review @cduhn17 's last commit, inputs. Thanks

@schmelz21 This remains unresolved. As shown below the names_obj dictionary is still accessed without any consideration for whether or not load_customer() has returned an empty dictionary.

pe-reports/src/pe_reports/report_generator.py

Lines 328 to 332 in 7115dd4

	names_obj = load_customers()
	for root, folders, files in contents:
	for folder_name in folders:
	agencies.append(names_obj[folder_name][0])

@mcdonnnj, appreciate the quick response and apologies for the miss... circling in @cduhn17 to review and will get back with updates.

[cduhn17]

As a note to all involved, the PRs #31 and #33 are extremely old and only valid for our LookingGlass data pulls for which the contract ends on 9/30 and they will be OBE. Is there any possibility we can have them completed before I have to redirect the team to continue other work?

@mcdonnnj, Although we are going to sunset this version, I was hoping we could at least close out this last pr as it will allow for this version to pass checks. It also leaves us with working scripts in the event we need to revisit this code. Are you able to re-review @cduhn17 's last commit, inputs. Thanks

@schmelz21 This remains unresolved. As shown below the names_obj dictionary is still accessed without any consideration for whether or not load_customer() has returned an empty dictionary.

pe-reports/src/pe_reports/report_generator.py

Lines 328 to 332 in 7115dd4

	names_obj = load_customers()
	for root, folders, files in contents:
	for folder_name in folders:
	agencies.append(names_obj[folder_name][0])

As a note to all involved, the PRs #31 and #33 are extremely old and only valid for our LookingGlass data pulls for which the contract ends on 9/30 and they will be OBE. Is there any possibility we can have them completed before I have to redirect the team to continue other work?

@mcdonnnj, Although we are going to sunset this version, I was hoping we could at least close out this last pr as it will allow for this version to pass checks. It also leaves us with working scripts in the event we need to revisit this code. Are you able to re-review @cduhn17 's last commit, inputs. Thanks

@schmelz21 This remains unresolved. As shown below the names_obj dictionary is still accessed without any consideration for whether or not load_customer() has returned an empty dictionary.

pe-reports/src/pe_reports/report_generator.py

Lines 328 to 332 in 7115dd4

	names_obj = load_customers()
	for root, folders, files in contents:
	for folder_name in folders:
	agencies.append(names_obj[folder_name][0])

@mcdonnnj, appreciate the quick response and apologies for the miss... circling in @cduhn17 to review and will get back with updates.

@schmelz21 , @mcdonnnj
Per commit eeb47ed I was thinking we had addressed the question of if the load_customer return value is empty or not right in the load_customer function.

pe-reports/src/pe_reports/report_generator.py

Lines 65 to 73 in 7115dd4

	def load_customers():
	"""Export PowerPoint report set to output directory."""
	try:
	if os.path.exists(CUSTOMERS) and os.path.getsize(CUSTOMERS) != 0:
	with open(CUSTOMERS) as customers_file:
	return json.load(customers_file)
	except FileNotFoundError as not_found:
	logging.error("%s : Missing input data. No report generated.", not_found)
	return dict()

[mcdonnnj]

As a note to all involved, the PRs #31 and #33 are extremely old and only valid for our LookingGlass data pulls for which the contract ends on 9/30 and they will be OBE. Is there any possibility we can have them completed before I have to redirect the team to continue other work?

@mcdonnnj, Although we are going to sunset this version, I was hoping we could at least close out this last pr as it will allow for this version to pass checks. It also leaves us with working scripts in the event we need to revisit this code. Are you able to re-review @cduhn17 's last commit, inputs. Thanks

@schmelz21 This remains unresolved. As shown below the names_obj dictionary is still accessed without any consideration for whether or not load_customer() has returned an empty dictionary.

pe-reports/src/pe_reports/report_generator.py

Lines 328 to 332 in 7115dd4

	names_obj = load_customers()
	for root, folders, files in contents:
	for folder_name in folders:
	agencies.append(names_obj[folder_name][0])

As a note to all involved, the PRs #31 and #33 are extremely old and only valid for our LookingGlass data pulls for which the contract ends on 9/30 and they will be OBE. Is there any possibility we can have them completed before I have to redirect the team to continue other work?

@mcdonnnj, Although we are going to sunset this version, I was hoping we could at least close out this last pr as it will allow for this version to pass checks. It also leaves us with working scripts in the event we need to revisit this code. Are you able to re-review @cduhn17 's last commit, inputs. Thanks

@schmelz21 This remains unresolved. As shown below the names_obj dictionary is still accessed without any consideration for whether or not load_customer() has returned an empty dictionary.

pe-reports/src/pe_reports/report_generator.py

Lines 328 to 332 in 7115dd4

	names_obj = load_customers()
	for root, folders, files in contents:
	for folder_name in folders:
	agencies.append(names_obj[folder_name][0])

@mcdonnnj, appreciate the quick response and apologies for the miss... circling in @cduhn17 to review and will get back with updates.

@schmelz21 , @mcdonnnj Per commit eeb47ed I was thinking we had addressed the question of if the load_customer return value is empty or not right in the load_customer function.

pe-reports/src/pe_reports/report_generator.py

Lines 65 to 73 in 7115dd4

	def load_customers():
	"""Export PowerPoint report set to output directory."""
	try:
	if os.path.exists(CUSTOMERS) and os.path.getsize(CUSTOMERS) != 0:
	with open(CUSTOMERS) as customers_file:
	return json.load(customers_file)
	except FileNotFoundError as not_found:
	logging.error("%s : Missing input data. No report generated.", not_found)
	return dict()

@cduhn17 All that has been done is making sure that the function returns a dictionary no matter what. However if there is an issue accessing the file in some way it returns an empty dictionary. This will result in a KeyError if you attempt to access a specific key in the empty dictionary. There should either be a check if names_obj is empty or the attempt to access a key that may or may not exist should be done in a safe manner (or both).

[cduhn17]

@

As a note to all involved, the PRs #31 and #33 are extremely old and only valid for our LookingGlass data pulls for which the contract ends on 9/30 and they will be OBE. Is there any possibility we can have them completed before I have to redirect the team to continue other work?

@mcdonnnj, Although we are going to sunset this version, I was hoping we could at least close out this last pr as it will allow for this version to pass checks. It also leaves us with working scripts in the event we need to revisit this code. Are you able to re-review @cduhn17 's last commit, inputs. Thanks

@schmelz21 This remains unresolved. As shown below the names_obj dictionary is still accessed without any consideration for whether or not load_customer() has returned an empty dictionary.

pe-reports/src/pe_reports/report_generator.py

Lines 328 to 332 in 7115dd4

	names_obj = load_customers()
	for root, folders, files in contents:
	for folder_name in folders:
	agencies.append(names_obj[folder_name][0])

As a note to all involved, the PRs #31 and #33 are extremely old and only valid for our LookingGlass data pulls for which the contract ends on 9/30 and they will be OBE. Is there any possibility we can have them completed before I have to redirect the team to continue other work?

@mcdonnnj, Although we are going to sunset this version, I was hoping we could at least close out this last pr as it will allow for this version to pass checks. It also leaves us with working scripts in the event we need to revisit this code. Are you able to re-review @cduhn17 's last commit, inputs. Thanks

@schmelz21 This remains unresolved. As shown below the names_obj dictionary is still accessed without any consideration for whether or not load_customer() has returned an empty dictionary.

pe-reports/src/pe_reports/report_generator.py

Lines 328 to 332 in 7115dd4

	names_obj = load_customers()
	for root, folders, files in contents:
	for folder_name in folders:
	agencies.append(names_obj[folder_name][0])

@mcdonnnj, appreciate the quick response and apologies for the miss... circling in @cduhn17 to review and will get back with updates.

@schmelz21 , @mcdonnnj Per commit eeb47ed I was thinking we had addressed the question of if the load_customer return value is empty or not right in the load_customer function.

pe-reports/src/pe_reports/report_generator.py

Lines 65 to 73 in 7115dd4

	def load_customers():
	"""Export PowerPoint report set to output directory."""
	try:
	if os.path.exists(CUSTOMERS) and os.path.getsize(CUSTOMERS) != 0:
	with open(CUSTOMERS) as customers_file:
	return json.load(customers_file)
	except FileNotFoundError as not_found:
	logging.error("%s : Missing input data. No report generated.", not_found)
	return dict()

@cduhn17 All that has been done is making sure that the function returns a dictionary no matter what. However if there is an issue accessing the file in some way it returns an empty dictionary. This will result in a KeyError if you attempt to access a specific key in the empty dictionary. There should either be a check if names_obj is empty or the attempt to access a key that may or may not exist should be done in a safe manner (or both).

@mcdonnnj ,

I have addressed your recent request for a change to address an empty dictionary see commit e1df499

pe-reports/src/pe_reports/report_generator.py

Lines 324 to 357 in e1df499

	def generate_reports(db, datestring, data_directory, output_directory):
	"""Process steps for generating report data."""
	agencies = []
	contents = os.walk(data_directory)
	try:
	if load_customers():
	names_obj = load_customers()
	for root, folders, files in contents:
	for folder_name in folders:
	agencies.append(names_obj[folder_name][0])
	else:
	raise KeyError("The return data from load_customer was empty.")
	except KeyError as ke:
	logging.critical(ke)
	try:
	if agencies:
	requests = get_requests(db, agency_list=agencies)
	request_data = list(requests)
	except TypeError:
	return 4
	try:
	cyhy_agencies = len(request_data)
	logging.debug(f"{cyhy_agencies} agencies found in CyHy")
	except pymongo.errors.OperationFailure:
	logging.critical(
	"Mongo database error while counting the number of request documents returned",
	exc_info=True,
	)
	generated_reports = 0

[schmelz21]

Closing pull-request as efforts to update the main branch have been canceled. Routines used in version 0.1.1 have been sunsetted with no future plans to update.