-
Notifications
You must be signed in to change notification settings - Fork 6
feat: feed submission issue labelling; refactoring utils and adding unit tests #1544
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
base: main
Are you sure you want to change the base?
Conversation
…ilityData/mobility-feed-api into feat/feed-submission-issue-labelling
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.
Pull request overview
This PR introduces new labeling functionality for feed submission GitHub issues, including "invalid" and "auth required" labels, reformatted region labels (now "region/{continent}"), and adds comprehensive unit test coverage for previously untested functions across the feed-form package.
Key Changes:
- Added URL validation functions (
isValidZipUrl,isValidZipDownload) to detect invalid feed URLs and apply the "invalid" label - Refactored code to extract
createGithubIssue,sendSlackWebhookfunctions into separate utility modules for better testability - Added unit tests for
writeToSheet,createGithubIssue,buildGithubIssueBody,sendSlackWebhook, and URL validation functions
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
functions/packages/feed-form/src/impl/utils/url-parse.ts |
New utility module with ZIP URL validation functions to determine feed validity |
functions/packages/feed-form/src/impl/utils/slack.ts |
Extracted Slack webhook functionality from main implementation file |
functions/packages/feed-form/src/impl/utils/github-issue.ts |
Extracted GitHub issue creation logic with new label assignment for auth and invalid feeds |
functions/packages/feed-form/src/impl/feed-form-impl.ts |
Removed inline functions in favor of imported utilities from new modules |
functions/packages/feed-form/src/impl/__mocks__/feed-submission-form-request-body.mock.ts |
Mock data fixture for reusable test request bodies |
functions/packages/feed-form/src/__tests__/utils/url-parse.spec.ts |
Unit tests for URL validation functions |
functions/packages/feed-form/src/__tests__/utils/slack.spec.ts |
Unit tests for Slack webhook function |
functions/packages/feed-form/src/__tests__/utils/github-issue.spec.ts |
Unit tests for GitHub issue creation and body building |
functions/packages/feed-form/src/__tests__/feed-form.spec.ts |
Added tests for writeToSheet function and refactored existing tests to use mock fixture |
| ): Promise<boolean> { | ||
| try { | ||
| if (!url) return false; | ||
| const response = await axios.head(url, {maxRedirects: 2}); |
Copilot
AI
Jan 8, 2026
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.
The maxRedirects: 2 limit may be too restrictive for legitimate feeds that use multiple redirect chains (e.g., CDN routing, authentication flows). Consider increasing this to 5 to match common HTTP client defaults while still preventing infinite redirect loops.
| const response = await axios.head(url, {maxRedirects: 2}); | |
| const response = await axios.head(url, { maxRedirects: 5 }); |
| if (!isValidZipUrl(formData.feedLink)) { | ||
| if (!await isValidZipDownload(formData.feedLink)) { | ||
| labels.push("invalid"); | ||
| } |
Copilot
AI
Jan 8, 2026
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.
Nested conditionals can be simplified by using logical operators. This would improve readability by making the logic clearer: if (!isValidZipUrl(formData.feedLink) && !await isValidZipDownload(formData.feedLink)). This expresses the intent that both checks must fail before adding the 'invalid' label.
| if (!isValidZipUrl(formData.feedLink)) { | |
| if (!await isValidZipDownload(formData.feedLink)) { | |
| labels.push("invalid"); | |
| } | |
| if (!isValidZipUrl(formData.feedLink) && !(await isValidZipDownload(formData.feedLink))) { | |
| labels.push("invalid"); |
| const formData = { | ||
| sampleRequestBodyGTFS | ||
| } as any; |
Copilot
AI
Jan 8, 2026
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.
The formData object is incorrectly structured. It creates {sampleRequestBodyGTFS: {...}} instead of spreading the properties. This should be const formData = sampleRequestBodyGTFS as any; to correctly reference the mock data.
| const formData = { | |
| sampleRequestBodyGTFS | |
| } as any; | |
| const formData = sampleRequestBodyGTFS as any; |
Summary:
This is a PR that follows from #1536. After some review comments, it was recommended that we create a new PR to add unit tests, and to remove the code that makes Github GraphQL calls (and potentially add in a future PR). The functionality of this PR remains the same as before: introducing the "invalid" label, introducing the "auth required" label, and reformatting the region label.
In terms of unit testing, this PR contains:
previously missing unit tests for:
as well as the newly added:
Also created a mock FeedSubmissionFormRequestBody.mock.ts to flexibly use in other tests.
To Discuss
David raised a concern about the "invalid" and "auth required" labels which needs to be discussed while reviewing this PR: #1536 (comment)
Expected behavior:
Screenshots of previous and new testing:

Testing tips:
Provide tips, procedures and sample files on how to test the feature.
Testers are invited to follow the tips AND to try anything they deem relevant outside the bounds of the testing tips.
Please make sure these boxes are checked before submitting your pull request - thanks!
./scripts/api-tests.shto make sure you didn't break anything