Skip to content

Conversation

@jonavellecuerdo
Copy link
Contributor

@jonavellecuerdo jonavellecuerdo commented Aug 6, 2024

Purpose and background context

Implement a user interface for the Flask application. Developing the user interface will take an iterative approach before launching to production, and this repo is likely to evolve after getting feedback from stakeholders and other folks (DataEng / EngX). It's also worth noting that:

  • In developing the frontend, importing the MIT Libraries stylesheet proved really helpful (shoutout to @matt-bernhardt for the advice 🎉 ) and then proceeding to build the templates to conform to the stylesheet was definitely the right approach!
    • The HTML templates were for the header, footer, breadcrumb were slightly modified versions of the HTML templates from MITLibraries/mitlib-style/_includes on GitHub.
    • The additional stylesheet contains--as its name implies--additional stylings, such as the inclusion of the platform name in the header.
  • This PR does not include any authentication at this point, and there will be a separate body of work for that component.
  • No linting or additional unit tests are applied at this point.
    • The hope is that reviewing the application in Dev will be sufficient for purposes of reviewing this PR.
    • Linting and unit tests will be applied before launching to Prod.
    • Want to prioritize understanding and implementing the authentication process before circling back to linting + unit tests.

How can a reviewer manually see the effects of these changes?

A Docker image has been published to Dev and the test Lambda function URL was redeployed to use the latest image with these changes. See section "Review the draft application" in the comment on Jira ticket IN-1015 for more information.

Includes new or updated dependencies?

NO

Changes expectations for external applications?

NO

What are the relevant tickets?

https://mitlibraries.atlassian.net/browse/IN-1015

Developer

  • All new ENV is documented in README
  • All new ENV has been added to staging and production environments
  • All related Jira tickets are linked in commit message(s)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer(s)

  • The commit message is clear and follows our guidelines (not just this PR message)
  • There are appropriate tests covering any new functionality
  • The provided documentation is sufficient for understanding any new functionality introduced
  • Any manual tests have been performed and verified
  • New dependencies are appropriate or there were no changes

@jonavellecuerdo jonavellecuerdo force-pushed the ui-dev branch 4 times, most recently from 7132aff to 85d0c8f Compare August 8, 2024 16:42
@jonavellecuerdo jonavellecuerdo self-assigned this Aug 8, 2024
Comment on lines +23 to +28
fetch-mitlib-style: # Fetch MIT Libraries style guide
mkdir -p tmp
curl -o tmp/assets.zip https://codeload.github.com/MITLibraries/mitlib-style/zip/master
unzip -o -d tmp tmp/assets.zip
# Copy the compiled stylesheet
cp tmp/mitlib-style-master/dest/css/libraries-main.min.css webapp/static/libraries-main.min.css
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 11 to 57
@app.route("/")
def hello_world() -> str:
return f"Hello, World! WORKSPACE={app.config["WORKSPACE"]}"
def index() -> str:
return render_template("index.html")

@app.route("/about")
def about() -> str:
return "This is the About page!"
@app.route("/process-invoices")
def process_invoices() -> str:
return render_template("process_invoices.html")

@app.route("/process-invoices/run/<run_type>")
def process_invoices_run(run_type):
ecs_client = ECSClient()
if active_tasks := ecs_client.get_active_tasks():
return render_template(
"errors/error_400_cannot_run_multiple_tasks.html",
active_tasks=active_tasks,
)
if run_type == "review":
task_arn = ecs_client.execute_review_run()
elif run_type == "final":
task_arn = ecs_client.execute_final_run()
task_id = task_arn.split("/")[-1]
return redirect(url_for("process_invoices_status", task_id=task_id))

@app.route("/process-invoices/status/<task_id>")
def process_invoices_status(task_id):
try:
_, logs = get_task_status_and_logs(task_id)
except ECSTaskLogStreamDoesNotExistError as exception:
return render_template(
"errors/error_404_object_not_found.html", error=exception
)

return render_template(
"process_invoices_status.html",
logs=logs,
task_id=task_id,
)

@app.route("/process-invoices/status/<task_id>/data")
def process_invoices_status_data(task_id):
try:
task_status, logs = get_task_status_and_logs(task_id)
except ECSTaskLogStreamDoesNotExistError:
task_status = "UNKNOWN"
logs = ["Log stream does not exist."]

return jsonify({"status": task_status, "logs": logs})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These routes define the structure of each page of the Flask app.

Comment on lines 30 to 62
<script>
// URL to fetch JSON data from
const url = "{{ url_for('process_invoices_status_data', task_id=task_id) }}";
var status_element = document.getElementById("status");
var interval = setInterval(function () {
fetch_monitor_data()
if (status_element.textContent === "COMPLETED"){
clearInterval(interval);
return;
}
}, 2000);
// Fetch JSON data
function fetch_monitor_data() {
fetch(url)
.then(response => response.json())
.then(data => {
console.log(data);
// Update the content of the elements
status_element.textContent = data.status;
const logs_element = document.getElementById('logs');
logs_element.textContent = ''
data.logs.forEach(item => {
const line = document.createElement('p');
line.textContent = item;
logs_element.appendChild(line);
});
})
.catch(error => {
console.error('Error fetching data:', error);
});
}

</script>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This JavaScript is what updates the task status page while the ECS task is running. It will repeatedly send a request to the /process-invoices/status/<task_id>/data route to get the status of the ECS task and update the page with the retrieved information.

Comment on lines +49 to +53
return self.run(run_type="review", commands=["--blah"])

def execute_final_run(self) -> str | None:
logger.info("Executing ECS task for 'Final Run'")
return self.run(run_type="final", commands=["--real", "--final"])
return self.run(run_type="final", commands=["--blah", "--bloop"])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

temporary command args while we're running tests

Why these changes are being introduced:
* These changes implement a UI for the SAP Invoices
Flask app, guided by the standards used for MIT
Libraries (MITL)-owned apps.

How this addresses that need:
* Import MIT Libraries style guide
* Update HTML templates in accordance with style guide
* Streamline logic in app routes

Side effects of this change:
* None

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/IN-1015
@jonavellecuerdo jonavellecuerdo changed the title [wip] ui dev Create user interface for Flask app Aug 8, 2024
@jonavellecuerdo jonavellecuerdo marked this pull request as ready for review August 8, 2024 17:07
@coveralls
Copy link

coveralls commented Aug 8, 2024

Pull Request Test Coverage Report for Build 10308011811

Details

  • 28 of 60 (46.67%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-10.4%) to 86.76%

Changes Missing Coverage Covered Lines Changed/Added Lines %
webapp/utils/aws/ecs.py 8 9 88.89%
webapp/utils/init.py 3 13 23.08%
webapp/app.py 16 37 43.24%
Totals Coverage Status
Change from base Build 10184135969: -10.4%
Covered Lines: 249
Relevant Lines: 287

💛 - Coveralls

Copy link

@ehanson8 ehanson8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, the documentation makes this process very clear!

Comment on lines 8 to 28
The method relies on instances of ECSClient and CloudWatchLogsClient.
To understand the flow of logic described below, some important clarification:
* "task exists": This means the task appears in the Amazon ECS task history.
Tasks are retained for about an hour after stopping.
* "task does not exist": This means the task does not show up in the Amazon ECS
task history. This does not mean the task wasn't executed -- just that it wasn't
run within the last hour. A CloudWatch log stream may exist for the task.
The flow of logic is as follows:
1. Get the status of a task.
- If the ECS task exists, the status for the task is retrieved.
When task_status = "STOPPED" the ECS task is considered "COMPLETED" (the task run completed).
- If the ECS task does not exist, proceed.
2. Get the logs for the task.
- Whether or not the ECS task exists, retrieve the logs when task_status="COMPLETED".
This ensures full set of logs are retrieved only when the task run completed.
If logs are not found, raise ECSTaskLogStreamDoesNotExistError.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great docstring!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clear description of the process!

Copy link
Contributor

@ghukill ghukill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved!

As discussed in slack a bit, I think the task status and logs retrieval could be tightented up a bit, but the UI itself is looking great. And, all the description and documentation around it.

Seems like it would make sense to break those task status and logs updates into a new PR, so happily approving this one as-is.

@jonavellecuerdo jonavellecuerdo merged commit aeb61ac into main Aug 9, 2024
@jonavellecuerdo jonavellecuerdo deleted the ui-dev branch August 9, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants