Conversation
Summary of ChangesHello @zensgit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the robustness of the PLM UI by stabilizing search-related regression tests and introducing a suite of advanced features to the attendance plugin. It also includes several improvements to PLM data federation, ensuring more accurate data mapping and better error visibility. The changes are accompanied by detailed documentation and updated verification scripts. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR enhances PLM UI regression testing and extends the attendance plugin with leave, overtime, approval flow, and rotation features. It also improves PLM federation by adding BOM metadata support, product detail mapping, and error handling.
Changes:
- Wait for PLM search API response before reading results table in UI regression tests
- Add leave types, overtime rules, approval flows, and rotation rules to attendance plugin
- Enhance PLM BOM adapter to support depth, effective date, find_num, and refdes fields
- Add verification and development reports documenting changes
Reviewed changes
Copilot reviewed 32 out of 36 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/verify-plm-ui-regression.sh | Added wait for search response to prevent race condition |
| scripts/verify-plm-bom-tools.sh | Enhanced BOM seed with find_num/refdes and search reindex |
| scripts/verify-attendance-ui.mjs | Extended UI tests with leave/overtime/request flows and debug support |
| plugins/plugin-attendance/index.cjs | Major feature additions: leave types, overtime rules, approval flows, rotation scheduling |
| packages/core-backend/src/db/migrations/* | New migrations for attendance feature tables |
| packages/openapi/src/paths/attendance.yml | Added API endpoints for new attendance features |
| packages/core-backend/src/data-adapters/PLMAdapter.ts | Enhanced BOM support with metadata fields and error handling |
| docs/* | Added verification and development reports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async function waitForStatusChange(page, previousText, actionLabel) { | ||
| try { | ||
| await page.waitForFunction( | ||
| (prev) => { | ||
| const el = document.querySelector('.attendance__status') | ||
| if (!el) return false | ||
| const text = el.textContent ? el.textContent.trim() : '' | ||
| return text.length > 0 && text !== prev | ||
| }, | ||
| previousText, | ||
| { timeout: timeoutMs } | ||
| ) | ||
| return getStatusText(page) | ||
| } catch (error) { | ||
| const currentText = await getStatusText(page) | ||
| await captureDebug(page, actionLabel, { previousText, currentText }) | ||
| if (currentText && currentText === previousText) { | ||
| logInfo(`Status unchanged after ${actionLabel}; continuing with "${currentText}"`) | ||
| return currentText | ||
| } | ||
| throw error | ||
| } |
There was a problem hiding this comment.
The waitForStatusChange function catches timeout errors but continues execution if the status hasn't changed. This could mask real timeout issues. The function should distinguish between "status unchanged" (which might be acceptable in some flows) and "status element disappeared" or other timeout scenarios. Consider logging a warning or throwing an error if the status element becomes unavailable during the wait.
| if (action === 'approve') { | ||
| const canApprove = await isApproverAllowed(trx, requesterId, currentStep, logger) | ||
| if (!canApprove) { | ||
| throw new HttpError(403, 'FORBIDDEN', 'Not authorized for this approval step') | ||
| } | ||
| } |
There was a problem hiding this comment.
The approval step authorization check in line 1986 uses isApproverAllowed but only validates when action is 'approve'. However, the function should also validate authorization for 'reject' actions to ensure users can only reject requests they're authorized to approve. This could allow unauthorized users to reject requests.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fix for a flaky PLM UI test by waiting for the search response, which is a great improvement for test stability. It also adds a significant amount of new functionality to the attendance plugin, including leave/overtime requests, approval flows, and rotation scheduling, along with corresponding database migrations, API endpoints, and tests.
While the added features and test improvements are valuable, this PR is very large and combines several unrelated changes. This makes it challenging to review thoroughly and increases the risk of introducing issues. In the future, please try to split large changes into smaller, focused pull requests. For example, the PLM test fix could have been a separate PR, and the attendance feature could have been broken down into multiple PRs (e.g., DB changes, backend API, tests).
I've provided a few suggestions for refactoring to improve code maintainability. Overall, the changes are positive, but the size and scope of the PR are a concern.
| const partNumber = String( | ||
| props.item_number || props.part_number || props.number || props.code || props.internal_reference || '' | ||
| item.item_number || | ||
| props.item_number || | ||
| props.part_number || | ||
| props.number || | ||
| props.code || | ||
| props.internal_reference || | ||
| '' | ||
| ) | ||
| const revision = String(props.revision || props.version || props.rev || props.version_label || '') | ||
| const status = String(item.state || props.state || '') | ||
| const description = props.description ? String(props.description) : undefined | ||
| const createdAt = this.toIsoString(item.created_on || props.created_at || props.created_on || props.create_date) | ||
| const updatedAt = this.toIsoString(item.modified_on || props.updated_at || props.modified_on || props.write_date) || createdAt | ||
| const itemType = item.type || (props.item_type as string | undefined) || (props.itemType as string | undefined) || (props.type as string | undefined) | ||
| const itemType = | ||
| item.type || | ||
| item.item_type_id || | ||
| (props.item_type_id as string | undefined) || | ||
| (props.item_type as string | undefined) || | ||
| (props.itemType as string | undefined) || | ||
| (props.type as string | undefined) |
There was a problem hiding this comment.
The logic for resolving partNumber and itemType involves checking multiple fallback properties. This pattern is repeated in mapYuantusProductFields and mergeYuantusProductDetail. To improve code maintainability and reduce duplication, consider creating helper functions for resolving these properties from Yuantus items and their properties objects.
| context.api.http.addRoute( | ||
| 'POST', | ||
| '/api/attendance/requests', | ||
| withPermission('attendance:write', async (req, res) => { |
There was a problem hiding this comment.
The request handler for POST /api/attendance/requests has become quite large and complex due to handling multiple request types (missed_check_in, leave, overtime, etc.). To improve readability and maintainability, consider refactoring this logic. You could extract the logic for each request type into its own helper function. This would make the main handler a dispatcher and easier to follow.
Summary\n- wait for PLM search response before reading the results table in regression UI tests.\n- add verification reports and dev report.\n\n## Testing\n- AUTO_START=true PLM_BASE_URL=http://127.0.0.1:7910 bash scripts/verify-plm-ui-full.sh