Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/127 add alert messages to dataset view #194

Merged
merged 31 commits into from
Oct 27, 2023

Conversation

ekraffmiller
Copy link
Contributor

What this PR does / why we need it:

Adds Alert messages to Dataset View Page (for draft version, unpublished version, and version not found)

Which issue(s) this PR closes:

Special notes for your reviewer:

Suggestions on how to test this:

View the alert messages in Storybook, in the DatasetAlerts section. In the local dev environment, go to Dataverse and create the 3 scenarios (draft version, unpublished version, and version not found), then view the dataset page in the SPA.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

no

Is there a release notes update needed for this change?:

no

Additional documentation:

@coveralls
Copy link

coveralls commented Oct 10, 2023

Coverage Status

coverage: 98.58% (+0.2%) from 98.394% when pulling ecf87c4 on feature/127-add-alert-messages-to-dataset-view into 74bb2de on develop.

@MellyGray MellyGray assigned MellyGray and unassigned ekraffmiller Oct 11, 2023
@MellyGray MellyGray changed the base branch from develop to feature/99-alert-mechanism October 11, 2023 07:59
Copy link
Contributor

@MellyGray MellyGray left a comment

Choose a reason for hiding this comment

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

The component looks good but there are some behaviours that are different from JSF, take a look at my comments

public/locales/en/dataset.json Show resolved Hide resolved
src/stories/dataset/Dataset.stories.tsx Show resolved Hide resolved
public/locales/en/dataset.json Show resolved Hide resolved
public/locales/en/dataset.json Show resolved Hide resolved
public/locales/en/dataset.json Show resolved Hide resolved
public/locales/en/dataset.json Show resolved Hide resolved
@MellyGray MellyGray assigned ekraffmiller and unassigned MellyGray Oct 11, 2023
@ekraffmiller ekraffmiller added the Size: 3 A percentage of a sprint. 2.1 hours. label Oct 11, 2023
Base automatically changed from feature/99-alert-mechanism to develop October 18, 2023 20:55
@ekraffmiller
Copy link
Contributor Author

Hi @MellyGray , welcome back from vacation!
I think I addressed all the issues, the only remaining one being needing to add privateUrlToken to the Dataset object, as described above. If we agree that another property should be added, then I can update the code to use it.

Also, while testing on Demo dataverse, I noticed use case I hadn't seen before, 'Incomplete Metadata':
Screenshot 2023-10-21 at 4 48 30 PM

I'm not sure how the dataset got into that state - some required fields were missing. I think it may have gotten into a bad state when the Demo dataverse was updated - maybe a new metadata block was added? But since the JSF has an alert for it, I think we need to add it (maybe in a subsequent ticket). The style of it, with red text in a warning box, doesn't match our existing Alert component, so we should probably talk to the team about modifying it for the SPA.

@MellyGray
Copy link
Contributor

@ekraffmiller

The applied changes look good! But now there is a space at the right which wasn't there before
Captura de pantalla 2023-10-23 a las 13 02 42

Also, go ahead with adding privateUrlToken to the Dataset object

We can create a new issue for the Incomplete Metadata message and discuss the style with the team 👍

@MellyGray MellyGray assigned ekraffmiller and unassigned MellyGray Oct 23, 2023
@MellyGray MellyGray assigned ekraffmiller and unassigned MellyGray Oct 24, 2023
Copy link
Contributor

@MellyGray MellyGray left a comment

Choose a reason for hiding this comment

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

LGTM! Let's move this forward

@MellyGray MellyGray removed their assignment Oct 26, 2023
@kcondon kcondon self-assigned this Oct 26, 2023
@kcondon
Copy link
Contributor

kcondon commented Oct 26, 2023

Tested as recommended and I'm seeing the alerts in the running spa service. However, noticed a couple issues that are more about dataset state and navigation but related:

  1. When the dataset is a draft and has never been published, ie no 1.0, the alert does say it is a draft and unpublished but does not show the unpublished tag under the title, only the draft tag. This is different from the current app
    Screen Shot 2023-10-26 at 4 50 50 PM
    Screen Shot 2023-10-26 at 4 49 36 PM

  2. When referencing a non existent version, eg. 2.0 when there is only a 1.0 and a draft, in the old app is goes to 1.0 and provides the does not exist alert. In SPA, it goes to the latest version, ie. the draft, says 2.0 doesn't exist but this is the draft and needs to be published.
    Screen Shot 2023-10-26 at 4 58 02 PM
    Screen Shot 2023-10-26 at 4 48 18 PM (1)

@ekraffmiller
Copy link
Contributor Author

I fixed the logic for determining if the dataset has been published (this is temporary, and will be replaced with info from the server when integration is completed).
For the requested version, if the version isn't found, it will only show the Draft if the user has permission to view the draft, otherwise it will show the latest published version.

@kcondon kcondon merged commit 54ed937 into develop Oct 27, 2023
4 of 6 checks passed
@kcondon kcondon deleted the feature/127-add-alert-messages-to-dataset-view branch October 27, 2023 19:19
jayanthkomarraju pushed a commit to jayanthkomarraju/dataverse-frontend that referenced this pull request May 31, 2024
…to-dataset-view

Feature/127 add alert messages to dataset view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: 3 A percentage of a sprint. 2.1 hours.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add all alert messages related to the Dataset View Mode
4 participants