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/211 edit publish alert messages #217

Merged
merged 18 commits into from
Nov 23, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,23 @@
"plugins": ["react", "simple-import-sort", "import", "unused-imports"],
"rules": {
"@typescript-eslint/no-unused-vars": "off",
"react/prop-types": "off",
MellyGray marked this conversation as resolved.
Show resolved Hide resolved
"unused-imports/no-unused-imports": "error",
"unused-imports/no-unused-vars": [
"warn",
{ "vars": "all", "varsIgnorePattern": "^_", "args": "after-used", "argsIgnorePattern": "^_" }
{
"vars": "all",
"varsIgnorePattern": "^_",
"args": "after-used",
"argsIgnorePattern": "^_"
}
],
"@typescript-eslint/no-empty-function": [
"error",
{
"allow": ["arrowFunctions"]
}
],
"@typescript-eslint/no-empty-function": ["error", { "allow": ["arrowFunctions"] }],
"react/react-in-jsx-scope": "off",
"prettier/prettier": [
"error",
Expand Down
24 changes: 24 additions & 0 deletions public/locales/en/dataset.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,30 @@
"uploadFiles": "Upload Files"
},
"alerts": {
"publishInProgress": {
"heading": "Publish in Progress",
"alertText": "The dataset is locked while the persistent identifiers are being registered or updated, and/or the physical files are being validated."
},
"filesUpdated": {
"heading": "Success!",
"alertText": "One or more files have been updated."
},
"termsUpdated": {
"heading": "Success!",
"alertText": "The terms for this dataset have been updated."
},
"thumbnailUpdated": {
"heading": "Success!",
"alertText": "Dataset thumbnail updated."
},
"datasetDeleted": {
"heading": "Success!",
"alertText": "This dataset draft has been deleted."
},
"metadataUpdated": {
"heading": "Success!",
"alertText": "The metadata for this dataset has been updated."
},
"draftVersion": {
"heading": "This draft version needs to be published",
"alertText": "When ready for sharing, please <b>publish</b> it so that others can see these changes"
Expand Down
12 changes: 9 additions & 3 deletions src/dataset/domain/models/Dataset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,20 @@ export enum DatasetAlertMessageKey {
REQUESTED_VERSION_NOT_FOUND = 'requestedVersionNotFound',
REQUESTED_VERSION_NOT_FOUND_SHOW_DRAFT = 'requestedVersionNotFoundShowDraft',
SHARE_UNPUBLISHED_DATASET = 'shareUnpublishedDataset',
UNPUBLISHED_DATASET = 'unpublishedDataset'
UNPUBLISHED_DATASET = 'unpublishedDataset',
METADATA_UPDATED = 'metadataUpdated',
FILES_UPDATED = 'filesUpdated',
TERMS_UPDATED = 'termsUpdated',
THUMBNAIL_UPDATED = 'thumbnailUpdated',
DATASET_DELETED = 'datasetDeleted',
PUBLISH_IN_PROGRESS = 'publishInProgress'
}

export class DatasetAlert {
constructor(
public readonly variant: AlertVariant,
public readonly message: DatasetAlertMessageKey,
public readonly dynamicFields?: object
public readonly messageKey: DatasetAlertMessageKey,
public dynamicFields?: object
) {}
}

Expand Down
4 changes: 4 additions & 0 deletions src/sections/alerts/Alerts.module.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.container > * {
margin-top: 1em;
margin-right: 0.5em;
}
34 changes: 34 additions & 0 deletions src/sections/alerts/Alerts.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { Alert } from '@iqss/dataverse-design-system'

import { useTranslation } from 'react-i18next'
import styles from './Alerts.module.scss'
import parse from 'html-react-parser'
import { DatasetAlert } from '../../dataset/domain/models/Dataset'

interface AlertsProps {
alerts: DatasetAlert[]
}

export function Alerts({ alerts }: AlertsProps) {
const { t } = useTranslation('dataset')
return (
<div className={styles.container}>
{alerts.map((alert: DatasetAlert, index) => {
const translatedMsg = alert.dynamicFields
? t(`alerts.${alert.messageKey}.alertText`, alert.dynamicFields)
: t(`alerts.${alert.messageKey}.alertText`)
const translatedHeading = t(`alerts.${alert.messageKey}.heading`)
const alertKey = `alert-${index}`
return (
<Alert
key={alertKey}
variant={alert.variant}
customHeading={translatedHeading}
dismissible={false}>
{parse(translatedMsg)}
</Alert>
)
})}
</div>
)
}
18 changes: 18 additions & 0 deletions src/sections/dataset/DatasetAlertContext.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { createContext, useContext } from 'react'

import { DatasetAlert, DatasetAlertMessageKey } from '../../dataset/domain/models/Dataset'

interface DatasetAlertContextProps {
setDatasetAlerts: (datasetAlerts: DatasetAlert[]) => void
datasetAlerts: DatasetAlert[]
addDatasetAlert: (newAlert: DatasetAlert) => void
removeDatasetAlert: (alertId: DatasetAlertMessageKey) => void
}

export const DatasetAlertContext = createContext<DatasetAlertContextProps>({
datasetAlerts: [],
setDatasetAlerts: /* istanbul ignore next */ () => {},
addDatasetAlert: () => {},
removeDatasetAlert: () => {}
MellyGray marked this conversation as resolved.
Show resolved Hide resolved
})
export const useDatasetAlertContext = () => useContext(DatasetAlertContext)
32 changes: 32 additions & 0 deletions src/sections/dataset/DatasetAlertProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { PropsWithChildren, useState } from 'react'
import { DatasetAlertContext } from './DatasetAlertContext'

import { DatasetAlert, DatasetAlertMessageKey } from '../../dataset/domain/models/Dataset'

export const DatasetAlertProvider = ({ children }: PropsWithChildren) => {
const [datasetAlerts, setDatasetAlerts] = useState<DatasetAlert[]>([])

const addDatasetAlert = (newAlert: DatasetAlert) => {
setDatasetAlerts((prevAlerts) => [...prevAlerts, newAlert])
}
// This function will be accessible by any child component to update the datasetAlerts state
const handleSetDatasetAlerts = (alerts: DatasetAlert[]) => {
setDatasetAlerts(alerts)
}
const removeDatasetAlert = (alertId: DatasetAlertMessageKey) => {
console.log('deleting alert', alertId)
setDatasetAlerts((prevAlerts) => prevAlerts.filter((alert) => alert.messageKey !== alertId))
}

return (
<DatasetAlertContext.Provider
value={{
datasetAlerts,
setDatasetAlerts: handleSetDatasetAlerts,
addDatasetAlert,
removeDatasetAlert
MellyGray marked this conversation as resolved.
Show resolved Hide resolved
}}>
{children}
</DatasetAlertContext.Provider>
)
}
5 changes: 4 additions & 1 deletion src/sections/dataset/DatasetFactory.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { SettingJSDataverseRepository } from '../../settings/infrastructure/Sett
import { FilePermissionsProvider } from '../file/file-permissions/FilePermissionsProvider'
import { SettingsProvider } from '../settings/SettingsProvider'
import { DatasetProvider } from './DatasetProvider'
import { DatasetAlertProvider } from './DatasetAlertProvider'

const datasetRepository = new DatasetJSDataverseRepository()
const fileRepository = new FileJSDataverseRepository()
Expand All @@ -24,7 +25,9 @@ export class DatasetFactory {
<SettingsProvider repository={settingRepository}>
<MetadataBlockInfoProvider repository={metadataBlockInfoRepository}>
<AnonymizedProvider>
<DatasetWithSearchParams />
<DatasetAlertProvider>
<DatasetWithSearchParams />
</DatasetAlertProvider>
</AnonymizedProvider>
</MetadataBlockInfoProvider>
</SettingsProvider>
Expand Down
31 changes: 5 additions & 26 deletions src/sections/dataset/dataset-alerts/DatasetAlerts.tsx
Original file line number Diff line number Diff line change
@@ -1,34 +1,13 @@
import { Alert } from '@iqss/dataverse-design-system'
import { DatasetAlert } from '../../../dataset/domain/models/Dataset'
import { useTranslation } from 'react-i18next'
import styles from './DatasetAlerts.module.scss'
import parse from 'html-react-parser'
import { useDatasetAlertContext } from '../DatasetAlertContext'
import { Alerts } from '../../alerts/Alerts'

interface DatasetAlertsProps {
alerts: DatasetAlert[]
}

export function DatasetAlerts({ alerts }: DatasetAlertsProps) {
const { t } = useTranslation('dataset')

return (
<div className={styles.container}>
{alerts.map((alert: DatasetAlert, index) => {
const translatedMsg = alert.dynamicFields
? t(`alerts.${alert.message}.alertText`, alert.dynamicFields)
: t(`alerts.${alert.message}.alertText`)
const translatedHeading = t(`alerts.${alert.message}.heading`)
const alertKey = `alert-${index}`
return (
<Alert
key={alertKey}
variant={alert.variant}
customHeading={translatedHeading}
dismissible={false}>
{parse(translatedMsg)}
</Alert>
)
})}
</div>
)
const statusAlerts = useDatasetAlertContext()
alerts = alerts.concat(statusAlerts.datasetAlerts)
return <Alerts alerts={alerts}></Alerts>
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of a different design

Suggested change
const statusAlerts = useDatasetAlertContext()
alerts = alerts.concat(statusAlerts.datasetAlerts)
return <Alerts alerts={alerts}></Alerts>
const { addAlert } = useAlertContext()
alerts.map((alert) => addAlert(alert))
return <Alerts></Alerts>

So the responsibility of DatasetAlerts is to add the alerts that are created when there is a page reload and to call the Alert component that displays all the page alerts.

But the <Alerts></Alerts> component doesn't expect alerts by props, instead it reads the alerts from the AlertsContext

export function Alerts() {
   const { alerts } = useAlertContext()

This way if there is some action that doesn't require a page reload, the Alerts component will be updated with a new alert but without overriding the initial alerts. That's the cool thing about the context, it can update a state from wherever without the need of props

Additionally I did some renaming so the alerts are general alerts instead of dataset alerts so we can reuse the context and the component in future pages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I see what you mean, that makes sense. I had to add some code to make sure that the same alert isn't added multiple times, because it seems that the render of DatasetAlerts.tsx is called multiple times when the page is loaded.
I agree about making DatasetAlert more general - I renamed the DatasetAlert type and moved it to it's own file, Alert.ts. Do you think there is a need for subclassing Alert, and AlertMessageKey, that way for example, Dataset.alerts could still contain alerts of type DatasetAlert, and DatasetAlert would only allow a subset of messages? I think maybe for now it's ok to keep it general, and subclass if the alerts need be be more organized, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

The changes look good!

I like the idea of subclassing Alert. But as you said, it's fine to keep it like this since there are no other types of alerts. We can add the subclasses once there are other type of alerts

}
55 changes: 54 additions & 1 deletion src/stories/dataset/dataset-alerts/DatasetAlert.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,22 +1,75 @@
import type { Meta, StoryObj } from '@storybook/react'

import { DatasetPublishingStatus, DatasetVersion } from '../../../dataset/domain/models/Dataset'
import {
DatasetAlert,
DatasetAlertMessageKey,
DatasetPublishingStatus,
DatasetVersion
} from '../../../dataset/domain/models/Dataset'
import { DatasetAlerts } from '../../../sections/dataset/dataset-alerts/DatasetAlerts'
import { WithI18next } from '../../WithI18next'

import {
DatasetMother,
DatasetPermissionsMother
} from '../../../../tests/component/dataset/domain/models/DatasetMother'
import { DatasetAlertContext } from '../../../sections/dataset/DatasetAlertContext'

const meta: Meta<typeof DatasetAlerts> = {
title: 'Sections/Dataset Page/DatasetAlerts',
component: DatasetAlerts,
decorators: [WithI18next]
}
const allUpdateAlerts: DatasetAlert[] = [
new DatasetAlert('success', DatasetAlertMessageKey.METADATA_UPDATED),
new DatasetAlert('success', DatasetAlertMessageKey.THUMBNAIL_UPDATED),
new DatasetAlert('success', DatasetAlertMessageKey.TERMS_UPDATED),
new DatasetAlert('success', DatasetAlertMessageKey.FILES_UPDATED),
new DatasetAlert('success', DatasetAlertMessageKey.DATASET_DELETED)
]

export default meta
type Story = StoryObj<typeof DatasetAlerts>
export const UpdateAlerts: Story = {
render: () => {
const dataset = DatasetMother.createRealistic()

return (
<DatasetAlertContext.Provider
value={{
datasetAlerts: allUpdateAlerts,
setDatasetAlerts: () => {},
addDatasetAlert: () => {},
removeDatasetAlert: () => {}
}}>
<div>
<DatasetAlerts alerts={dataset.alerts} />
</div>
</DatasetAlertContext.Provider>
)
}
}

const publishAlert = new DatasetAlert('warning', DatasetAlertMessageKey.PUBLISH_IN_PROGRESS)
export const PublishInProgress: Story = {
render: () => {
const dataset = DatasetMother.createRealistic()

return (
<DatasetAlertContext.Provider
value={{
datasetAlerts: [publishAlert],
setDatasetAlerts: () => {},
addDatasetAlert: () => {},
removeDatasetAlert: () => {}
}}>
<div>
<DatasetAlerts alerts={dataset.alerts} />
</div>
</DatasetAlertContext.Provider>
)
}
}

export const DraftVersion: Story = {
render: () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ const expectedDataset = {
{ semanticMeaning: 'dataset', value: 'Draft' },
{ semanticMeaning: 'warning', value: 'Unpublished' }
],
alerts: [{ variant: 'warning', message: 'draftVersion', dynamicFields: undefined }],
alerts: [{ variant: 'warning', messageKey: 'draftVersion', dynamicFields: undefined }],
summaryFields: [
{
name: 'citation',
Expand Down Expand Up @@ -148,11 +148,11 @@ const expectedDatasetAlternateVersion = {
alerts: [
{
variant: 'warning',
message: 'draftVersion',
messageKey: 'draftVersion',
dynamicFields: undefined
},
{
message: 'requestedVersionNotFoundShowDraft',
messageKey: 'requestedVersionNotFoundShowDraft',
variant: 'warning',
dynamicFields: { requestedVersion: '4.0' }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ interface DatasetTranslation {
[DatasetAlertMessageKey.REQUESTED_VERSION_NOT_FOUND_SHOW_DRAFT]: AlertTranslation
[DatasetAlertMessageKey.UNPUBLISHED_DATASET]: AlertTranslation
[DatasetAlertMessageKey.SHARE_UNPUBLISHED_DATASET]: AlertTranslation
[DatasetAlertMessageKey.METADATA_UPDATED]: AlertTranslation
[DatasetAlertMessageKey.FILES_UPDATED]: AlertTranslation
[DatasetAlertMessageKey.PUBLISH_IN_PROGRESS]: AlertTranslation
[DatasetAlertMessageKey.TERMS_UPDATED]: AlertTranslation
[DatasetAlertMessageKey.DATASET_DELETED]: AlertTranslation
[DatasetAlertMessageKey.THUMBNAIL_UPDATED]: AlertTranslation
}
}

Expand Down Expand Up @@ -63,8 +69,8 @@ it('renders alerts with correct text', () => {
cy.fixture('../../../public/locales/en/dataset.json').then((dataset: DatasetTranslation) => {
cy.mount(<DatasetAlerts alerts={alerts} />)

const alertHeading = dataset.alerts[draftAlert.message].heading
const alertText = removeMarkup(dataset.alerts[draftAlert.message].alertText)
const alertHeading = dataset.alerts[draftAlert.messageKey].heading
const alertText = removeMarkup(dataset.alerts[draftAlert.messageKey].alertText)
cy.findByText(alertHeading).should('exist')
cy.findByRole('alert').should(($element) => {
// text() removes markup, so we can compare to the expected text
Expand Down