Show terms modal before download when dataset has custom terms or non-default license#956
Draft
ChengShi-1 wants to merge 1 commit intodevelopfrom
Conversation
github-actions
bot
added
FY26 Sprint 21
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the “download guard” behavior so that users must accept dataset terms before starting a download when a dataset has custom terms, a non-default license (not CC0 1.0), or a guestbook—while continuing to bypass the modal for draft datasets and dataset editors.
Changes:
- Generalized the download gating condition from “guestbook exists” to “terms/custom license/guestbook requires acceptance” across dataset, file, and bulk-download entry points.
- Renamed/extended the modal component to
DownloadWithTermsAndGuestbookModaland adjusted the form/hook logic to support “custom terms only” scenarios. - Added/updated component + e2e tests and updated the changelog.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e-integration/shared/datasets/DatasetHelper.ts | Adds helper to set dataset custom terms via API for e2e setup. |
| tests/e2e-integration/e2e/sections/file/File.spec.tsx | Adds e2e coverage for file downloads triggering terms modal for guests but not editors. |
| tests/e2e-integration/e2e/sections/dataset/Dataset.spec.tsx | Adds e2e coverage for dataset ZIP downloads triggering terms modal for guests but not editors; refactors /spa base path. |
| tests/component/sections/file/file-action-buttons/access-file-menu/FileTabularDownloadOptions.spec.tsx | Updates props to match new “requires terms/guestbook” gating. |
| tests/component/sections/file/file-action-buttons/access-file-menu/FileNonTabularDownloadOptions.spec.tsx | Updates props to match new “requires terms/guestbook” gating. |
| tests/component/sections/file/file-action-buttons/access-file-menu/AccessFileMenu.spec.tsx | Adds component coverage for terms-only modal and bypass behavior (draft/editor). |
| tests/component/sections/dataset/dataset-files/guestbook/DownloadWithTermsAndGuestbookModal.spec.tsx | Renames and extends modal tests to cover “custom terms only” behavior. |
| tests/component/sections/dataset/dataset-files/files-table/file-actions/download-files/DownloadFilesButton.spec.tsx | Adds bulk-download tests ensuring modal appears for custom terms and bypasses for editors. |
| tests/component/sections/dataset/dataset-action-buttons/access-dataset-menu/AccessDatasetMenu.spec.tsx | Adds dataset menu tests for custom terms and non-default license triggering modal + bypass cases. |
| src/stories/guestbooks/guestbook-applied-modal/DownloadWithTermsAndGuestbookModal.stories.tsx | Updates story to new modal component name and storybook metadata. |
| src/sections/file/file-action-buttons/access-file-menu/FileTabularDownloadOptions.tsx | Renames prop to requiresTermsOrGuestbook and uses it to decide modal vs direct download. |
| src/sections/file/file-action-buttons/access-file-menu/FileNonTabularDownloadOptions.tsx | Renames prop to requiresTermsOrGuestbook and uses it to decide modal vs direct download. |
| src/sections/file/file-action-buttons/access-file-menu/FileDownloadOptions.tsx | Implements new gating logic (custom terms/non-default license/guestbook) for file downloads and swaps modal component. |
| src/sections/dataset/dataset-files/files-table/file-actions/file-actions-cell/file-action-buttons/file-options-menu/useGuestbookCollectSubmission.ts | Simplifies submission handler to support modal flows without a loaded guestbook object. |
| src/sections/dataset/dataset-files/files-table/file-actions/file-actions-cell/file-action-buttons/file-options-menu/GuestbookCollectForm.tsx | Hides guestbook fields when no guestbook is present; updates stylesheet import. |
| src/sections/dataset/dataset-files/files-table/file-actions/file-actions-cell/file-action-buttons/file-options-menu/DownloadWithTermsAndGuestbookModal.tsx | Renames modal component and enables operation when no guestbook exists (terms/license-only). |
| src/sections/dataset/dataset-files/files-table/file-actions/file-actions-cell/file-action-buttons/file-options-menu/DownloadWithTermsAndGuestbookModal.module.scss | Adds styling module used by the updated terms/guestbook UI. |
| src/sections/dataset/dataset-files/files-table/file-actions/download-files/DownloadFilesButton.tsx | Extends bulk-download flow to show modal for custom terms/non-default license; passes license/terms into modal. |
| src/sections/dataset/dataset-action-buttons/access-dataset-menu/AccessDatasetMenu.tsx | Extends dataset ZIP download flow to show modal for custom terms/non-default license; passes license/terms into modal. |
| CHANGELOG.md | Documents the expanded conditions that trigger the terms modal and bypass rules. |
Comments suppressed due to low confidence (1)
src/sections/dataset/dataset-files/files-table/file-actions/file-actions-cell/file-action-buttons/file-options-menu/DownloadWithTermsAndGuestbookModal.tsx:60
- With this PR the modal can open even when
guestbookIdis undefined (custom terms / non-default license only). In that case the form fields are hidden, butprefilledAccountFieldValuesstill populatesformValuesfor authenticated users andbuildGuestbookResponse()will submit name/email/etc on Accept. This can unintentionally send PII to the access API when no guestbook is configured. Consider clearing/avoiding identity field prefills and submitting an empty guestbook response whenhasGuestbookis false.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+69
to
+75
| const bypassTermsGuard = resolvedIsDraftDataset || resolvedCanEdit | ||
| const hasGuestbook = guestbookId !== undefined | ||
| const hasNonDefaultLicense = | ||
| resolvedDatasetLicense !== undefined && resolvedDatasetLicense.name !== defaultLicense.name | ||
| const hasCustomTerms = resolvedDatasetCustomTerms !== undefined | ||
| const shouldShowModal = | ||
| !bypassTermsGuard && (hasGuestbook || hasCustomTerms || hasNonDefaultLicense) |
There was a problem hiding this comment.
hasGuestbook is derived only from the guestbookId prop, but on dataset pages the AccessFileMenu is rendered without a guestbookId prop (it relies on useDataset() context). This makes shouldShowModal false for “guestbook-only” datasets and bypasses the guestbook/terms flow. Consider resolving the guestbook id from props or dataset context (e.g., guestbookId ?? dataset?.guestbookId) and pass that resolved value down to the modal.
|
coverage: 97.569% (+0.005%) from 97.564% — 938-dataset-terms-modal-should-be-showed-when-download-files-with-custom-terms into develop |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how 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 or changelog update needed for this change?:
Additional documentation: