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

4.2.2 staging #1809

Merged
merged 30 commits into from
Jun 22, 2023
Merged

4.2.2 staging #1809

merged 30 commits into from
Jun 22, 2023

Conversation

kaladay
Copy link
Contributor

@kaladay kaladay commented Jun 20, 2023

No description provided.

cstarcher and others added 30 commits June 7, 2023 13:06
…ltiple modules.

This is the error Eclipse is showing:
```
The package "javax.xml.parsers" is accessible from more than one module: <unnamed>, java.xml
```

This is not helpful, as `unnamed` is rather useless for debugging.

Used this command to identify where the `javax.xml.parsers` is being brought in.
```
mvn dependency:copy-dependencies -DincludeScope=test -DoutputDirectory=deps
```
Did a search inside the `deps/` directory for the `javax.xml.parsers` inclusion.
This showed that the `xml-apis` is bringing in a duplicate and older dependency.
Excluding `xom` and manually including it allows for excluding the `xml-apis`.
The mvnrepository.com shows that `xom` moved from https://mvnrepository.com/artifact/xom/xom to https://mvnrepository.com/artifact/com.io7m.xom/xom .

The `xom` dependency change happened here: 018e460 for issue #1772.
This takes a simple approach by just hiding the `ADVISOR` type from the create/edit modal for e-mails.
This has a side-effect of causing the built-in "SYSTEM Initial Submission" from displaying the selection as that appears to select `ADVISOR` by default.
The "SYSTEM Advisor Review Request" is unaffected because it assigns `Committee Chair` by default.

The previous commit ab8238d revealed that simply removing exposes a large amount of hard-coded behavior.
This approach is abandoned given the amount of hard-coded references and uses of `ADVISOR` and the specific fix strategy being requested.
The mentioned commit reveals some of the problems such as the system templates, like "SYSTEM Initial Submission", stop appearing.

There are numerous concerns about the hard-coded behavior around `ADVISOR`.
The following are two major areas to pay attention to:
- https://github.com/TexasDigitalLibrary/Vireo/blob/3bf83e24cddf228dcf032e47410a2155a8d9f902/src/main/webapp/app/controllers/submission/adminSubmissionViewController.js#L121
- https://github.com/TexasDigitalLibrary/Vireo/blob/3bf83e24cddf228dcf032e47410a2155a8d9f902/src/main/webapp/app/controllers/submission/adminSubmissionViewController.js#L405
…_not_sending

Issue 1666: Remove hard-coded Advisor from organization e-mail list.
…LECT in VocabularyWord.

The `ControlledVocabulary` is switched to `LAZY` and sets the batch size to 1000 for the dictionary property.
The increased batch size should reduce the amount of queries for larger data sets.
The `LAZY` is set to not immediately load the dictionary.

A major performance problem is within the `VocabularyWord` Entity where all contacts are loaded.
There may be as many as 20,000 contacts.
Loading every single one as a fully populated Entity Object (and then one at a time) is very costly.
Reduce the impact of this by using a `SUBSELECT`.

Furthermore, use `LAZY` in more properties on the `VocabularyWord` Entity.

This approach does not solve the problem.
Instead, it reduces the impact.
For a set of maybe 17,000 contacts, the loading of a single organization may take up to 22 seconds.
After this change it may take up to 4 seconds.
This is still too long but is far better.

The problem is in how the Entity is being loaded.
Other strategies need to be developed (such as using Projection).
Such strategies are not performed within this commit.
This is affecting the **Choose Your Organization** page.

Too much unnecessary data is being loaded off of the specific Submission end point `/all-by-user`.
This end point is only used for the main page that then leads to the **Choose Your Organization** page.

This reduces the loaded data to only the minimum needed on the Student Submission List View.
The observed difference in performance is ~800 milliseconds prior to this change and ~200 milliseconds after this change (for vocabulary words with ~17,000 contacts).
Also note that Without the code from commit a2e872a, the prior performance is ~22 seconds (for vocabulary words with ~17,000 contacts).
The commit a2e872a introduced changes that breaks tests.
A transaction is now needed.

Also fix comment from other transactional test.
The Assignee must have settings so that the "Assigned To" can be properly displayed on the student submission history page.

This doesn't seem necessary for the Submitter.
The Submitter will not have settings loaded to avoid loading anything extra for performance reasons.
This actually only needs to be values for two predicates:
1) `dc.title`
2) `_doctype_primary`

The problem is the current design does not make this easy.
Rather than taking a more complicated approach, suffer the performance cost and load all field values for the submissions.
In the particular data set that I am using (a copy of production data), the request goes from ~200ms to ~300ms with this change.
I consider this an acceptable loss for the time being.
Future designs should be applied to avoid this and load only the two values for the specified predicates.
…atch_and_subselect

Issue 1756: Increase batch size in ControlledVocabulary and use SUBSELECT in VocabularyWord.
…ubmission_projection

Issue 1756: Use Projection for specific Submission usage.
Must now add `@Transactional`.

I moved the relevant code blocks into their own functions and added `@Transactional`.
This turned out not to work.
The `@Transactional` must be on the `public void run()` function.

I left the separate functions as-is because it is cleaner code.

The expansive is taking a bit long sometimes so I lowered the action log max and provided a second parameter to make it configurable.
```
  generate 5 20
```
Would Generate 5 submissions with a max of 20 action logs randomly generated when in expansive mode.

Improve some messages.

Simplify some of the arguments, moving them out of the switch statement (making the coder slightly cleaner).
…on-select_using_extend

Issue 1804: Reset deposit location and use angular.extend() in scope.selectDepositLocation().
This is a work in progress commit.

The problem is that the model listener is on a per individual filter basis.
The entire set for both saved filters and active filters for a given user needs to be handled as a whole.

Remove or disable the model listener, replace the old end points, and add new broadcasts and listeners that operate on a set basis rather than on individual basis.
…set pagination.

The changes come in from the broadcast.
The recipient of the broadcast doesn't necessarily know what the broadcast is for.
Provide additional details about the broadcast so that the list can be more appropriately queried and built.
A new enumeration is provided to facilitate this.
Only the most basic parts of the enumeration are created.

Prevent the double-loading of the query by removing the direct calls to refresh the query.
Instead, let the broadcast listener handle the calling of the query.
Add `/user` and the appropriate user ID to ensure that updates for the current user are processed.
…see it.

Only two actions are needed here:
1. `SAVE`
2. `REMOVE`

All users who can see a public filter should have their saved filter list up date when a public filter is added or removed.

The `SAVE` will only refresh the list if the user is different from the user who created the filter.
The user who created the filter will already be up to date due to the other broadcasts.

The `SAVE` also needs to handle the case where the owning user changes the filter from public to private or vice versa.

The `REMOVE` needs special handling because the object is removed.
Send only the ID.
Future iterations may want to also pass the user ID to allow for detecting if the current user is the one who deleted the filter.
Currently, passing both the Filter ID and the User ID results in overlap because they both get serialized into a key named `Long` and as a result one overwrites the other.
…ional.

The `@Transactional` does not consistently work when run under the Cli Component.
Sometimes the Submissions appear but most often times they do not.

Move the code into a separate class file with `@Transactional` and `@Service` and then calling those methods appears to produce the expected behavior.
…li_and_lazy_load

Issue 1756: CLI Generation broke after switching to LAZY initialization.
…ould_be_method

Fix spelling mistake where 'metod' should be 'method'.
…-after_xom

Change xom dependency to address javax.xml.parsers is accessible in multiple modules.
…refreshing

Issue 1795: Redesign filter broadcasting logic.
The use of `ManagedConfigurationRepo.getAll()` and `StudentSubmissionRepo.getAll()` is incorrect here.

The `getAll()` methods are being called already a few lines up.
This is likely causing a double load and is likely affecting performance.

Use `ready()` instead.
…eady

Issue 1756: Should use ready() rather than getAll().
…d filter.

The filter should not be altered.
This hot fix takes a quick approach of just always assigning the "persisted" filter as the active filter.

When this filter gets modified, the "persisted" filter now is what gets modified rather than the saved filter.

For future notes and investigations consider that this solution also suggests that the "active filter" concept can be thrown away and the "persisted filter" could always be the active filter.
…_filter_still_being_used

Hotfix: Making changes after selecting a saved filter alters the saved filter.
@smutniak smutniak merged commit 6181bf0 into main Jun 22, 2023
@kaladay kaladay deleted the 4.2.2_staging branch July 11, 2023 15:06
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.

3 participants