344 - Display real collection data in the Collection Page

[MellyGray]

What this PR does / why we need it:

This PR adds the integration with js-dataverse getCollection use case to display real collection data in the Collection Page.

Depends on:

• Create GetCollection use case dataverse-client-javascript#131

Which issue(s) this PR closes:

• Closes Display real Collection name, description and affiliation in the Collection Page #344

Special notes for your reviewer:

e2e MetadataBlock test is failing because of some recent changes to the API. This is fixed in another PR:

• 316 - Functional create dataset form #334

The UI does not exactly match that of JSF, as best practices dictate that breadcrumbs should always be positioned at the top, before anything else

Suggestions on how to test this:

Step 1: Run the Development Environment

1. Execute npm i.
2. Navigate with cd packages/design-system && npm run build.
3. Return with cd ../../.
4. Ensure you have a .env file similar to .env.example, with the variable VITE_DATAVERSE_BACKEND_URL=http://localhost:8000.
5. Navigate with cd dev-env.
6. Start the environment using ./run-env.sh unstable.
7. To verify the environment, visit http://localhost:8000 and check your local Dataverse installation.

Step 2: Test the Form

1. Navigate to http://localhost:8000.
2. Log in as an admin using the username: dataverseAdmin and password: admin1.
3. Create a new Dataverse collection.
4. Go to <http://localhost:8000/spa/collections?id=>
5. Check that the collection data is correct and that the breadcrumbs are correct.

Does This PR Introduce a User Interface Change? If Mockups Are Available, Please Link/Include Them Here:

Is There a Release Notes Update Needed for This Change?:

Collection Page now displays real collection data

[coveralls]

coverage: 97.494% (+0.04%) from 97.459%
when pulling bd9ce3c on feature-344-display-real-collection-data
into c0f2d0b on develop.

[ekraffmiller]

Hi @MellyGray it looks good, I just have a couple of comments, and also can you resolve the merge conflicts? thank you

[ekraffmiller]

In the description, we need to support markup, like we do in the Metadata Fields, for example this is what the description looks like in JSF:

[MellyGray]

Well spotted! I've applied the changes

[ekraffmiller]

Thanks! It is handling the markdown now. I tried it again though, and the e2e test isn't working, I realized js-dataverse needs to be converting the html to markdown. I created an issue for that: IQSS/dataverse-client-javascript#140

[MellyGray]

Ok, thanks!

[ekraffmiller]

Does getById() need to return undefined as well as Collection? In the js-dataverse useCase, it returns only Collection

https://github.com/IQSS/dataverse-client-javascript/blob/e0438a257ec5a1aaf56300da73a1b68c79cd80d5/src/collections/infra/repositories/CollectionsRepository.ts#L9

[MellyGray]

Returning Collection | undefined in TypeScript works like Java's Optional. It means the 'get' function could return a Collection if one is found, or undefined if not. This ensures type safety: just as you must unwrap an Optional in Java to access its value, in TypeScript, checking for undefined ensures you're aware when a value might not be present. If the Collection isn't found, we typically show a 'Page Not Found' message. For other errors, we throw an exception

Using this type definition, when we create a hook to get the collection, we acknowledge that the collection might not be found and could be undefined. Therefore, we need to handle the possibility of undefined when accessing the collection's properties, ensuring our code safely deals with the absence of a value.

It's a matter of design choice, but another approach could be to always return a Collection and throw an error for any scenario where this isn't possible

[ekraffmiller]

Ok, I see what you mean and how it compares to Java Optional, that makes sense. And in the hook useCollections.tsx it makes sense for collection to be type Collection | undefined . But the js-dataverse doesn't return undefined when the id is not found, it throws an error. So because the useCase doesn't handle the error from js-dataverse, it doesn't return undefined, but the error is caught in the hook. So that's why I was confused about Collection | undefined being declared in the useCase. It implies that the function handles the error if the collection ins't found.

Now that I look at the logic for getting Datasets, the same thing is happening - if a dataset isn't found, js-dataverse throws an error, and it isn't handled by the useCase, it's handled by the caller of the useCase. So I think the same question applies, should we be handling the error in the useCase, if it is declared to sometimes return undefined?

[MellyGray]

I think js-dataverse should return undefined if it can't find a dataset or any other entity. This way, users know when something doesn't exist and can decide what to do next, like showing an error page or something else. It adds an extra step, but I believe it's useful. However, I might see it this way because of how we do things on the frontend.

We should talk about this with @GPortas. If we decide against returning undefined in js-dataverse, we'll just handle that part on the frontend.

[GPortas]

@ekraffmiller @MellyGray

I think js-dataverse should always throw errors for any API operation that returns an error. Since it's an API wrapper, IMO it seems more natural to return an error to the consumer than an undefined value, like the API does (the API doesn't return a 200 response with a null body if a resource is not found). Note that the message accompanying the 404 error returned by the API may provide useful information to the consumer, which would be lost if we return undefined.

Regarding what Melina says about ensuring our code safely handles the absence of a value, this can still be done by throwing an error. In js-dataverse there are different types of errors defined, very high level and general purpose for now: https://github.com/IQSS/dataverse-client-javascript/tree/develop/src/core/domain/repositories. For now any error returned by the API in a read operation returns a ReadError, which includes the message with the HTTP error code as a very simple string, but we can define a new NotFoundError that extends ReadError also including more refined data. This way we can safely identify a not found error within the block catch of the promise, and carry out the necessary actions in the SPA.

[MellyGray]

@GPortas sounds good! How do we proceed with this PR? Should I remove the undefined from the repository definition or keep it until we implement the NotFoundError in js-dataverse?

[GPortas]

@MellyGray I would remove the undefined type in this PR and then create two new issues:

1. Create the NotFoundError in js-dataverse (js-dataverse repository)
2. Handle the NotFoundError properly in the frontend (dependent on the previous one) (SPA repository)

Please tell me what you think!

[GPortas]

@MellyGray @ekraffmiller I just created the issues. (prioritized for the next sprint):

• Create the NotFoundError dataverse-client-javascript#142
• Integrate the js-dataverse not found error #363

[MellyGray]

I removed the undefined

[ekraffmiller]

looks good to me!

[g-saracca]

Looks good!