fix: move API usage files to API folders #131
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Chloe070196
changed the base branch from
24.10.00_additional_usage_graphs
to
refactor_API_usagegraphs
|
Awaiting opportunity to test fully - unsure how to get other API than SystemAPI to show on the API Usage Dashboard under System Reports |
Keeping the dashboard and usage graph service files for APIs in the Admin folder would have created the need for additional parameters and control flow logic in the exportUsageData method which both Admin usage data CSV export and API usage data CSV export relied on. Additionally, the resulting class names do not follow the same naming conventions (<serviceName>_Dashboard and <serviceName>_UsageGraphs) as the rest of Aspen. This refactor provides a separation of concerns by replacing services/Admin/APIUsageDashboard.php and services/Admin/APIUsageGraphs.php with services/API/Dashboard.php and services/API/UsageGraphs. In doing so, it also fixes an issue where only the csv file for 'runPendingDatabaseUpdates' could be downloaded. Test plan: 1) Log in to Aspen as an Admin 2) In Aspen Administration > System Reports, open API Usage Dashboard. Notice the title of the links match their section and header. 3) For each API and for each of their stats, open their usage graph page. Notice that the data matches the data on the dashboard, both in the raw data table and on the graph. Also notice that the tilte of the graph match its section and header in the dashboard. 4) Click 'Export to CSV' (bottom left of the page) to check that the downloaded file's contents match those of the raw data table and that its name includes the stat name. Run through this plan before applying the patch, and notice that the CSV export only works for 'runPendingDatabaseUpdates'. After the patch, it is expected to work for any usage graph.
Chloe070196
force-pushed
the
refactor_API_usagegraphs
branch
from
5ae8dbe to
1d4edb6
Compare
Chloe070196
changed the base branch from
refactor_API_usagegraphs
to
where_is_it_button_renders_consistently
Chloe070196
changed the base branch from
where_is_it_button_renders_consistently
to
24.10.00_refactor_API_usagegraphs
ammopt
merged commit 37c0949
into
PTFS-Europe:24.10.00_refactor_API_usagegraphs
3 checks passed
|
Good fix. Test plan works as advertised. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BUG (in 24.10.00): In #126, for Aspen Administration > System Reports > API Usage Dashboards, only the 'runPendingDatabaseUpdates' data could be successfully exported as a CSV due to a hard-coded bit of control flow in
services/Admin/AJAX.php.Keeping the dashboard and usage graph service files for APIs in the Admin folder would have created the need for additional parameters and control flow logic in the exportUsageData method which both Admin usage data CSV export and API usage data CSV export relied on. Additionally, their class names did not follow the same naming conventions (_Dashboard and_UsageGraphs) as the other usage dashboards and graphs classes now tend to do in Aspen.
This refactor provides some additional separation of concerns by replacing
services/Admin/APIUsageDashboard.phpandservices/Admin/APIUsageGraphs.phpwithservices/API/Dashboard.phpandservices/API/UsageGraphs, thereby fixing the csv export issue.Expected behaviour post-patch:
/Admin/APIUsageDashboardto/API/UsageDashboardand/Admin/APIUsageGraphsto/API/UsageGraphsTest plan:
ensure that api_usage is populated by more than one module and more than one method. Out of the box adb only tends to show 'runPendingDatabaseUpdates' for the 'SystemAPI' module. If not, insert additional mock data (e.g INSERT INTO api_usage values (2,2024,8,'2024-8','UserAPI', 'updateNotificationOnboardingStatus', 12);)
Log in to Aspen as an Admin
In Aspen Administration > System Reports, open API Usage Dashboard. Notice the title of the links match their section and header.
For each API and for each of their stats, open their usage graph page. Notice that the data matches the data on the dashboard, both in the raw data table and on the graph. Also notice that the title of the graph match its section and header in the dashboard.
Click 'Export to CSV' (bottom left of the page) to check that the downloaded file's contents match
those of the raw data table and that its name includes the stat name.
To replicate the bug, follow this test plan on a 24.10.00-based branch without applying the patch. After applying, navigate back to the Aspen Administration menu page and access the API Usage Dashboard from the link there (notice the change in url).