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

Issue 1930: Add custom filter for embargo type by list. #1935

Merged
merged 3 commits into from
May 10, 2024

Conversation

kaladay
Copy link
Contributor

@kaladay kaladay commented May 8, 2024

resolves #1930

There is existing (dead) code for this already.
Re-vitalize the (dead) code, clean it up, and get it working.

The embargoTypes.name SQL in the back-end operates differently in that it is not involved in part of a column name and is exclusively for filtering. This changes the nature of the query and I opted to go with sub-queries for selecting the IDs because I can design it this way and avoid LEFT JOIN (or INNER JOIN). This has not seen extensive performance testing but this new SQL query conditions should be run through some performance testing.

I went with Embargo Type for the new custom filter because the existing code used that name.

A custom filter uniqueEmbargoType is created because the unique/uniq angularjs is not available. Using unique or uniq yields an error like this:

Unknown provider: uniqueFilterProvider <- uniqueFilter

Using the custom filter (uniqueEmbargoType) avoids having to pass the additional | filter:{isActive: true} and similar. This can be done and is done within the same filter.

This adds a unit test for the filter (AngularJS context of filter) for the Embargo Type filter (Vireo table list context of filter).

The addition of the Embargo Type causes a test in the SystemDataLoaderTest class to fail due to a hard-coded value representing all of the default filters. This has been updated to include the new filter in the total.

The UI has an existing filter view, and that filter view is now slightly tweaked to address problems in the user experience. I opted to make its design more closely match that of the Status filter (which goes in line with the issue request using Status filter as an example).

This PR has an escaped issue discovered and fixed in #1936.
There is overlapping changes and so if PR #1936 is not to be merged then the SQL escape fixes need to be cherry-picked out from that PR and added to this PR.

The following SQL may be needed to updating existing repositories when not using the ddl-auto to update.

INSERT INTO submission_list_column (id,title,input_type_id) VALUES (DEFAULT,'Embargo Type', 1);
INSERT INTO submission_list_column_value_path (submission_list_column_id,value_path,value_path_order) VALUES (currval('submission_list_column_id_seq'),'embargoTypes',0)
INSERT INTO submission_list_column_value_path (submission_list_column_id,value_path,value_path_order) VALUES (currval('submission_list_column_id_seq'),'name',1)

There is existing (dead) code for this already.
Re-vitalize the dead code, clean it up, and get it working.

The `embargoTypes.name` SQL in the back-end operates differently in that it is not involved in part of a column name and is exclusively for filtering.
This changes the nature of the query and I opted to go with sub-queries for selecting the IDs because I can design it this way and avoid `LEFT JOIN` (or `INNER JOIN`).
This has not seen extensive performance testing but this new SQL query conditions should be run through some performance testing.

I went with `Embargo Type` for the new custom filter because the existing code used that name.

A custom filter `uniqueEmbargoType` is created because the `unique`/`uniq` angularjs is not available.
Using `unique` or `uniq` yields an error like this:
```
Unknown provider: uniqueFilterProvider <- uniqueFilter
```

Using the custom filter (`uniqueEmbargoType`) avoids having to pass the additional ` | filter:{isActive: true}` and similar.
This can be done and is done within the same filter.

There are no existing filter unit tests to base off of.
I may follow up with a commit that adds filter unit tests after reviewing other angularjs projects where we have implemented a filter unit test.

The addition of the `Embargo Type` causes a test in the `SystemDataLoaderTest` class to fail due to a hard-coded value representing all of the default filters.
This has been updated to include the new filter in the total.

The UI has an existing filter view, but it was slightly tweak that filter view to address problems in the user experience.
I opted to make its design more closely match that of the Status filter (which goes in line with the issue request using Status filter as an example).
This adds the filter (in the context of AngularJS) for the Embargo Type.
Copy link
Contributor

@smutniak smutniak left a comment

Choose a reason for hiding this comment

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

it appears neutral as it didn't break anything but i won't really be able to test until PR 1936 so i'll approve.

@smutniak smutniak merged commit afeb066 into TexasDigitalLibrary:main May 10, 2024
1 check failed
@kaladay kaladay deleted the 54-embargo_type_filter branch May 13, 2024 20:36
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.

Embargo Filters require unassisted text entry instead of a list like vireo3, or typeahead
2 participants