Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive record-based support for SharePoint, enabling CRUD operations on SharePoint lists through a standardized record-based interface. The implementation treats SharePoint Lists as "tables" and List Items as "records", with proper handling of dynamic schemas and OData filtering.
Changes:
- Adds complete record-based helper functions for SharePoint (get-table-list, get-record-type, search-records, create-records, update-records, delete-records)
- Implements OData filter translation from Qore WHERE conditions with support for logical operators (AND, OR) and comparison operators (==, !=, >, >=, <, <=, contains, starts-with)
- Adds comprehensive test suite with 60+ unit and integration tests covering all CRUD operations, pagination, filtering, and edge cases
- Includes i18n translations for expression operators in English with proper type definitions
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ts/src/tests/sharepoint.test.ts | Comprehensive test suite covering unit tests for parsing, transformations, expressions, and OData filters, plus integration tests for CRUD operations |
| ts/src/i18n/i18n-types.ts | Auto-generated TypeScript type definitions for SharePoint expression translations |
| ts/src/i18n/en/apps/SharePoint/index.ts | English translations for all supported expression operators (logical, comparison, and string operators) |
| ts/src/apps/sharepoint/index.ts | Main app configuration updated to export record-based helpers and satisfy TQoreRecordBasedApp interface |
| ts/src/apps/sharepoint/helpers/record-based/update-records.ts | Sequential update implementation for SharePoint list items with WHERE filtering |
| ts/src/apps/sharepoint/helpers/record-based/search-records.ts | Iterator-based search with pagination, filtering, and sorting support |
| ts/src/apps/sharepoint/helpers/record-based/index.ts | Central export point for all record-based helper functions |
| ts/src/apps/sharepoint/helpers/record-based/get-table-list.ts | Fetches all accessible SharePoint lists across sites, excluding system lists and document libraries |
| ts/src/apps/sharepoint/helpers/record-based/get-search-options.ts | Defines search options schema with orderBy support for dynamic column sorting |
| ts/src/apps/sharepoint/helpers/record-based/get-record-type.ts | Returns dynamic schema for list items including metadata fields and column-based fields |
| ts/src/apps/sharepoint/helpers/record-based/get-expressions.ts | Defines supported filter expressions mapped to OData operators with i18n support |
| ts/src/apps/sharepoint/helpers/record-based/delete-records.ts | Sequential deletion of list items matching WHERE conditions with safety check requiring WHERE clause |
| ts/src/apps/sharepoint/helpers/record-based/create-records.ts | Sequential creation of list items with proper handling of array-valued fields using OData annotations |
| ts/src/apps/sharepoint/helpers/record-based/constants.ts | Core utilities including Graph client setup, table path parsing, type mappings, caching, and data transformations |
| ts/src/apps/sharepoint/helpers/record-based/apply-where-condition.ts | Translates Qore WHERE conditions to Microsoft Graph OData filter syntax with proper escaping and type handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .delete(); | ||
| deletedCount++; | ||
| } catch (error) { | ||
| Debugger.log(`Failed to delete item ${item.id}: ${error}`); |
There was a problem hiding this comment.
The use of Debugger.log for individual item failures is inconsistent with the pattern used in similar apps. For example, Jira uses console.warn and Shopify uses console.error for individual record failures during bulk operations. Consider using console.warn or console.error instead of Debugger.log to maintain consistency with similar record-based implementations in the codebase.
| const date = new Date(value); | ||
| if (!isNaN(date.getTime()) && value.includes('-') && value.length >= 10) { | ||
| return `'${date.toISOString()}'`; |
There was a problem hiding this comment.
The automatic date detection logic on lines 33-35 could incorrectly identify strings as dates. For example, a string like "2026-01-01-report" would be treated as a date and converted to ISO format. Consider using a more robust date detection method (e.g., strict ISO 8601 format check with regex) or relying on explicit type information from the schema to avoid misidentifying strings that happen to contain dashes and have length >= 10 as dates.
| if (column.choice.displayAs === 'checkBoxes') { | ||
| return { | ||
| type: { type: 'list', element_type: 'string' }, | ||
| allowed_values: column.choice.choices?.map((c) => ({ display_name: c, value: c })), | ||
| }; | ||
| } | ||
| return { | ||
| type: 'string', | ||
| allowed_values: column.choice.choices?.map((c) => ({ display_name: c, value: c })), | ||
| }; |
There was a problem hiding this comment.
When column.choice.choices is undefined, the allowed_values field will be set to undefined, which may not be the intended behavior. Consider only including the allowed_values field when choices is defined and non-empty, similar to how it's handled in the mapColumnToField function on line 268.
| if (column.choice.displayAs === 'checkBoxes') { | |
| return { | |
| type: { type: 'list', element_type: 'string' }, | |
| allowed_values: column.choice.choices?.map((c) => ({ display_name: c, value: c })), | |
| }; | |
| } | |
| return { | |
| type: 'string', | |
| allowed_values: column.choice.choices?.map((c) => ({ display_name: c, value: c })), | |
| }; | |
| const choices = column.choice.choices; | |
| const allowedValues = | |
| choices && choices.length > 0 | |
| ? choices.map((c) => ({ display_name: c, value: c })) | |
| : undefined; | |
| if (column.choice.displayAs === 'checkBoxes') { | |
| return allowedValues | |
| ? { | |
| type: { type: 'list', element_type: 'string' }, | |
| allowed_values: allowedValues, | |
| } | |
| : { | |
| type: { type: 'list', element_type: 'string' }, | |
| }; | |
| } | |
| return allowedValues | |
| ? { | |
| type: 'string', | |
| allowed_values: allowedValues, | |
| } | |
| : { | |
| type: 'string', | |
| }; |
| if (column.choice.displayAs === 'checkBoxes') { | ||
| return { | ||
| type: { type: 'list', element_type: 'string' }, | ||
| allowed_values: column.choice.choices?.map((c) => ({ display_name: c, value: c })), | ||
| }; | ||
| } | ||
| return { | ||
| type: 'string', | ||
| allowed_values: column.choice.choices?.map((c) => ({ display_name: c, value: c })), | ||
| }; |
There was a problem hiding this comment.
When column.choice.choices is undefined, the allowed_values field will be set to undefined. Consider only including the allowed_values field when choices is defined and non-empty, to maintain consistency with the pattern used in mapColumnToField on line 268.
| if (column.choice.displayAs === 'checkBoxes') { | |
| return { | |
| type: { type: 'list', element_type: 'string' }, | |
| allowed_values: column.choice.choices?.map((c) => ({ display_name: c, value: c })), | |
| }; | |
| } | |
| return { | |
| type: 'string', | |
| allowed_values: column.choice.choices?.map((c) => ({ display_name: c, value: c })), | |
| }; | |
| const mappedChoices = | |
| column.choice.choices?.map((c) => ({ display_name: c, value: c })) ?? []; | |
| if (column.choice.displayAs === 'checkBoxes') { | |
| const base = { | |
| type: { type: 'list', element_type: 'string' }, | |
| } as const; | |
| return mappedChoices.length > 0 | |
| ? { ...base, allowed_values: mappedChoices } | |
| : base; | |
| } | |
| const base = { | |
| type: 'string', | |
| } as const; | |
| return mappedChoices.length > 0 | |
| ? { ...base, allowed_values: mappedChoices } | |
| : base; |
… into feature/221-sharepoint-record-based-support
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.