Enrich local data via the OpenAIRE Graph #1562

LucaGiamminonni · 2022-03-17T16:11:35Z

References

Related to Rest contracts for qaevents, qatopics and qasources endpoints RestContract#181
Related to Add a correction service to dspace to enhance the data quality (was Enrich local data via the OpenAIRE Graph) DSpace#8184

Description

The features included in this PR are the result of the OpenAIRE Call Innovation funded project "Enrich local data via the OpenAIRE Graph” awarded by 4Science (https://www.openaire.eu/open-call-winner-phase-1-4science). It provides a closer integration between DSpace and two OpenAIRE services, the Notification Broker and the OpenAIRE REST API.
Detailed documentation about the aims of the project, the implementation and the configuration options is available at https://4science.github.io/oaire-eld/#/
This PR regarding only the data correction section. The publication claim will be migrated with other PR.

How to Use / Test

Video: https://github.com/DSpace/DSpace/assets/483997/6db3dd92-1e26-416c-b130-9f5677f7acaa

Start up frontend & backend
Import a valid JSON OpenAIRE Graph JSON file using the import-openaire-events Process (in the Processes Admin UI)
- Sample file: https://github.com/DSpace/DSpace/files/13505259/test-nbevents-sample.-.Copie.json
- Structure of this JSON is defined at https://github.com/4Science/oaire-eld/blob/main/data-correction.md
- "ENRICH" types defined at https://api.openaire.eu/vocabularies/dnet:topic_types
Assuming import is successful, this will create several Notifications on the Admin menu -> Notifications -> Quality Assurance page. (The exact number/type of Notifications is defined in the JSON file you imported)
From that page, you can now use the "Actions" button to view each notification and decide whether to Accept/Ignore/Reject each.
- Accepting will apply the change.
- Ignoring will delete the change without applying
- Reject will delete the change without applying and send a notification back to the service.

…ders.

into CST-5337

lgtm-com · 2022-03-17T16:29:37Z

This pull request introduces 5 alerts when merging a4718b6 into 3ecb3c2 - view on LGTM.com

new alerts:

5 for Unused variable, import, function or class

paulo-graca

Thank you @LucaGiamminonni! I've just noticed some missing typescript docs. They can be useful if someone tries to override this code to better understand what is the purpose.

paulo-graca · 2022-04-21T14:17:35Z

...tifications-broker-source-page-component/admin-notifications-broker-source-page.component.ts

+  selector: 'ds-admin-notifications-broker-source-page-component',
+  templateUrl: './admin-notifications-broker-source-page.component.html',
+})
+export class AdminNotificationsBrokerSourcePageComponent {}


missing typescript docs

paulo-graca · 2022-04-21T14:17:57Z

...s/admin-notifications-broker-topics-page/admin-notifications-broker-topics-page.component.ts

+  selector: 'ds-notification-broker-page',
+  templateUrl: './admin-notifications-broker-topics-page.component.html'
+})
+export class AdminNotificationsBrokerTopicsPageComponent {


missing typescript docs

paulo-graca · 2022-04-21T14:18:56Z

...s/admin-notifications-broker-events-page/admin-notifications-broker-events-page.component.ts

+  selector: 'ds-notifications-broker-events-page',
+  templateUrl: './admin-notifications-broker-events-page.component.html'
+})
+export class AdminNotificationsBrokerEventsPageComponent {


missing typescript docs

tdonohue

@LucaGiamminonni : I'm no longer able to test this as the backend doesn't compile & this PR has merge conflicts. But, I did give this a code review today. Overall the code looks well commented & well designed. Obviously we'll need to rename many of these classes eventually to move from "notification broker" (NB) to "quality assurance" (QA) as decided in a dev meeting. Beyond that, I just have a few other inline comments.

Once this & the backend are ready for testing again, I'll gladly give this a test so that I can comment on the UI, user experience, etc.

tdonohue · 2022-05-17T16:09:59Z

src/app/notifications/broker/events/notifications-broker-events.component.ts

+    );
+  }
+
+  protected computePIDHref(event: OpenaireBrokerEventMessageObject) {


This method has many hardcoded URL prefixes which sites may need to occasionally change. I'm wondering if there's a way to move this logic to the backend where it could be more easily configurable? Is there a reason why the prefix needs to be added here in the UI, instead of just making sure the full URL can be sent by the backend?

Yes I will modify the rest side resource to include the url in the OpenaireBrokerEventMessageObject

tdonohue · 2022-05-17T16:10:50Z

src/app/notifications/broker/events/notifications-broker-events.scomponent.scss

@@ -0,0 +1,21 @@
+.button-rows {


This file name has an extra s before component. It says .scomponent.scss

tdonohue · 2022-05-17T16:12:09Z

src/app/notifications/broker/source/notifications-broker-source.component.ts

+  templateUrl: './notifications-broker-source.component.html',
+  styleUrls: ['./notifications-broker-source.component.scss']
+})
+export class NotificationsBrokerSourceComponent implements OnInit {


Please add TypeDocs

tdonohue · 2022-06-08T15:02:46Z

Moving this PR to 7.4 as it won't be possible to review/approve in time for 7.3. We'll instead work to get this merged just after the 7.3 release.

lgtm-com · 2022-07-06T15:33:25Z

This pull request introduces 6 alerts when merging a7d2278 into fae355a - view on LGTM.com

new alerts:

6 for Unused variable, import, function or class

tdonohue

@LucaGiamminonni and @frabacche : Gave this a code review today. Overall, it looks great! I found some minor issues and a few questions/suggestions (see inline comments below).

I also wanted to note that some of the bugs I reported in the backend PR are actually frontend issues (e.g. the accessibility issues). So, those will need to be addressed in this PR. See this review for those minor bugs: DSpace/DSpace#8184 (review)

Overall, this looks great. Just minor cleanup necessary and then I'll be +1. Some of this feedback is optional to address...so feel free to let me know if there's anything here you cannot address easily.

...dmin-quality-assurance-source-page-component/admin-quality-assurance-source-data.reslover.ts

src/app/shared/mocks/notifications.mock.ts

src/app/suggestion-notifications/qa/events/quality-assurance-events.component.html

...estion-notifications/qa/project-entry-import-modal/project-entry-import-modal.component.html

src/app/core/suggestion-notifications/qa/events/quality-assurance-event-data.service.spec.ts

src/app/suggestion-notifications/suggestion-notifications.module.ts

tdonohue

@frabacche ; I gave this another look today. My prior feedback has been resolved, but there are a few small suggestions I have for the new configurations you've added (see inline below).

I believe the only remaining feedback to address is to try to fix the small bugs mentioned in my review of the backend. The following bugs still exist from that prior report:

Breadcrumbs are not working properly. They always only have one level. It should be possible to move from a Topic back to the list of topics using the breadcrumbs. (This is visible in the screenshot below where that page doesn't have a parent page in the breadcrumbs)

All small action buttons (Accept/Ignore/Reject) on a topic page have no accessibility text. This can be easily seen by performing a scan of the page using a browser accessibility plugin like "axe DevTools". See the screenshot.

I believe the same accessibility issue also exists with the "Actions" buttons on the "Quality Assurance" page. The accessibility text on those buttons is just a number.

When I visit the page for a topic (like in the screenshot above), I see data logged to my Console window (in Chrome's DevTools). Something looks to be sending all the JSON data to console.log?
...
6. The ordering of the "Current Topics" on the initial "Quality Assurance" page (/admin/notifications/quality-assurance/openaire) is unclear & seems to randomly change at times. (WE AGREED it should be alphabetical) FIXED NOW ON BACKEND
...
9. Finally, ... it'd be nice if there was a way to disable this feature altogether. (By this, I mean that the OpenAIRE quality assurance page should not appear if OpenAIRE integration is somehow disabled. It could be via a boolean setting in qaevents.cfg or maybe it just checks to see if the qaevents.openaire.broker-url is specified. If so, it's enabled, if not it's disabled.) FIXED NOW ON BACKEND

All other feedback was addressed. You mentioned the above are being "worked on", so this is just a reminder that I'm waiting on these before giving a +1 vote. Overall, this is nearly ready. The sooner you can get to these changes, the sooner we can get this merged!

src/config/quality-assurance.config.ts

src/app/notifications/qa/events/quality-assurance-events.component.ts

paulo-graca

Thank you, @LucaGiamminonni, for addressing my comments. I'm currently heavily involved in our local projects, and I don't have much time right now, and until the end of the year, to do thorough reviews. Please excuse me for that.

I just want to share with you some concerns I have and what I'm expecting the result to be for this PR:

I expect to have a consistent interface that doesn't break with what the user is used to. So, I expect the site navigation to be consistent.

I also expect the user to be content source-aware. It should be clear at any time that the suggested content came from a specific source (we can have others than OpenAIRE).

This is mainly an admin feature, so perhaps this comment doesn't make sense, but I didn't see themed components. Not sure if needed.

This feature should be source-agnostic, but I saw some references to OpenAIRE (properties, methods).

I'm not intending to block this PR. If @tdonohue understands that this PR can go as is, he can have my +1, and issues can be addressed later on.

src/app/core/notifications/qa/models/quality-assurance-event.model.ts

remove console logs Approved-by: Stefano Maffei

add accessibility text Approved-by: Stefano Maffei

CST-12904 breadcrumbs qa Approved-by: Giuseppe Digilio

tdonohue · 2023-12-18T16:29:41Z

@frabacche : This appears to have 3 lint errors after the recent updates. Run yarn lint locally to see them. You may also be able to run yarn lint --fix to try to fix them automatically (doesn't always work).

In the meantime, I'll give it a test in the next few hours and verify everything looks good.

tdonohue

👍 Thanks @frabacche , @FrancescoMolinaro and @LucaGiamminonni ! This all looks good to me now. I gave it another test/review and verified my prior feedback has all been addressed. I also checked @paulo-graca 's feedback and it also looks to be addressed, except for a request for making all components themeable. But, in my opinion, that can be addressed in a later PR as more features will need to be built on this initial PR.

tdonohue · 2023-12-18T19:52:51Z

Merging, as this has a +1 from myself, and a prior +1 from @paulo-graca . I've also verified that all of @paulo-graca 's feedback appears to be addressed except for the themed components. But those can be added in later PRs

Thanks again to @frabacche , @FrancescoMolinaro , @LucaGiamminonni and the entire 4Science team for their hard work on this new feature!

LucaGiamminonni and others added 8 commits February 18, 2022 17:26

[CST-5249] Migration of OPENAIRE correction service

b8e9f62

[CST-5246] Correction service should support multiple providers

4ca5138

[CST-5337] OAIRE-ELD correction service should support multiple provi…

8ce3148

…ders.

[CST-5337] test cases done.

00f7fa9

[CST-5337] change end point.

d63bf55

Merge remote-tracking branch '4Science-bitbucket/main' into CST-5337

c400026

[CST-5337] Fixed notifications labels

6dfeb1a

Merge branch 'CST-5337' of https://bitbucket.org/4Science/dspace-angular

a4718b6

into CST-5337

LucaGiamminonni mentioned this pull request Mar 17, 2022

Add a correction service to dspace to enhance the data quality (was Enrich local data via the OpenAIRE Graph) DSpace/DSpace#8184

Merged

6 tasks

tdonohue mentioned this pull request Mar 22, 2022

Enrich local data via the OpenAIRE Graph #1007

Closed

5 tasks

tdonohue requested review from tdonohue and paulo-graca March 24, 2022 15:45

tdonohue assigned LucaGiamminonni Mar 25, 2022

paulo-graca reviewed Apr 21, 2022

View reviewed changes

tdonohue added the medium priority label May 5, 2022

github-actions bot added the merge conflict label May 10, 2022

tdonohue reviewed May 17, 2022

View reviewed changes

LucaGiamminonni and others added 4 commits July 6, 2022 16:39

Merge remote-tracking branch '4Science-bitbucket/main' into CST-5337

df93058

[CST-5337] Replace Notifications broker with Quality assurance

d5c1b11

[CST-5249] Fix deprecated selector creation

a355a15

[CST-5249] Renamed nbevent and nbtopics

a7d2278

LucaGiamminonni added 2 commits July 6, 2022 17:57

[CST-5249] Renamed notifications module to suggestion-notifications

4a996b4

[CST-5337] Fixed lint and tests

f2400a7

This was referenced Dec 1, 2023

Coar Notify Integration #2681

Merged

Coar Notify Integration DSpace/DSpace#9218

Merged

tdonohue requested changes Dec 4, 2023

View reviewed changes

FrancescoMolinaro added 8 commits December 12, 2023 09:34

add qa breadcrumb

eed9896

add QA breadcrumb resolver and service

b9085d5

rename data service

b9af731

align to branch, update test

53329cd

add accessibility text

78df236

refactor, improve code according to suggestions

922172d

remove console logs

8113946

add quality assurance configuration

7fe81ec

tdonohue reviewed Dec 13, 2023

View reviewed changes

src/config/quality-assurance.config.ts Outdated Show resolved Hide resolved

src/config/quality-assurance.config.ts Outdated Show resolved Hide resolved

FrancescoMolinaro added 2 commits December 14, 2023 12:11

refactor config QA

f2be578

add QA authorization config

f7be255

paulo-graca reviewed Dec 15, 2023

View reviewed changes

src/app/notifications/qa/events/quality-assurance-events.component.ts Outdated Show resolved Hide resolved

paulo-graca reviewed Dec 15, 2023

View reviewed changes

src/app/core/notifications/qa/models/quality-assurance-event.model.ts Outdated Show resolved Hide resolved

src/app/core/notifications/qa/models/quality-assurance-event.model.ts Outdated Show resolved Hide resolved

FrancescoMolinaro added 6 commits December 18, 2023 08:23

Merged in CST-12908-remove-console-log (pull request DSpace#1109)

71783bb

remove console logs Approved-by: Stefano Maffei

Merged in CST-12906-accessibility-qa (pull request DSpace#1108)

b3a345a

add accessibility text Approved-by: Stefano Maffei

refactor

2d2ca20

merge CST-5337

fd4ca45

Merged in CST-12904-breadcrumbs-qa (pull request DSpace#1107)

9351b58

CST-12904 breadcrumbs qa Approved-by: Giuseppe Digilio

adapt config for url search

c386a4d

frabacche requested a review from tdonohue December 18, 2023 11:55

fix lint

56cf0e1

tdonohue added the integration: QA Notifications Related to QA (Quality Assurance) notifications from external sources label Dec 18, 2023

tdonohue approved these changes Dec 18, 2023

View reviewed changes

tdonohue added this to the 8.0 milestone Dec 18, 2023

tdonohue merged commit 92a10ce into DSpace:main Dec 18, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enrich local data via the OpenAIRE Graph #1562

Enrich local data via the OpenAIRE Graph #1562

LucaGiamminonni commented Mar 17, 2022 •

edited by tdonohue

Loading

lgtm-com bot commented Mar 17, 2022

paulo-graca left a comment

paulo-graca Apr 21, 2022

paulo-graca Apr 21, 2022

paulo-graca Apr 21, 2022

tdonohue left a comment

tdonohue May 17, 2022

LucaGiamminonni Jul 8, 2022

tdonohue May 17, 2022

tdonohue May 17, 2022

tdonohue commented Jun 8, 2022

lgtm-com bot commented Jul 6, 2022

tdonohue left a comment

tdonohue left a comment •

edited

Loading

paulo-graca left a comment

tdonohue commented Dec 18, 2023 •

edited

Loading

tdonohue left a comment

tdonohue commented Dec 18, 2023

Enrich local data via the OpenAIRE Graph #1562

Enrich local data via the OpenAIRE Graph #1562

Conversation

LucaGiamminonni commented Mar 17, 2022 • edited by tdonohue Loading

References

Description

How to Use / Test

lgtm-com bot commented Mar 17, 2022

paulo-graca left a comment

Choose a reason for hiding this comment

paulo-graca Apr 21, 2022

Choose a reason for hiding this comment

paulo-graca Apr 21, 2022

Choose a reason for hiding this comment

paulo-graca Apr 21, 2022

Choose a reason for hiding this comment

tdonohue left a comment

Choose a reason for hiding this comment

tdonohue May 17, 2022

Choose a reason for hiding this comment

LucaGiamminonni Jul 8, 2022

Choose a reason for hiding this comment

tdonohue May 17, 2022

Choose a reason for hiding this comment

tdonohue May 17, 2022

Choose a reason for hiding this comment

tdonohue commented Jun 8, 2022

lgtm-com bot commented Jul 6, 2022

tdonohue left a comment

Choose a reason for hiding this comment

tdonohue left a comment • edited Loading

Choose a reason for hiding this comment

paulo-graca left a comment

Choose a reason for hiding this comment

tdonohue commented Dec 18, 2023 • edited Loading

tdonohue left a comment

Choose a reason for hiding this comment

tdonohue commented Dec 18, 2023

LucaGiamminonni commented Mar 17, 2022 •

edited by tdonohue

Loading

tdonohue left a comment •

edited

Loading

tdonohue commented Dec 18, 2023 •

edited

Loading