Skip to content

Created ListableNotificationObject#1936

Merged
tdonohue merged 9 commits intoDSpace:mainfrom
atmire:w2p-95335_created-ListableNotificationObjectComponent_contribute-7.2
Dec 7, 2022
Merged

Created ListableNotificationObject#1936
tdonohue merged 9 commits intoDSpace:mainfrom
atmire:w2p-95335_created-ListableNotificationObjectComponent_contribute-7.2

Conversation

@alexandrevryghem
Copy link
Copy Markdown
Member

@alexandrevryghem alexandrevryghem commented Oct 28, 2022

Description

@listableObjectsComponents could not handle failed RemoteData objects. To fix this a new component and his model were created, they can be used to display error messages (also works for success, info and warning messages) in lists in between the successful RemoteData objects.

image

(To display error notifications like above extra configuration is needed. You will need to iterate over your list of RemoteData objects to ensure that you only return the payload of the RemoteData<T> if it exists , if it doesn't exist you need to return a new ListableNotificationObject)

Instructions for Reviewers

List of changes in this PR:

  • Created the ListableNotificationObject
  • Created resource type LISTABLE_NOTIFICATION_OBJECT
  • (Also added another commit to this PR, it's an improvement regarding the PR Add more themed components #1674 where the ExpandableNavbarSectionComponent was not present in the ENTRY_COMPONENTS. This caused it to sometimes load to slow which in turn caused some layout issues)

Guidance for how to test or review this PR:

  • Create a ds-listable-notification-object on the home page and give with value new ListableNotificationObject(NotificationType.Error, 'Error message') for the field object.
  • An error notification should be visible.

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
  • 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.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • 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.

@alexandrevryghem alexandrevryghem self-assigned this Oct 28, 2022
@artlowel artlowel added this to the 7.5 milestone Oct 28, 2022
@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge label Oct 28, 2022
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@alexandrevryghem : I gave this a review today, and while the code looks reasonable, I'm not understanding how this is supposed to work in the codebase.

I expected that this new ListableNotificationObject would be used elsewhere in our code to automatically trigger a ListableNotificationObject to be created if the corresponding ListableObject could not be loaded (based on the screenshot you show above).

However, it appears this new ListableNotificationObject is unused in our codebase & the only way to use it would be to create custom code. Is that accurate? Or, am I misunderstanding how this is used by our codebease.

If this ListableNotificationObject is unused at this time, then I'd recommend we wait to add it into the codebase until there is a real use case in the DSpace codebase.

Comment thread src/assets/i18n/en.json5 Outdated
"person.orcid.registry.auth": "ORCID Authorizations",
"home.recent-submissions.head": "Recent Submissions",

"idle-modal.extend-session": "Extend session",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This appears to be a duplicative i18n key. It already exists on line 4582, so it can be removed from this line

@alexandrevryghem alexandrevryghem force-pushed the w2p-95335_created-ListableNotificationObjectComponent_contribute-7.2 branch 4 times, most recently from 83144cb to 82e588b Compare December 1, 2022 17:34
@alexandrevryghem alexandrevryghem force-pushed the w2p-95335_created-ListableNotificationObjectComponent_contribute-7.2 branch from 82e588b to 4897110 Compare December 2, 2022 09:21
@alexandrevryghem alexandrevryghem force-pushed the w2p-95335_created-ListableNotificationObjectComponent_contribute-7.2 branch from 4897110 to a0c3ca5 Compare December 2, 2022 10:07
@alexandrevryghem
Copy link
Copy Markdown
Member Author

The ListableNotificationObject is now used in DSOSelectorComponent when no/new results cannot be retrieved. This error can be simulated by turning off Solr.

To test this you need to click on add Item/Collection/Community in the admin sidebar and when you see the results you have loaded the first page of results you need to stop Solr. When you scroll down the next page DSO objects won't be successfully retrieved and an error will be displayed in between other DSO objects. If you start Solr back up you can click on the error notification and the error message should disappear and the next DSO objects will be added to the list.

Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @alexandrevryghem ! I gave this another look today & I was able to successfully test it using this "New -> Item" popup (and stopping Solr immediately after loading that page). After I restarted Solr, clicking on the "Something went wrong.." message loaded the next results.

@tdonohue tdonohue merged commit 66820dd into DSpace:main Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge improvement

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

3 participants