-
Notifications
You must be signed in to change notification settings - Fork 16
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
428 Show New Version Numbers in Publish Dataset Modal #473
Conversation
src/dataset/infrastructure/repositories/DatasetJSDataverseRepository.ts
Outdated
Show resolved
Hide resolved
...ata-fields-section/metadata-input-level-fields-block/input-levels-table/InputLevelsTable.tsx
Outdated
Show resolved
Hide resolved
if (releasedVersionExists && (!nextMajorVersion || !nextMinorVersion)) { | ||
console.log('Error: nextMajorVersion or nextMinorVersion is missing') | ||
return null | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this, if everything works well, this case should never happen.
and in any case we should not get to this point, if there is no released version and there is no major or minor version we should directly hide the button to publish dataset, but again I think we don't need that also.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this case should never happen, but since the two version number params are optional, I thought I should have a check just for completeness. I was trying to make the params required only if releasedVersionExists==true, but I couldn't find a clean way to do that. But I don't mind removing it
if (releasedVersionExists && (!nextMajorVersion || !nextMinorVersion)) { | ||
console.log('Error: nextMajorVersion or nextMinorVersion is missing') | ||
return null | ||
} | ||
return ( | ||
<Modal show={show} onHide={handleClose} size="lg"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Modal could be size="xl" everything fits better.
@@ -0,0 +1,39 @@ | |||
import { useEffect, useState } from 'react' | |||
import { DatasetRepository } from '../../../dataset/domain/repositories/DatasetRepository' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this hook anymore right? If that is the case I think we can delete it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, will remove it
handleClose={handleClose} | ||
/> | ||
) | ||
cy.findByText('Update Current Version').should('exist') | ||
cy.contains('Update Current Version').should('exist') | ||
}) | ||
}) | ||
describe('PublishDatasetModal', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this file we have three describe('PublishDatasetModal'
, we should only have one same describe with different cases inside.
handleClose={handleClose} | ||
/> | ||
) | ||
cy.findByText('Update Current Version').should('exist') | ||
cy.contains('Update Current Version').should('exist') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can change all the cy.contains
for:
cy.findByLabelText(/Update Current Version/).should('exist')
I think is more suitable for finding an input by its label.
@g-saracca I made the requested changes, thanks! FileJSDataverseRepository.spec.ts is failing in Github, but I can't reproduce it locally, can you try it in your local environment? |
Hi @ekraffmiller , yes, that happen to me too. Try applying this change in the cypress config👇 |
That worked, thanks! I just realized I missed some requested changes, will do that now 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekraffmiller Thanks for addressing all the changes!
I'm sorry I missed some observations in the first review.
- I think we need to set
margin-bottom: 0;
toPublishDatasetHelpText
component in.warningText
class to avoid too much spacing. - Also the CC0 link should have a target="_blank" attribute so it opens the link in a new tab.
- And could we enable the update current version checkbox now that is allowed by the use case? Or perhaps we should do it in a separate issue?
thanks @g-saracca, requested changes made 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ekraffmiller , approving!
Will be nice to add in the How To test this, a note that update current version is enabled now.
Thanks! I added testing instructions. |
I added an issue for the error that occurred in the UploadDatasetFiles unit test: #481 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approve merging commit
What this PR does / why we need it:
For Drafts of previously released versions, the Modal will show the new version numbers for Minor and Major Version updates.
This PR also enables "Update Current Version" option, because it the js-dataverse use case has been updated to allow this option
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Create a test user account in JSF
Test Publishing from "Data Curator Role"
Test Update Current Version:
Only if you are logged in as a super user, there should be a third Publish Option, "Update Current Version". To test this:
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: