-
Notifications
You must be signed in to change notification settings - Fork 9
feat: add explore feedback query params #1214
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
Open
nicolaskempf57
wants to merge
10
commits into
main
Choose a base branch
from
feat/add_explore_feedback_query_params
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
8885780
feat: add buildExplorerFeedbackUrl util to prefill the explorer feedb…
53bf123
feat(components): emit the selected resource from ResourceExplorer
6ca8393
feat: prefill the explorer feedback form from the banner link
8b05877
test: cover the explorer feedback link prefill end to end
8e33072
refactor: send a simplified browser/device string in navigateur_appareil
c290c17
refactor: send a simplified browser/device string in navigateur_appareil
nicolaskempf57 e1a780e
feat: use external resource url
nicolaskempf57 ee1a9fe
test: use test name to check user agent
nicolaskempf57 c8960c8
refac: use NPM package instead of custom user agent parsing
nicolaskempf57 0d5c7b4
Merge branch 'main' into feat/add_explore_feedback_query_params
nicolaskempf57 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { buildExplorerFeedbackUrl, getSimplifiedUserAgent } from '~/utils/explorer-feedback' | ||
|
|
||
| const dataset = { | ||
| id: 'ds-1', | ||
| title: 'Recensement de la population', | ||
| page: 'https://www.data.gouv.fr/fr/datasets/recensement/', | ||
| } | ||
| const resourceExternalUrl = 'https://www.data.gouv.fr/fr/datasets/recensement/?resource_id=res-1' | ||
| const resourceFormat = 'csv' | ||
| const simplifiedUserAgent = 'Firefox - desktop' | ||
|
|
||
| describe('buildExplorerFeedbackUrl', () => { | ||
| it('appends all six context params to the base URL', () => { | ||
| const url = new URL(buildExplorerFeedbackUrl('https://example.com/feedback', { dataset, resourceExternalUrl, resourceFormat, simplifiedUserAgent })) | ||
|
|
||
| expect(url.searchParams.get('dataset_id')).toBe('ds-1') | ||
| expect(url.searchParams.get('dataset_url')).toBe('https://www.data.gouv.fr/fr/datasets/recensement/') | ||
| expect(url.searchParams.get('dataset_name')).toBe('Recensement de la population') | ||
| expect(url.searchParams.get('url_ressource')).toBe('https://www.data.gouv.fr/fr/datasets/recensement/?resource_id=res-1') | ||
| expect(url.searchParams.get('format_ressource')).toBe('csv') | ||
| expect(url.searchParams.get('navigateur_appareil')).toBe(simplifiedUserAgent) | ||
| }) | ||
|
|
||
| it('keeps query params already present in the base URL', () => { | ||
| const url = new URL(buildExplorerFeedbackUrl('https://example.com/feedback?source=banner', { dataset, resourceExternalUrl, resourceFormat, simplifiedUserAgent })) | ||
|
|
||
| expect(url.searchParams.get('source')).toBe('banner') | ||
| expect(url.searchParams.get('dataset_id')).toBe('ds-1') | ||
| }) | ||
|
|
||
| it('omits resource params when no resource is resolved yet', () => { | ||
| const url = new URL(buildExplorerFeedbackUrl('https://example.com/feedback', { dataset, resourceExternalUrl: null, resourceFormat: null, simplifiedUserAgent })) | ||
|
|
||
| expect(url.searchParams.has('url_ressource')).toBe(false) | ||
| expect(url.searchParams.has('format_ressource')).toBe(false) | ||
| expect(url.searchParams.get('dataset_id')).toBe('ds-1') | ||
| }) | ||
|
|
||
| it('omits navigateur_appareil during SSR (no user agent)', () => { | ||
| const url = new URL(buildExplorerFeedbackUrl('https://example.com/feedback', { dataset, resourceExternalUrl, resourceFormat, simplifiedUserAgent: null })) | ||
|
|
||
| expect(url.searchParams.has('navigateur_appareil')).toBe(false) | ||
| }) | ||
|
|
||
| it('returns the base URL unchanged when it is not a valid absolute URL', () => { | ||
| expect(buildExplorerFeedbackUrl('/relative/path', { dataset, resourceExternalUrl, resourceFormat, simplifiedUserAgent })).toBe('/relative/path') | ||
| }) | ||
| }) | ||
|
|
||
| describe('getSimplifiedUserAgent', () => { | ||
| it('detects Firefox on desktop', () => { | ||
| expect(getSimplifiedUserAgent('Mozilla/5.0 (X11; Linux x86_64; rv:141.0) Gecko/20100101 Firefox/141.0')).toBe('Firefox - desktop') | ||
| }) | ||
|
|
||
| it('detects Chrome on desktop', () => { | ||
| expect(getSimplifiedUserAgent('Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/128.0.0.0 Safari/537.36')).toBe('Chrome - desktop') | ||
| }) | ||
|
|
||
| it('detects Safari on mobile', () => { | ||
| expect(getSimplifiedUserAgent('Mozilla/5.0 (iPhone; CPU iPhone OS 17_0 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.0 Mobile/15E148 Safari/604.1')).toBe('Safari - mobile') | ||
| }) | ||
|
|
||
| it('detects Edge on tablet', () => { | ||
| expect(getSimplifiedUserAgent('Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/128.0.0.0 Safari/537.36 Edg/128.0.0.0 Tablet PC')).toBe('Edge - tablet') | ||
| }) | ||
|
|
||
| it('detects Android phone as mobile', () => { | ||
| expect(getSimplifiedUserAgent('Mozilla/5.0 (Linux; Android 14; Pixel 7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/128.0.0.0 Mobile Safari/537.36')).toBe('Chrome - mobile') | ||
| }) | ||
|
|
||
| it('detects Android tablet without an explicit Tablet token', () => { | ||
| expect(getSimplifiedUserAgent('Mozilla/5.0 (Linux; Android 14; SM-X910) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/128.0.0.0 Safari/537.36')).toBe('Chrome - tablet') | ||
| }) | ||
|
|
||
| it('falls back to Other for unknown browsers', () => { | ||
| expect(getSimplifiedUserAgent('Mozilla/5.0 (X11; Linux x86_64) SomeUnknownBrowser/1.0')).toBe('Other - desktop') | ||
| }) | ||
| }) |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { test, expect } from '../base' | ||
| import { createDatasetWithRemoteResources, deleteDatasets, enableNewExplorer } from '../helpers' | ||
|
|
||
| const createdDatasets: Array<string> = [] | ||
|
|
||
| test.afterEach(async ({ page, request }) => { | ||
| await page.context().clearCookies({ name: 'new_explorer' }) | ||
| await deleteDatasets(request, createdDatasets) | ||
| }) | ||
|
|
||
| test('the feedback link pre-fills the form with the current context', async ({ page, request }) => { | ||
| const { dataset, resources } = await createDatasetWithRemoteResources(request, `Test explorer feedback ${Date.now()}`, ['Fichier numero 01']) | ||
| createdDatasets.push(dataset.id) | ||
| const resource = resources[0]! | ||
| const resourceQuery = `?resource_id=${resource.id}` | ||
|
|
||
| await enableNewExplorer(page, `/datasets/${dataset.id}${resourceQuery}`) | ||
|
|
||
| const link = page.getByRole('link', { name: 'Donner votre avis' }) | ||
| // The resource params only appear once the explorer has forwarded its | ||
| // selection client-side, so let the assertion retry instead of reading the | ||
| // SSR href once. | ||
| await expect(link).toHaveAttribute('href', /url_ressource=/) | ||
|
|
||
| const hrefAttribute = await link.getAttribute('href') | ||
| expect(hrefAttribute).toBeTruthy() | ||
| const href = new URL(hrefAttribute!) | ||
| expect(href.searchParams.get('dataset_id')).toBe(dataset.id) | ||
| expect(href.searchParams.get('dataset_name')).toBe(dataset.title) | ||
| expect(href.searchParams.get('dataset_url')).toContain(`/datasets/${dataset.slug}`) | ||
| expect(href.searchParams.get('url_ressource')).toBe(`${href.searchParams.get('dataset_url')}${resourceQuery}`) | ||
| expect(href.searchParams.get('format_ressource')).toBe('csv') | ||
| // The spec runs on both chromium and firefox projects. | ||
| const expectedBrowser = test.info().project.name === 'firefox' ? 'Firefox' : 'Chrome' | ||
| expect(href.searchParams.get('navigateur_appareil')).toBe(`${expectedBrowser} - desktop`) | ||
| }) |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import type { DatasetV2 } from '@datagouv/components-next' | ||
|
|
||
| export type ExplorerFeedbackContext = { | ||
| dataset: Pick<DatasetV2, 'id' | 'page' | 'title'> | ||
| // The selected resource's canonical permalink (getResourceExternalUrl), not | ||
| // its raw file URL. | ||
| resourceExternalUrl: string | null | ||
| // The selected resource's format; only set together with resourceExternalUrl. | ||
| resourceFormat: string | null | ||
| simplifiedUserAgent: string | null | ||
| } | ||
|
|
||
| // Returns a human-readable "Browser - device" string from a raw user agent. | ||
| // Detection is intentionally simple: the form owner only needs a rough idea of | ||
| // the visitor's environment, not a full parser. | ||
| export function getSimplifiedUserAgent(userAgent: string): string { | ||
| const ua = userAgent.toLowerCase() | ||
|
|
||
| let browser = 'Other' | ||
| if (ua.includes('firefox/')) { | ||
| browser = 'Firefox' | ||
| } | ||
| else if (ua.includes('edg/')) { | ||
| browser = 'Edge' | ||
| } | ||
| else if (ua.includes('chrome/') || ua.includes('chromium/')) { | ||
| browser = 'Chrome' | ||
| } | ||
| else if (ua.includes('safari/')) { | ||
| browser = 'Safari' | ||
| } | ||
| else if (ua.includes('opera/') || ua.includes('opr/')) { | ||
| browser = 'Opera' | ||
| } | ||
|
|
||
| let device = 'desktop' | ||
| if (ua.includes('ipad') || ua.includes('tablet')) { | ||
| device = 'tablet' | ||
| } | ||
| else if (ua.includes('mobile')) { | ||
| device = 'mobile' | ||
| } | ||
| else if (ua.includes('android')) { | ||
| // Android tablets usually omit both "Mobile" and "Tablet". | ||
| device = 'tablet' | ||
| } | ||
|
|
||
| return `${browser} - ${device}` | ||
| } | ||
|
nicolaskempf57 marked this conversation as resolved.
|
||
|
|
||
| // Builds the banner's feedback-form URL with the current context pre-filled as | ||
| // query params, so the form owner knows what the visitor was looking at. Resource | ||
| // params are omitted (not sent empty) until the explorer resolves its selection, | ||
| // and the simplified user agent only exists client-side. | ||
| export function buildExplorerFeedbackUrl(baseUrl: string, { dataset, resourceExternalUrl, resourceFormat, simplifiedUserAgent }: ExplorerFeedbackContext): string { | ||
| let url: URL | ||
| try { | ||
| url = new URL(baseUrl) | ||
| } | ||
| catch { | ||
| // Misconfigured base URL: better an un-prefilled form than a broken page. | ||
| return baseUrl | ||
| } | ||
| url.searchParams.set('dataset_id', dataset.id) | ||
| url.searchParams.set('dataset_url', dataset.page) | ||
| url.searchParams.set('dataset_name', dataset.title) | ||
| if (resourceExternalUrl) { | ||
| url.searchParams.set('url_ressource', resourceExternalUrl) | ||
| } | ||
| if (resourceFormat) { | ||
| url.searchParams.set('format_ressource', resourceFormat) | ||
| } | ||
| if (simplifiedUserAgent) { | ||
| url.searchParams.set('navigateur_appareil', simplifiedUserAgent) | ||
| } | ||
| return url.toString() | ||
| } | ||
Oops, something went wrong.
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.
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 it currently doesn't show in the fields in the form. As discussed, I think it would be fair to be transparent about the context information being sent
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.
@agarrone regarding the form fields visibility