TexasDigitalLibrary / Vireo

Vireo is a turnkey Electronic Thesis and Dissertation (ETD) Management System.
https://texasdigitallibrary.atlassian.net/wiki/spaces/VUG/pages/87490642/About
GNU General Public License v2.0
44 stars 33 forks source link

Issue 1842: Redesign Organization and Triptych to use partial data. #1855

Closed kaladay closed 11 months ago

kaladay commented 11 months ago

resolves #1842

Do not send the full Organization structure to the front-end for the triptych and related functionality to process. Change the behavior to send a `tree view that has a minimal amount of data, essentially removing the recursive nature of the Organization and Workflow Step structure. This allows for incredibly fast responses.

This works great for the New Submission view. The Organization Settings Admin view, however, is another matter.

The Triptych stores the organization structure and selects the Organizations for editing via the Triptych's Organization tree structure (the parent-child properties). These are loaded via the tree view and do not have the required data. A new view, called shallow is created in order to load the necessary information while not loading the entire Organization. The shallow load of an Organization is not as performant as I would like it to be. Therefore, only load the shallow when necessary (which happens to be whenever the Organization is selected).

Once an Organization is selected and the shallow is loaded, subsequent selects end up causing a new load. To prevent this, caching of the just loaded shallow Organization is performed.

Now the Organization needs to be designated as dirty so that any updates will trigger an appropriate reload.

Java Side / Back End Changes

Add New end points:

  1. /all/{specific}, where specific is any supported string (currently only tree and shallow). This loads all organizations for the given specific view.
  2. /get/{id}/{specific}, where specific is any supported string (currently only tree and shallow). This loads a single organization for the given specific view.

Add a comment to isComplete() method as this is now more relevant than before.

The appropriate end points are added in the repo to facilitate the described behavior.

The model views are broken down further into two new types:

  1. SimpleModelView: Only has one property, the id.
  2. SimpleNameModelView: Only has two properties, the id and 'name'.

The model views are then updated to utilize these views as appropriate.

The following model views are then added:

  1. TreeOrganizationView: A minimal Organization structure designed to provide the entire Organization tree.
  2. ShallowOrganizationView: An expanded version of TreeOrganizationView designed to be editable via the Organization Settings Admin view.
  3. SimpleWorkflowStepView: A minimal Workflow Step view designed to reduce the performance cost by only loading the required properties and is designed to be editable via the Organization Settings Admin view..

The SimpleVocabularyWordView is not used any more and so it is removed.

UI: API Mapping Changes

The Field Profile and the Organization must now be lazy loaded. They are causing a performance problem and are often loaded when the loaded data is not even being used.

UI: OrganizationRepo.getAll()

The OrganizationRepo.getAll() is being called in numerous controllers when that data is not going to be used any more. The getAll() is built into weaver-ui-core and must not be used when the goal here is to load a tree view or a shallow view. This is also a major performance bottleneck. The data loaded by this is also unavailable for direct manipulation and therefore cannot be used.

Get rid of all of these calls and implement the required behavior to perform the tree and shallow loads using the newly added end point.

UI: Numerous views call things like getSelectedOrganization().acceptsSubmissions or getSelectedOrganization().name

This is not save in that if nothing is selected then there is an attempt to act on a property of NULL. This is an error that becomes more fully exposed now that the data is being more dynamically loaded than before. Add several functions that check the existence of the object in place of these (returning NULL otherwise).

The new functions are named using obvious names, such as changing getSelectedOrganization().name into getSelectedOrganizationName().

UI: Setting the Selected Organization

The mouse can be clicked while selecting, so add a loadingOrganization to prevent bad behavior that corrupt the local data.

When selecting the Organization, check to see if the Organization is already cached locally, is only loaded as tree in the cache, or the cached version isdirty and needs to be updated. In these cases, perform the shallow load.

Of particular note is that the loaded Organizations must all reference the new shallow Organization where applicable. Walk through the entire tree structure and update the existing Organization with the now downloaded shallow version.

UI: Setting the Selected Organization, when Dirty

This has a notable performance hit and could use further optimization by breaking down the logic in cases when this is dirty. This change set is big enough and so more granular improvements are skipped at this time.

A quick solution might be to walk through the local cache and perform the appropriate updates (if possible) on the local cached data. Otherwise, do a shallow load, which is expensive (but still cheaper than loading the full Organization model).

UI: Ready or Not

The OrganizationRepo.ready(0 calls are not needed anymore as the full Organization list is not being loaded. Instead, set ready when the Organization tree array is loaded.

When the Organization tree array is fully loaded, then select the first Organization as the selected Organization by default.

Each Organization in the tree array must be a proper Organization model object. Walk through the entire tree array, including parents and children, instantiating each Organization as a proper Organization object. Take care of designating that this is a tree view loaded Organization and is neither a shallow nor a complete Organization.

The rebuilding of the Organization to instantiate each Organization in the tree array must be made available so that the Triptych can trigger the rebuild when needed. This includes when the Organization gets a BROADCAST, such as when an Organization is added or deleted.

UI: A Lot is Tied to the Triptych

The Triptych is loaded by two Controllers:

  1. NewSubmissionController
  2. OrganizationSettingsController

The Triptych requires functions and properties to be defined in each of these. Then the Triptych will include other Controllers that then make similar assumptions.

The Triptych child Controllers:

  1. emailWorkflowRulesController
  2. fieldProfileManagementController
  3. noteManagementController
  4. organizationManagementController
  5. organizationSideBarController

Each of these has its own nuance and special behavior that needs to be appropriately updated and handled. Each of these has its own view that must be updated as appropriate.

Changing any part of the Triptych affects all of these.

The Triptych is tied to those two Controllers above as well as the OrganizationRepo. Any changes on these can force changes on all of the child Controllers.

Using reduced data, such as the tree view in an Organization therefore affects all of these Controllers and their (numerous) related views.

Each of the child view needs to take special care to set the selected Organization to be dirty on any change they perform. Each of the child view should no longer load the full Organization repository.

The organizationSideBarController in particular keeps its own copy of the Organization data and needs to perform its own load of the Organization tree view. Fortunately, this is an inexpensive operation.

UI: The Triptych

Add a repoListenLock to prevent running the rebuild process too much (and therefore breaking things). The timeout already exists, so just add a lock that unlocks once the timeout resolves and the task is resolved. The timeout is greatly reduce to improve the re-load response time. The lock will prevent most of the problems and should be better than the timeout.

The Triptych must recursively walk through the Organization tree until it finds the desired Organization. A more centralized Organization design could allow for a cache where each Organization has a key mapped by the id at the top level to avoid the need for such complex searches. This could then be further optimized to select from this cache rather than from the tree so that the tree need not be recursively updated (except on tree structural changes like adding and removing an Organization). This is omitted due to additional complexity of such a change.

UI: The Organization

The Organization has a several repository-type methods on the model. This is inefficient as these methods are copied for every Organization, which has a processing cost (which is now shown to be a major performance bottleneck in general). Re-locate these into the OrganizationRepo, accepting the appropriate Organization as a parameter.

This further fixes problems where the children Contollers may need information from the parent Controllers that is not necessarily available.

UI: The Organization Repository

Expose the selected Organization so that one Controller in particular that requires this data but does not have it on its scope can access.

Add the getAllSpecific() method to handle fetching the appropriate Organization tree from the back end with the appropriate specific view mode.

For compatibility with other existing behavior, the getSelectedOrganization() method will call findById() as it did before. This data is neither used or loaded and should return nothing in the Triptych use cases. When the findById() returns nothing, then attempt to get using the explicitly saved selected Organization (selectedOrganization).

Update several end points to use the selectedId and not risk access on a NULL property.

As mentioned above in the "The Organization" section, add the repository specific methods pulled off of the Organization model.

UI: Omitted Designs - Centralized Organization

There are additional changes that I experimented with. One of which is using the OrganizationRepo as the one source of the loaded Organization array. This worked well but involved a lot more changes. To avoid adding even more code changes, I omitted this change.

Using a centralized Organization would be a design improvement in that all of the Organization repository loading data is in one place. This would further reduce duplicate calls, such as how the organizationSideBarController currently works.

UI: More Optimization is Still Needed

The performance problems are greatly reduced but the Organization Settings Admin View still loads slower than desirable for the individual (shallow) Organization data.

As suggested above, a more granular solution should be considered.

Editing the properties on an Organization is now slower due to the need to refresh the data due to a lack of more granular local cache management.

UI: Unresolved Problem, Console Errors in Organization Settings Admin View

There are some errors that appear in the console log but do not have any functional impact. Investigating these will take some time that I did not want to delay this change set further by.

I believe these are caused by data being accessed and the data is not fully loaded. The problem is that the errors do not provide sufficient details on exactly where the problem happens.

The errors are like this:

Failed to set an indexed property on 'Window': Indexed property setter is not supported.

The error comes from weaver-ui-core not safely handling bad data. The solution should be to just fix the bad data from happening.

UI: Existing Problems - Locked Text Fields Show Nothing

This is a notable existing problem and is not caused by these changes.

I have observed this on several sprints after testing for them. I thought this was fixed so maybe there was a merge error that caused the fix to get lost or otherwise not committed.

Unit Tests for both Back End and Front End (UI)

~This commit does not include the appropriate unit tests given the size of the change set.~

~I will follow up this commit with some unit tests at some point.~

Unit tests are now added, including coverage on much of the newly added code.