Feature/99 alert mechanism #190
Conversation
There was a problem hiding this comment.
I really like the implementation of the Dataset Alerts as part of the Dataset model 🎉 I just left a few minor comments
I was also thinking about the other alerts, the ones that are not related with the Dataset Model but with certain behaviours of the user, such as 'there was an error when you clicked some button' Are we planning to implement those in the other alerts issue or should we create a new issue for this type of alerts not related with domain models?
...dataset/dataset-files/files-table/files-info/file-info-cell/file-info-data/FileDate.spec.tsx
Show resolved
Hide resolved
| ).build() | ||
| } | ||
|
|
||
| static toVersion( | ||
| jDatasetVersionId: number, | ||
| jsDatasetVersionInfo: JSDatasetVersionInfo | ||
| jsDatasetVersionInfo: JSDatasetVersionInfo, | ||
| requestedVersion?: string |
There was a problem hiding this comment.
There are some e2e tests that are checking the DatasetVersion that need to be fixed because of the added requestedVersion property
|
Hi Melina, I think I addressed all your change requests - the e2e test is failing, but in a different way on the github action than in my local environment - maybe that's because they are including different dataverse images? |
@ekraffmiller the FileEmbargoDate component tests are still failing for me Regarding the e2e tests running in the Github action, I had the same question and Guillermo told me that the Github Action is pointing to the dataverse develop branch image because ideally the backend changes should be merged before we start working on the frontend. But that's not the case right now, so some tests are failing in the GitHub action. But apparently it's ok, now the backend PR's have been given priority, so hopefully the backend will be ahead of the frontend soon So I ran the e2e locally and I'm having this error, but I already fixed it in another branch:
Then some e2e tests fail from time to time because of the wait(), Cypress doesn't like being waited for so long, sometimes it crashes, we'll have to address that at some point but for this PR, I think it's ok like this. In summary, can you take a look at the unit tests for the FileEmbargoDate component? the e2e test are ok like this |
|
Hi Melina, Gustavo suggested I create a spike for the question of the dates being returned by the API, so I created this issue IQSS/dataverse#9979. I think the locale logic is separate from the timezone issue, and we have agreement on using the user's locale, so I don't think this PR is dependent on that, but let me know what you think. |
Feature/99 alert mechanism
What this PR does / why we need it:
This PR adds logic to the JSDatasetMapper that will create DatasetAlert objects to be used in the Dataset View page
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Run the Cypress JSDatasetMapper unit test
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?:
Additional documentation: