Content reports ported from DSpace 6.x #1985

jeffmorin · 2022-12-02T19:25:22Z

References

Fixes [DS-4301] Ensure that DSpace REST Report capabilities are part of the DSpace 8 REST API DSpace#7641
Related to [DS-4301] Added Content Reports section and Filtered Collections report therein RestContract#202
Requires using backend PR: Content reports ported from DSpace 6.x DSpace#8598

Description

This pull request includes an initial version of the two content reports ported from DSpace 6.x.

Instructions for Reviewers

This pull request adds a Content Reports section in the Administration menu to provide access to both reports from the Angular web application.

After logging in using an account with administrator permissions, select the Report item from the Administration menu at the left of the page, then select one of the two entries in this section.

In the Filtered Items report, the Export Metadata functionality is not yet implemented, it will be part of another pull request, once this one is all settled and merged.

Please note:

I am a beginner when it comes to anything related to Angular. I will need proper guidance to apply any requested change.
In the component classes (filtered-collections-component.ts and filtered-items-component.ts), I use a DSpaceRestService instance to send my requests to the REST layer. It may be better to create and use a specific data service similar to CollectionDataService, but I will need help to do so if requested.
I wrote a few unit tests for my components, but even after several days I didn't succeed in making my tests in filtered-items-component.spec.ts run properly. I will definitely need help with this.

Checklist

My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
My PR passes TSLint validation using yarn run lint [Not sure]
My PR doesn't introduce circular dependencies
My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods. [Not sure]
My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide. [Not yer, please see above]
If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

github-actions · 2022-12-09T17:33:36Z

Hi @jeffmorin,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

jeffmorin · 2022-12-15T18:08:04Z

I rebased my branch and fixed the merge conflict.

src/app/admin/admin-reports/filtered-collections/filtered-collections.component.html

+                                <tbody>
+                                    <tr *ngFor="let coll of results.collections">
+                                        <td><a href="{{ coll.communityHandle }}" target="_blank">{{ coll.communityLabel }}</a></td>
+                                        <td><a href="{{ coll.handle }}" target="_blank">{{ coll.label }}</a></td>


src/app/admin/admin-reports/filtered-collections/filtered-collections.component.html

+                                </thead>
+                                <tbody>
+                                    <tr *ngFor="let coll of results.collections">
+                                        <td><a href="{{ coll.communityHandle }}" target="_blank">{{ coll.communityLabel }}</a></td>


tdonohue

@jeffmorin : Good start here. I will admit, I have not done a thorough code review here yet, but I called out a few things that are failing in our GitHub CI checks & a few minor things to cleanup.

I did test this, and overall it seems to work. I'll admit though that I find the interface confusing to interact with (but it may be simply because I'm not as experienced with the older DSpace 6.x tool -- as I see this interface looks similar to that tool). I did find a few bugs though.

First, I noticed that, in the "Collection Filter Report", when you select a large number of filters (e.g. all of them), the results are very hard to read. Here's how they used to look in DSpace 6.x

However, here's how it looks in this PR:

I find this table view very difficult to read for a few reasons:

The columns have no borders, making it hard to differentiate where one column ends and the other begins.
The column headers are really long and wrap heavily (not sure how to get around this though)
If you select more headers than can fit on a page, there's no way to scroll to the right. You'll notice the right-most column in the screenshot above has it's header cut off. There's no way to scroll right to see the rest of it, and all the other columns.
There doesn't appear to be any way to paginate the results? Maybe I just don't have enough collections yet though... but I was surprised it shows everything on one page. That may not provide great performance if you have hundreds of collections.

A few other bugs:

The links in this table are all broken. They appear to be trying to go to the handle, but the URL is incorrect. They probably should just link to the UUID of each object.
When no results are returned on this page, the view looks confusing.

The same sorts of issues also exist in the Metadata Query Report. The tables are difficult to read as there's no column borders. I cannot scroll to the right, and pagination doesn't appear to work (at least for me it's always showing everything on page 1).

Overall, it is a good start...and it does behave similar to these tools in DSpace 6.x. I just think the UI needs some cleanup to make it easier to understand the results you are seeing.

Some more inline comments appear below regarding small things to clean up.

tdonohue · 2022-12-16T20:15:01Z

src/app/admin/admin-reports/filtered-collections/filtered-collections.component.html

+                                </thead>
+                                <tbody>
+                                    <tr *ngFor="let coll of results.collections">
+                                        <td><a href="{{ coll.communityHandle }}" target="_blank">{{ coll.communityLabel }}</a></td>


There's a security alert on this line because you are using target="_blank" without also specifying this attribute: rel="noopener noreferrer" You should either add that attribute, or remove the target from this link. For more info, see https://cwe.mitre.org/data/definitions/1022.html

tdonohue · 2022-12-16T20:15:16Z

src/app/admin/admin-reports/filtered-collections/filtered-collections.component.html

+                                <tbody>
+                                    <tr *ngFor="let coll of results.collections">
+                                        <td><a href="{{ coll.communityHandle }}" target="_blank">{{ coll.communityLabel }}</a></td>
+                                        <td><a href="{{ coll.handle }}" target="_blank">{{ coll.label }}</a></td>


This line has the same security alert as the one above

tdonohue · 2022-12-16T20:16:28Z

src/app/admin/admin-reports/filtered-items/filtered-items-model.ts

@@ -0,0 +1,23 @@
+import { Item } from "src/app/core/shared/item.model";


This line has a lint failure (run yarn lint locally to see it). We require single quotes in all import statements. So, change this to:

import { Item } from 'src/app/core/shared/item.model';

tdonohue · 2022-12-16T20:17:06Z

src/app/admin/admin-reports/filtered-items/filtered-items.component.spec.ts

@@ -0,0 +1,108 @@
+import { waitForAsync, ComponentFixture, TestBed} from '@angular/core/testing';
+import { NO_ERRORS_SCHEMA } from '@angular/core';
+import { TranslateLoader, TranslateModule, TranslateService } from '@ngx-translate/core';


This line has lint errors because you don't use TranslateService in this spec. It can be removed from the import.

tdonohue · 2022-12-16T20:18:26Z

src/app/admin/admin-reports/filtered-items/filtered-items.component.spec.ts

+import { RawRestResponse } from 'src/app/core/dspace-rest/raw-rest-response.model';
+import { CommunityDataService } from 'src/app/core/data/community-data.service';
+import { ObjectCacheService } from 'src/app/core/cache/object-cache.service';
+import { ActionsSubject, ReducerManager, ReducerManagerDispatcher, StateObservable, Store, StoreModule } from '@ngrx/store';


This line has lint errors because you don't use ActionsSubject, ReducerManager, ReducerManagerDispatcher, StateObservable or Store in this spec. They should all be removed from this import

tdonohue · 2022-12-16T20:18:52Z

src/app/admin/admin-reports/filtered-items/filtered-items.component.spec.ts

+import { CollectionDataService } from 'src/app/core/data/collection-data.service';
+import { MetadataSchemaDataService } from 'src/app/core/data/metadata-schema-data.service';
+import { MetadataFieldDataService } from 'src/app/core/data/metadata-field-data.service';
+import { StoreMock } from 'src/app/shared/testing/store.mock';


StoreMock is also not used in this file. So it should be removed.

tdonohue · 2022-12-16T20:19:38Z

src/app/admin/admin-reports/filtered-items/filtered-items.component.ts

+  pageLimits: OptionVO[];
+
+  queryForm: FormGroup;
+  currentPage: number = 0;


Because this is a simple integer, you can remove the type here. It should just be currentPage = 0; Again, this error can be seen via yarn lint

tdonohue · 2022-12-16T20:21:04Z

src/app/admin/admin-reports/filtered-items/filtered-items.component.ts

+      query.predicates
+          .map(qp => qp.toFormGroup(this.formBuilder))
+          .forEach(qp => this.addQueryPredicate(qp));
+      if (query.predicates.length == 0) {


This should be a triple equals (===) instead of ==. See https://howtodoinjava.com/typescript/equals-vs-strict-equals/

tdonohue · 2022-12-16T20:27:42Z

src/assets/i18n/ar.json5

+
+  //"admin.reports.collections.title": "Collection Filter Report",


I've noticed that most of this PR is simply updates to every one of our language packs. You don't need to make these changes to every single language pack. You can just update en.json5 and that's OK (though you are welcome to edit others if you want). I just wanted to point this out though because it makes this PR look a lot larger than it really is & that might intimidate other code reviewers.

tdonohue · 2022-12-16T20:29:40Z

src/app/admin/admin-reports/filtered-collections/filtered-collection.model.ts

@@ -0,0 +1,36 @@
+export class FilteredCollection {


Similar to Java, we require TypeDocs (similar to JavaDocs) to be added to all exported/public classes. If you could add them to all the new classes in this PR, that'd be appreciated (they don't need to be added to HTML or SCSS files...just the TS module files).

github-actions · 2022-12-21T16:09:50Z

Hi @jeffmorin,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

tdonohue assigned jeffmorin Dec 2, 2022

tdonohue added new feature component: administrative tools Related to the admin menu or tools labels Dec 2, 2022

tdonohue mentioned this pull request Dec 7, 2022

Content reports ported from DSpace 6.x DSpace/DSpace#8598

Merged

6 tasks

github-actions bot added the merge conflict label Dec 9, 2022

tdonohue self-requested a review December 15, 2022 15:26

jeffmorin closed this Dec 15, 2022

jeffmorin force-pushed the main branch from 2a00548 to ed593d5 Compare December 15, 2022 17:48

jeffmorin reopened this Dec 15, 2022

github-actions bot removed the merge conflict label Dec 15, 2022

github-advanced-security bot found potential problems Dec 15, 2022

View reviewed changes

tdonohue requested changes Dec 16, 2022

View reviewed changes

github-actions bot added the merge conflict label Dec 21, 2022

jeffmorin closed this Jan 9, 2023

jeffmorin force-pushed the main branch from 1d1c713 to 95b8dc3 Compare January 9, 2023 14:26

jeffmorin mentioned this pull request Mar 24, 2023

Content reports ported from DSpace 6.x #2163

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Content reports ported from DSpace 6.x #1985

Content reports ported from DSpace 6.x #1985

jeffmorin commented Dec 2, 2022 •

edited by tdonohue

Loading

github-actions bot commented Dec 9, 2022

jeffmorin commented Dec 15, 2022

tdonohue left a comment •

edited

Loading

tdonohue Dec 16, 2022

tdonohue Dec 16, 2022

tdonohue Dec 16, 2022

tdonohue Dec 16, 2022

tdonohue Dec 16, 2022

tdonohue Dec 16, 2022

tdonohue Dec 16, 2022

tdonohue Dec 16, 2022

tdonohue Dec 16, 2022 •

edited

Loading

tdonohue Dec 16, 2022

github-actions bot commented Dec 21, 2022

		@@ -0,0 +1,23 @@
		import { Item } from "src/app/core/shared/item.model";


		//"admin.reports.collections.title": "Collection Filter Report",

Content reports ported from DSpace 6.x #1985

Content reports ported from DSpace 6.x #1985

Conversation

jeffmorin commented Dec 2, 2022 • edited by tdonohue Loading

References

Description

Instructions for Reviewers

Checklist

github-actions bot commented Dec 9, 2022

jeffmorin commented Dec 15, 2022

tdonohue left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdonohue Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 21, 2022

jeffmorin commented Dec 2, 2022 •

edited by tdonohue

Loading

tdonohue left a comment •

edited

Loading

tdonohue Dec 16, 2022 •

edited

Loading