Skip to content

Use the DSONameService to display DSpaceObjects names#2129

Merged
tdonohue merged 13 commits intoDSpace:mainfrom
alexandrevryghem:retrieve-name-with-dsonameservice-main
May 22, 2023
Merged

Use the DSONameService to display DSpaceObjects names#2129
tdonohue merged 13 commits intoDSpace:mainfrom
alexandrevryghem:retrieve-name-with-dsonameservice-main

Conversation

@alexandrevryghem
Copy link
Copy Markdown
Member

@alexandrevryghem alexandrevryghem commented Mar 2, 2023

References

Description

Used the DSONameService on all the places where the name of a DSpaceObject is rendered

Instructions for Reviewers

List of changes in this PR:

  • I replaced all the places who used dso.name (found using the regex \.name[^a-zA-Z0-9] & by browsing the through the UI)
  • Also fixed CSS of ItemCollectionMapperComponent
    • The title at the top needed some extra margin top
    • There was a lot of whitespace in the table (at first I thought it was an empty column)
      image
  • Also fixed issue in SearchFormComponent where a page refresh after selecting a scope would set the text value of the button by default to "All of DSpace" instead of the name of the Community/Collection.

Include guidance for how to test or review your PR.

  1. Edit the DSONameService.getName() & DSONameService.getHitHighlights() to return hardcoded easy to find strings like __EPERSON__
  2. Browse through all the pages on the repo and check that all the names of the Collections, Communities, Items or (E)Persons are being displayed with __{type}__
  3. Check that everything is still being displayed correctly without the __{type}__ overrides and with some Collections, Communities, ... without a dc.title, person.familyName, dc.lastname, ...

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 ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • 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 libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

…dsonameservice-7.4

# Conflicts:
#	src/app/access-control/epeople-registry/epeople-registry.component.html
#	src/app/access-control/epeople-registry/eperson-form/eperson-form.component.ts
#	src/app/access-control/group-registry/group-form/group-form.component.spec.ts
#	src/app/access-control/group-registry/group-form/group-form.component.ts
#	src/app/access-control/group-registry/group-form/members-list/members-list.component.html
#	src/app/access-control/group-registry/group-form/members-list/members-list.component.ts
#	src/app/collection-page/edit-item-template-page/edit-item-template-page.component.html
#	src/app/item-page/full/field-components/file-section/full-file-section.component.ts
#	src/app/item-page/media-viewer/media-viewer-video/media-viewer-video.component.ts
#	src/app/item-page/simple/field-components/file-section/file-section.component.html
#	src/app/item-page/simple/field-components/file-section/file-section.component.ts
#	src/app/item-page/versions/item-versions.component.ts
#	src/app/shared/auth-nav-menu/user-menu/user-menu.component.html
#	src/app/shared/auth-nav-menu/user-menu/user-menu.component.ts
#	src/app/shared/object-collection/shared/mydspace-item-submitter/item-submitter.component.html
@github-actions
Copy link
Copy Markdown

Hi @alexandrevryghem,
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.

@github-actions
Copy link
Copy Markdown

Hi @alexandrevryghem,
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.

…dsonameservice-main

# Conflicts:
#	src/app/item-page/media-viewer/media-viewer-video/media-viewer-video.component.ts
@alexandrevryghem alexandrevryghem force-pushed the retrieve-name-with-dsonameservice-main branch from 9969820 to e14760a Compare March 24, 2023 22:27
@tdonohue tdonohue self-requested a review March 30, 2023 14:51
@tdonohue tdonohue added this to the 7.6 milestone Mar 30, 2023
@tdonohue tdonohue requested a review from mwoodiupui March 30, 2023 14:51
@github-actions
Copy link
Copy Markdown

Hi @alexandrevryghem,
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.

…dsonameservice-main

# Conflicts:
#	src/app/browse-by/browse-by-date-page/browse-by-date-page.component.ts
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've reviewed the code and it looks good. It's basically the same change over and over. I've also tested as many of the components as I could from the UI and noticed no change in behavior. I've verified #343 is fixed.

This all looks good to me & I think it can be merged as soon as @mwoodiupui has a chance to give it a secondary test.

@alexandrevryghem
Copy link
Copy Markdown
Member Author

Hey @tdonohue, I'm just curious but does this mean that you were not able to test the fix for #2153?

@tdonohue
Copy link
Copy Markdown
Member

@alexandrevryghem : I'm not yet certain that this PR fixes #2153 to be honest... I do believe this PR improves the behavior because it properly handles a Submitter / EPerson which has no name.

However, I believe #2153 is describing a slightly different issue where the submitter is null (i.e. doesn't exist at all). In that scenario, I don't believe this DSONameService fixes the problem, because the DSO itself is null / undefined.

While I haven't had a chance to fully verify things, I believe this PR provides a partial fix to #2153 (for the scenario where the submitter exists but is missing a name).... and I think #2155 (also linked to #2153) may provide the rest of the fix (for the scenario where the submitter is null). That said, I haven't had a chance yet to test #2155 along with this PR...and it appears that #2155 needs more work based on recent reviews.

@github-actions
Copy link
Copy Markdown

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

@alexandrevryghem
Copy link
Copy Markdown
Member Author

@tdonohue: You're right this solution only fixes the problem when the firstname and/or lastname of the submitter is not defined and it does not handle the case when the user is not defined. Because that PR currently also doesn't fix the issue when the submitter is null yet I created this commit based on this branch. I created it because these 2 PRs will have a merge conflict and solving that merge conflict would basically result into a complete replacement from all the changes made in that PR with the usage of the DSONameService used in this PR. Because my solution also handles the cases where the name would correctly be displayed when only the firstname or only the lastname is used I think using this solution would be best. Maybe this PR can be merged first and then Cris could replace that PR with the changes I made in this commit (or I could just add that commit to this PR)

…onameservice-main

# Conflicts:
#	src/app/item-page/media-viewer/media-viewer-video/media-viewer-video.component.ts
@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented Apr 20, 2023

@alexandrevryghem : If you want to simply push your new fix to this PR, I'm OK with that. Currently, it appears the commit you linked it isn't in this PR (it's on a different branch).

Based on my reading of that new code, I think you are correct that it would make this PR a complete fix to #2153 (and the other PR could simply be closed). I'm OK with that approach...and I can then re-test this PR to verify that it completely fixes #2153.

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 retested this today and verified it fixes both #343 and also #2153. Here's a screenshot which proves submitter bug is now fixed... I have two WorkflowItems, one with an existing Submitter...and one where the submitter (EPerson object) is deleted.

submitter-test

So, I think this can be merged as soon as @mwoodiupui gives it a quick glance (even just a quick code review is fine).

@github-actions
Copy link
Copy Markdown

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

…dsonameservice-main

# Conflicts:
#	src/app/shared/object-detail/my-dspace-result-detail-element/claimed-task-search-result/claimed-task-search-result-detail-element.component.spec.ts
#	src/app/shared/object-detail/my-dspace-result-detail-element/claimed-task-search-result/claimed-task-search-result-detail-element.component.ts
#	src/app/shared/object-detail/my-dspace-result-detail-element/item-detail-preview/item-detail-preview.component.ts
#	src/app/shared/object-detail/my-dspace-result-detail-element/item-search-result/item-search-result-detail-element.component.spec.ts
#	src/app/shared/object-detail/my-dspace-result-detail-element/pool-search-result/pool-search-result-detail-element.component.spec.ts
#	src/app/shared/object-detail/my-dspace-result-detail-element/pool-search-result/pool-search-result-detail-element.component.ts
#	src/app/shared/object-detail/my-dspace-result-detail-element/workflow-item-search-result/workflow-item-search-result-detail-element.component.ts
#	src/app/shared/object-detail/my-dspace-result-detail-element/workspace-item-search-result/workspace-item-search-result-detail-element.component.ts
#	src/app/shared/object-grid/search-result-grid-element/collection-search-result/collection-search-result-grid-element.component.html
#	src/app/shared/object-grid/search-result-grid-element/community-search-result/community-search-result-grid-element.component.html
#	src/app/shared/subscriptions/subscription-modal/subscription-modal.component.html
#	src/app/shared/subscriptions/subscription-view/subscription-view.component.html
@tdonohue
Copy link
Copy Markdown
Member

@mwoodiupui : Would you have an chance to give this a quick test on your end so that we can merge this?

One very quick way to test this is to create a Community and then remove it's dc.title (at the database level). You can then just verify that it shows up as "Untitled" instead of as an empty entry like in the screenshot of the original ticket: #343 (comment)

@github-actions
Copy link
Copy Markdown

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

…dsonameservice-main

# Conflicts:
#	src/app/workflowitems-edit-page/advanced-workflow-action/advanced-workflow-action-select-reviewer/reviewers-list/reviewers-list.component.ts
Copy link
Copy Markdown
Member

@mwoodiupui mwoodiupui left a comment

Choose a reason for hiding this comment

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

Tested as @tdonohue suggests: it works as advertised. I have not read all of the code, but it looks sensible, builds and runs without error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug e/5 Estimate in hours usability

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

Submitter: undefined undefined Communities created without a title cannot be accessed

3 participants