Skip to content
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

Agents-transfers logging reports #65

Merged
merged 1 commit into from
Dec 7, 2020
Merged

Conversation

ross-spencer
Copy link
Contributor

@ross-spencer ross-spencer commented Sep 24, 2020

This finally completes an API endpoint and basic reports to list information about users and ingests for which they were responsible for. Delivering a tabular representation of that work and a prototype timeline using plotly express.

The API endpoint to access the data will be at:

  • curl -X GET "http://127.0.0.1:5000/api/data/agents-transfers/1" -H "accept: application/json"

Prototype gantt

image

Table

image

The work leverages much of the testing efforts of Tessa so thank you for that! And also tries to comply with new efforts around base templates as well. Hopefully it's looking okay.

@peterVG we'd spoken before about what methods we might use to deliver charts and there might be some good exploration to do there. For this work the code that enables that is located in this commit which you might be interested in: d8cf25d

Connected to #18

@ross-spencer ross-spencer mentioned this pull request Sep 30, 2020
@ross-spencer ross-spencer force-pushed the dev/issue-18-agents-table branch from d4a651d to 41464d2 Compare October 1, 2020 03:30
@ross-spencer ross-spencer force-pushed the dev/issue-18-agents-table branch from 41464d2 to 76d1810 Compare November 6, 2020 13:15
@ross-spencer ross-spencer force-pushed the dev/issue-18-agents-table branch 2 times, most recently from d140e7b to d9d2ed0 Compare December 2, 2020 05:13
@ross-spencer ross-spencer marked this pull request as ready for review December 2, 2020 05:29
@ross-spencer ross-spencer changed the title WIP: Demo Agents work Agents-transfers logging reports Dec 2, 2020
@ross-spencer ross-spencer requested review from tw4l and peterVG December 2, 2020 05:29
Copy link
Contributor

@tw4l tw4l left a comment

Choose a reason for hiding this comment

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

Hi @ross-spencer. This is looking great - nice work! I've left comments where I had questions or think there are some opportunities to tidy bits of the code up a bit, but overall it's already in good shape.

I'm wondering what the Gantt chart will look like for Storage Services that have been in production for months or years. Would the individual ingests still be visible and hoverable? It seems like the built-in plotly toolbar's zoom function would help but I suspect that might point to the need to add support for start/end date parameters in the near future. That said, I think it would be reasonable to do so in a follow-up PR, and as I pointed out offline my next PR will contain some helpers that might make that a bit simpler.

AIPscan/Data/data.py Outdated Show resolved Hide resolved
AIPscan/Reporter/report_ingest_log.py Outdated Show resolved Hide resolved
AIPscan/Reporter/templates/report_ingest_log_tabular.html Outdated Show resolved Hide resolved
AIPscan/Reporter/templates/report_ingest_log_tabular.html Outdated Show resolved Hide resolved
AIPscan/Reporter/templates/report_ingest_log_tabular.html Outdated Show resolved Hide resolved
AIPscan/Data/tests/test_agents_transfers.py Outdated Show resolved Hide resolved
AIPscan/Data/tests/test_agents_transfers.py Outdated Show resolved Hide resolved
AIPscan/Reporter/tests/test_ingest_log.py Show resolved Hide resolved
AIPscan/Reporter/tests/test_ingest_log.py Show resolved Hide resolved
AIPscan/Data/data.py Outdated Show resolved Hide resolved
@ross-spencer ross-spencer requested a review from tw4l December 3, 2020 00:16
@ross-spencer
Copy link
Contributor Author

Thanks Tessa - resubmitted for code review. Also noted offline, we're thinking we can improve the date handling around the plot chart sometime down the line using some of your new handlers. I'm looking forward to review those, and thanks again for the conversation around this.

@ross-spencer ross-spencer force-pushed the dev/issue-18-agents-table branch from 0c69ef3 to ecccc40 Compare December 3, 2020 00:39
AIPscan/Data/data.py Outdated Show resolved Hide resolved
@ross-spencer ross-spencer force-pushed the dev/issue-18-agents-table branch from ecccc40 to 9f7e25a Compare December 3, 2020 01:09
@ross-spencer
Copy link
Contributor Author

ross-spencer commented Dec 3, 2020

@tw4l there are a couple of commits on top of the first push here now - I noticed a broken endpoint (i broke it), and some documentation issues that it felt appropriate to fix. Sorry about that!

@tw4l
Copy link
Contributor

tw4l commented Dec 3, 2020

@ross-spencer cool, thanks! I'm planning to finish up my review tomorrow morning.

@tw4l
Copy link
Contributor

tw4l commented Dec 3, 2020

Hi @ross-spencer - this is looking great and I think we're very nearly there. In addition to my comment above about removing the FIELD_AIP constant (which is preventing some reports from loading properly for me), there are just two areas I think could use a little attention:

  1. Linting is failing for AIPscan/Data/tests/test_agents_transfers.py when I run the tests via tox. I think it just needs black to be run on it.
  2. Thank you for bringing the font-awesome assets in locally! There are a lot more files than I was expecting, and it's making it a bit hard to pick out our own JS and CSS files from the vendor ones. Would you mind seeing if we can group them together, maybe in a vendor/fontawesome subdirectory, or something like that? I recognize that we haven't done that for vendor assets yet, but it seems like this might be a good place to start with that. I'd also encourage seeing if we need all of these assets or whether a subset would suffice, but I'd understand if at this stage you'd rather put that time and effort elsewhere so please don't see that as a hard requirement.

(Edit: Added reference to FIELD_AIP constant issue)

@ross-spencer
Copy link
Contributor Author

Should be good now @tw4l lmk!

@ross-spencer ross-spencer requested a review from tw4l December 4, 2020 16:45
@ross-spencer
Copy link
Contributor Author

EDIT: You got me thinking about dates more and I spotted an issue with the query as a result so there are two other commits. They work like this:

  1. Creates a failure condition in the tests.
  2. Writes the fix to correct that.

Now I'm hoping this PR is looking good! 🤞

@ross-spencer ross-spencer force-pushed the dev/issue-18-agents-table branch from 2f3e65d to 70885f4 Compare December 4, 2020 19:10
Copy link
Contributor

@tw4l tw4l left a comment

Choose a reason for hiding this comment

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

Tests and linting are passing, code looks good, all issues resolved. LGTM! 👍 Good work too on updating the query and tests!

image

@ross-spencer ross-spencer force-pushed the dev/issue-18-agents-table branch from 70885f4 to ab27b95 Compare December 7, 2020 18:59
We include a data endpoint for retrieving information about users and
the transfers that they are responsible for. Using that data we
construct a tabular log of transfers as well as a prototype timeline
that shows the same information in an interactive Plotly Express
chart. These new reports follow some of the new best practices for
testing and templates established by Tessa in previous commits.

Plotly express is introduced here as well as a complete Fontawesome
package for use in future reports.
@ross-spencer ross-spencer force-pushed the dev/issue-18-agents-table branch from ab27b95 to 07d46de Compare December 7, 2020 19:19
@ross-spencer ross-spencer merged commit 07d46de into main Dec 7, 2020
@ross-spencer ross-spencer deleted the dev/issue-18-agents-table branch December 7, 2020 19:56
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.

2 participants