Dataset Page: Hide deaccession button if dataset is deaccessioned or draft

[ChengShi-1]

What this PR does / why we need it:

Which issue(s) this PR closes:

• Closes Dataset Page: Deaccession Button appears incorrectly #666

Special notes for your reviewer:

• change the test cy.wrap(repository.deaccession).should('be.calledTwice') to cy.wrap(repository.deaccession).should('have.been.called') because the original test is flaky in git environment. with the error messageTimed out retrying after 4000ms: expected deaccession to have been called exactly twice, but it was called once
• remove the version.length == 0, since the logic seems duplicated with isSomeVersionRelease

Suggestions on how to test this:

if the dataset version is draft or is deaccessioned, should not show Deaccession Dataset Bbutton

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:

[coveralls]

coverage: 97.383% (-0.4%) from 97.813%
when pulling c1e7293 on 666-dataset-page-deaccession-button-appears-incorrectly
into 4684edf on develop.

[ekraffmiller]

Looks good! I just have a suggestion for simplifying the logic

[ekraffmiller]

Hi @ChengShi-1 , I'm sorry, I was testing this against JSF and I realized this issue doesn't describe the logic correctly!
The Deaccession Dataset button should be displayed if there is any 'released' (published) version of the Dataset, even if the current version of the Dataset being viewed is not released.
So the button should be displayed if there are any publishedVersions, which are defined in this line:

  const publishedVersions =
    dataset.versionsSummaries?.filter(
      (version) => version.publishedOn && version.summary !== 'versionDeaccessioned'
    ) || []

But this logic needs to updated with the Deaccession Reason PR is merged (#664), because the deaccession info will be an object. So maybe it's best to keep this waiting until that PR is merged.

Sorry for the confusion on this! Let me know if you have questions

[ChengShi-1]

Hi @ChengShi-1 , I'm sorry, I was testing this against JSF and I realized this issue doesn't describe the logic correctly! The Deaccession Dataset button should be displayed if there is any 'released' (published) version of the Dataset, even if the current version of the Dataset being viewed is not released. So the button should be displayed if there are any publishedVersions, which are defined in this line:

  const publishedVersions =
    dataset.versionsSummaries?.filter(
      (version) => version.publishedOn && version.summary !== 'versionDeaccessioned'
    ) || []

But this logic needs to updated with the Deaccession Reason PR is merged (#664), because the deaccession info will be an object. So maybe it's best to keep this waiting until that PR is merged.

Sorry for the confusion on this! Let me know if you have questions

Hi Ellen, Thanks for review! Could you please check #669 ? It solved this issue here I think. I didn't notice this before, it was my bad.

[ekraffmiller]

Hi @ChengShi-1 , I'm sorry, I was testing this against JSF and I realized this issue doesn't describe the logic correctly! The Deaccession Dataset button should be displayed if there is any 'released' (published) version of the Dataset, even if the current version of the Dataset being viewed is not released. So the button should be displayed if there are any publishedVersions, which are defined in this line:

  const publishedVersions =
    dataset.versionsSummaries?.filter(
      (version) => version.publishedOn && version.summary !== 'versionDeaccessioned'
    ) || []

But this logic needs to updated with the Deaccession Reason PR is merged (#664), because the deaccession info will be an object. So maybe it's best to keep this waiting until that PR is merged.
Sorry for the confusion on this! Let me know if you have questions

Hi Ellen, Thanks for review! Could you please check #669 ? It solved this issue here I think. I didn't notice this before, it was my bad.

Ok, great! I will review that, and then we can probably close this. Thanks :)

[ekraffmiller]

This issue was resolved in #669