Usage Graphs refactor implementation #128
Open
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
|
Does not account for a more recent refactor (#131) of the Aspen API usage graph - needs to be updated |
these methods contain section-specific code. While they likely could be refactored to take in arrays and process these programmatically, allowing some of the logic to be abstracted, this goes beyond the scope of this initial refactor which only seeks to abstract the methods that are shared amongst all usage graphs classes, and for which the implementations are strongly similar.
the launch() method must be implemented on children of Admin_Admin, taking no parameter, and returning void. Since its logic is the same for every usagegraph class, with the exception of the section name, I extracted it into UsageGraphs_UsageGraphs::launchGraph($section), leaving launch() the responsibility to assign the section name.
since the permissions required to view admin usage graphs are the same no matter the section, canView() can be moved to the abstract UsageGraphs_UsageGraphs class to be used as is by all usagegraph classes
The buildCSV() method is shared amongst all usagegraphs classes and can therefore be added to the abstract UsageGraphs_UsageGraphs class, provided that it is amended to take in a string as a parameter with the name of the section (eg. 'Admin')
Certain dashboards (SideLoads and Open Archives come to mind) are made up of various subsections for each of which the datastructure is repeated. These subsections can correspond to collection, profiles, etc. So that users are fully aware of what data they are looking at, this update the launchGraph() and buildCSV() methods to ensure that the name of these subSection is taken into account.
Chloe070196
force-pushed
the
usagegraph_refactor_implementation
branch
from
04690dc to
1efe8b4
Compare
Chloe070196
changed the base branch from
24.10.00_additional_usage_graphs
to
usagegraphs_refactor_implementation
|
Rebased against 25.01.00 and updated to account for #131 |
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.
This patch implements the changes proposed in #120 so that all existing usage graphs:
The following sections are affected:
This also updates the Admin_AbstractUsageGraphs class as its methods needs to be applicable to graphs from dashboards that are divided into subsections (those might correspond to collection, profiles, etc).
Before testing, ensure the following tables contain data:
- aspen_usage
- user_ils_usage
- user_summon_usage
- ils_record_usage
- user_axis360_usage
- axis360_record_usage
- and axis360_stats
- user_sideload_usage table
- sideload_record_usage table
- api_usage
- materials_request_usage
- materials_request_status
Additionally, in Aspen Administration > System Administration > Modules, enable the following modules:
- Summon
- Axis360
Lastly, ensure that in Aspen Administration > System Administration > Permissions > Materials Requests, the admin user you are logged in as has the 'View Materials Requests Reports' permission.
This patch does not add any new features as such, and should not result in any changes in behaviour. The one exception is graph titles and csv names which may be more specific, as dashboard sections are now taken into account where relevant.
The test plan is almost the same as for #116, with the intent to check for unwanted changes in behaviour:
For each section: