feat(skilavottord): Add municipality #17109
Conversation
WalkthroughThis pull request introduces a comprehensive update to the Skilavottord application, focusing on adding municipality-related functionality. The changes span multiple components and modules, introducing a new Changes
Sequence DiagramsequenceDiagram
participant User
participant AuthSystem
participant RecyclingPartnerService
participant AccessControlService
User->>AuthSystem: Login with role
AuthSystem->>User: Validate role
alt Municipality Role
User->>RecyclingPartnerService: Request Partners
RecyclingPartnerService->>User: Return Municipality Partners
else Other Roles
User->>RecyclingPartnerService: Request Partners
RecyclingPartnerService->>User: Return Authorized Partners
end
User->>AccessControlService: Request Access Control
AccessControlService->>User: Return Permitted Actions
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (34)
apps/skilavottord/web/screens/AccessControl/components/AccessControlModal/AccessControlModal.tsx (5)
7-7: Remove duplicate Option importThe
Optiontype is imported individually and also as part of the destructured import from '@island.is/island-ui/core'.-import { Box, Button, Option, Select, Stack } from '@island.is/island-ui/core' +import { Box, Button, Select, Stack } from '@island.is/island-ui/core'
Line range hint
52-56: Clarify the empty onContinue handlerThe empty onContinue handler in the Modal component could be confusing. Consider removing it if not needed, as the form submission is handled by the onSubmit prop.
- onContinue={() => { - // Intentionally left empty - }} + onContinue={undefined}
Line range hint
124-131: Simplify email validation patternThe current email validation regex is complex and might miss some valid email formats. Consider using a simpler pattern or a dedicated email validation library.
pattern: { - value: - /^[\w!#$%&'*+/=?`{|}~^-]+(?:\.[\w!#$%&'*+/=?`{|}~^-]+)*@(?:[A-Z0-9-]+\.)+[A-Z]{2,6}$/i, + value: /^[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}$/i, message: t.modal.inputs.email.rules?.validate, },
Line range hint
196-207: Improve responsive design implementationThe Box component uses hardcoded padding values which might not scale well across different screen sizes. Consider using theme-based spacing values for better maintainability.
- <Box paddingX={[3, 3, 3, 15]}></Box> + <Box paddingX={[2, 3, 4, 5]}></Box>
Line range hint
30-209: Add loading state handlingThe form submission lacks loading state handling, which could lead to multiple submissions. Consider adding a loading state to disable the form during submission.
You could:
- Add a loading prop to the component
- Disable the submit button and form inputs during submission
- Show a loading indicator
Would you like me to provide a detailed implementation?
apps/skilavottord/ws/seeders/20201010211010-seed-recycling-request.js (1)
Line range hint
1-96: Consider adding documentation for test data scenariosThe seeder file contains various test scenarios (different request_types and recycling_partner_id states) but lacks documentation explaining the purpose of each scenario. Consider adding comments to explain:
- The significance of null vs non-null recycling_partner_id
- The progression of request_types
- The relationship between dates and states
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (3)
55-70: Simplify conditional logic for route variablesThe logic setting
breadcrumbTitle,title,info,activeSection, androutecan be simplified for better readability and maintainability.Consider refactoring as follows:
const isMunicipality = router.route === routes.municipalities.add const { t: { recyclingCompanies: t, municipalities: mt, routes }, } = useI18n() const breadcrumbTitle = isMunicipality ? mt.title : t.title const title = isMunicipality ? mt.municipality.add.title : t.recyclingCompany.add.title const info = isMunicipality ? mt.municipality.add.info : t.recyclingCompany.add.info const activeSection = isMunicipality ? 1 : 2 const route = isMunicipality ? routes.municipalities.baseRoute : routes.recyclingCompanies.baseRoute
117-125: Duplicate code in navigation after creationThe routing logic after a successful creation is duplicated for municipalities and recycling companies.
Refactor to eliminate duplication:
if (!errors) { - if (isMunicipality) { - router.push(routes.municipalities.baseRoute).then(() => { - toast.success(t.recyclingCompany.add.added) - }) - } else { - router.push(routes.recyclingCompanies.baseRoute).then(() => { - toast.success(t.recyclingCompany.add.added) - }) - } + router.push(route).then(() => { + toast.success(t.recyclingCompany.add.added) + }) }
129-135: SimplifyhandleCancelrouting logicThe
handleCancelfunction contains duplicated routing logic based onisMunicipality.Refactor to use the
routevariable:const handleCancel = () => { - if (isMunicipality) { - router.push(routes.municipalities.baseRoute) - } else { - router.push(routes.recyclingCompanies.baseRoute) - } + router.push(route) }apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanies.tsx (2)
59-72: Simplify context-based variablesThe variables
isMunicipality,buttonText,activeSection,title, andinfocan be consolidated for clearer logic.Consider refactoring:
const isMunicipality = router.route === routes.municipalities.baseRoute const buttonText = isMunicipality ? t.buttons.addMunicipality : t.buttons.add const activeSection = isMunicipality ? 1 : 2 const title = isMunicipality ? mt.title : t.title const info = isMunicipality ? '' : t.info
101-109: Duplicate code inhandleCreatefunctionThe
handleCreatefunction contains duplicated routing logic.Refactor to use the
routesobject based onisMunicipality:const handleCreate = () => { router.push({ - pathname: isMunicipality ? routes.municipalities.add : routes.recyclingCompanies.add, + pathname: route.add, }) }Ensure you define
routesimilarly to how it's done in other components.apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (3)
53-57: Avoid unnecessary default assignmentThe default assignment
|| []is unnecessary when usingskipin the query.Remove the redundant assignment.
const { data } = - useQuery<Query>(SkilavottordAllRecyclingPartnersByTypeQuery, { + useQuery<Query>(SkilavottordAllRecyclingPartnersByTypeQuery, { skip: isMunicipality, variables: { isMunicipality: true }, }) - || []
278-311: Conditional rendering might cause layout shiftConditional rendering of the
Selectcomponent may cause layout shifts. Consider rendering the component but disabling it or hiding it with CSS to maintain consistent layout.Alternatively, adjust the layout to accommodate the dynamic content.
374-375: Button icon mismatchThe
Buttoncomponent's icon has been changed tosave. Ensure this aligns with the action, and consider using thearrowForwardicon for consistency unless saving is appropriate here.apps/skilavottord/web/screens/AccessControl/AccessControl.tsx (1)
125-147: Unused imports and variablesThere are unused imports and variables, such as
municipalities,usefrom 'dd-trace', and potentially others.Remove unused imports and variables to clean up the code.
apps/skilavottord/web/pages/en/municipalities/index.tsx (1)
1-5: Consider improving type safety and import pathsWhile the implementation is functional, consider these improvements:
- Define the Screen type explicitly on the RecyclingCompanies component instead of using type casting
- Consider using path aliases for imports to improve maintainability
Example implementation:
- import { Screen } from '@island.is/skilavottord-web/types' + import { Screen } from '@types' - import { withLocale } from '@island.is/skilavottord-web/i18n' + import { withLocale } from '@i18n' - import { RecyclingCompanies } from '@island.is/skilavottord-web/screens' + import { RecyclingCompanies } from '@screens' - export default withLocale('en')(RecyclingCompanies as Screen) + export default withLocale('en')<Screen>(RecyclingCompanies)apps/skilavottord/web/pages/en/municipalities/add.tsx (1)
1-5: Apply consistent type safety improvementsSimilar to the index page, consider improving type safety and import paths:
Example implementation:
- import { Screen } from '@island.is/skilavottord-web/types' + import { Screen } from '@types' - import { withLocale } from '@island.is/skilavottord-web/i18n' + import { withLocale } from '@i18n' - import { RecyclingCompanyCreate } from '@island.is/skilavottord-web/screens/RecyclingCompanies/RecyclingCompanyCreate' + import { RecyclingCompanyCreate } from '@screens/RecyclingCompanies/Create' - export default withLocale('en')(RecyclingCompanyCreate as Screen) + export default withLocale('en')<Screen>(RecyclingCompanyCreate)apps/skilavottord/web/pages/en/municipalities/[id].tsx (1)
1-5: Consider creating a shared page componentThe code is identical to the Icelandic version except for the locale parameter. Consider creating a shared page component to reduce duplication:
+ // shared/MunicipalityPage.tsx + import { Screen } from '@island.is/skilavottord-web/types' + import { withLocale } from '@island.is/skilavottord-web/i18n' + import { RecyclingCompanyUpdate } from '@island.is/skilavottord-web/screens/RecyclingCompanies/RecyclingCompanyUpdate' + + export const createMunicipalityPage = (locale: string) => + withLocale(locale)(RecyclingCompanyUpdate as Screen)Then both locale pages could simply import and use this shared component:
import { createMunicipalityPage } from '../shared/MunicipalityPage' export default createMunicipalityPage('en') // or 'is'apps/skilavottord/ws/src/app/modules/auth/user.model.ts (1)
Line range hint
1-28: Consider adding role documentationThe Role enum would benefit from JSDoc comments describing the permissions and responsibilities of each role, especially the newly added municipality role.
export enum Role { + /** System administrator with full access */ developer = 'developer', + /** Company that processes recycled vehicles */ recyclingCompany = 'recyclingCompany', + /** Administrative user for recycling companies */ recyclingCompanyAdmin = 'recyclingCompanyAdmin', + /** Recycling fund administrator */ recyclingFund = 'recyclingFund', + /** Regular user accessing the system */ citizen = 'citizen', + /** Municipality administrator with specific access rights */ municipality = 'municipality', }apps/skilavottord/web/components/PageHeader/PageHeader.tsx (2)
11-14: Consider enhancing props interface documentationThe props interface could be more descriptive to improve maintainability.
-export const PageHeader: FC<{ title: string; info: string }> = ({ +interface PageHeaderProps { + /** Main heading text for the page */ + title: string; + /** Additional information to display in tooltip */ + info: string; +} + +export const PageHeader: FC<PageHeaderProps> = ({ title, info, }) => {
17-26: Consider layout shift optimizationThe current order switching between mobile and desktop might cause layout shift during page load.
Consider using CSS Grid's
grid-template-areasinstead of order properties for a more stable layout that doesn't cause Content Layout Shift (CLS).apps/skilavottord/web/utils/roleUtils.ts (2)
18-19: Enhance default case handlingThe default case using
startCasemight not be ideal for all future role values.- default: - return startCase(role) + default: { + console.warn(`Translation missing for role: ${role}`); + return startCase(role); + }
Missing test coverage for role translations
The codebase lacks dedicated test coverage for the
getRoleTranslationutility function. While there is a test file in the web directory (locales.spec.ts), it doesn't cover the role translations. The function handles critical UI text for different user roles, including the newly added municipality role, and should be properly tested for both Icelandic and English translations.Suggested actions:
- Create a new test file
roleUtils.spec.tsinapps/skilavottord/web/utils/- Add test cases covering all role translations in both languages
- Include specific test cases for the new municipality role translations ('Sveitarfélag'/'Municipality')
🔗 Analysis chain
Line range hint
6-20: Verify test coverage for new role translationThe new municipality role translation needs test coverage to ensure correct behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing test files and their coverage fd -e test.ts -e spec.ts roleUtils apps/skilavottord/web/utils/Length of output: 65
Script:
#!/bin/bash # Let's try a broader search for test files in the web directory fd -e test.ts -e spec.ts apps/skilavottord/web # Also search for any usage of getRoleTranslation in test files rg "getRoleTranslation" -t ts --glob "*test*" --glob "*spec*" # Search for any test files containing "role" or "Role" to ensure we don't miss any rg "role|Role" -t ts --glob "*test*" --glob "*spec*" apps/skilavottord/webLength of output: 577
Script:
#!/bin/bash # Search for test files in the correct way fd . apps/skilavottord/web -e test.ts -e spec.ts # Look for any test files containing Role enum ast-grep --pattern 'enum Role { $$$ }' # Search for any files importing roleUtils to see test coverage rg -l "from ['|\"].*roleUtils"Length of output: 1261
apps/skilavottord/web/components/NavigationLinks/NavigationLinks.tsx (2)
29-30: Remove unnecessary template literalsTemplate literals are used unnecessarily for static string values. Since these strings don't contain any interpolation, they can be simplified.
- title: `${sidenavText.recycled}`, - link: `${routes.recycledVehicles}`, + title: sidenavText.recycled, + link: routes.recycledVehicles,Apply similar changes to other occurrences.
Also applies to: 34-35, 40-41, 45-46
7-11: Define interface for component propsConsider extracting the props interface for better type documentation and reusability.
+interface NavigationLinksProps { + activeSection: number; +} + -export const NavigationLinks = ({ - activeSection, -}: { - activeSection: number -}) => { +export const NavigationLinks = ({ activeSection }: NavigationLinksProps) => {apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.model.ts (2)
108-108: Standardize comment styleThe inline comment style differs from the rest of the file. Consider removing it since the
@Fielddecorator withnullable: trueis self-documenting.- @Field({ nullable: true }) // Ensure this field is nullable in GraphQL + @Field({ nullable: true })
101-115: Consider grouping related fieldsThe new municipality-related fields could be grouped with other identifying fields at the top of the model for better organization.
Consider moving these fields after the
companyIdfield to keep related identifying fields together.apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts (1)
32-32: Remove redundant commentThe comment restates what's evident from the code and parameter names.
- // If Role.municipality, return all recycling partners that are not municipalitiesapps/skilavottord/ws/src/app/modules/vehicle/vehicle.resolver.ts (1)
21-32: Consider improving role-based filtering logicWhile the role-based filtering is functionally correct, the logic could be more explicit and maintainable.
Consider refactoring to:
- partnerId: user.role === Role.municipality ? user.partnerId : null, + partnerId: user.role === Role.municipality + ? user.partnerId + : (user.role === Role.recyclingFund ? null : user.partnerId),This makes the intent clearer for each role and improves maintainability.
apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts (1)
Line range hint
43-65: Consider implementing pagination optimizationThe current implementation might face performance issues with large datasets, especially when filtering across multiple partners.
Consider these optimizations:
- Add database indexes for frequently filtered columns
- Implement cursor-based pagination instead of offset-based
- Consider caching frequently accessed partner relationships
apps/skilavottord/ws/src/app/modules/accessControl/accessControl.resolver.ts (1)
20-25: Consider organizing roles by privilege levelThe roles array would be more maintainable if organized by privilege level.
Consider reorganizing roles:
roles: [ Role.developer, Role.recyclingFund, + Role.municipality, Role.recyclingCompanyAdmin, - Role.municipality, ],apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (1)
149-151: Add type validation for municipalityId.The current type checking could be more robust.
- if (typeof input.municipalityId !== 'string') { - input.municipalityId = input.municipalityId?.value || '' - } + input.municipalityId = typeof input.municipalityId === 'string' + ? input.municipalityId + : input.municipalityId?.value || ''apps/skilavottord/web/i18n/locales/is.json (2)
368-372: Ensure consistent navigation structure.The new municipality-related entries in the sidenav section should maintain hierarchy consistency with other sections.
Consider this structure for better consistency:
"recyclingFundSidenav": { "title": "Úrvinnslusjóður", - "municipalityTitle": "Sveitarfélag", "recycled": "Meðhöndluð ökutæki", "companies": "Móttökuaðilar", "accessControl": "Aðgangsstýring", "municipalities": "Sveitarfélög" + "municipality": { + "title": "Sveitarfélag" + } }
415-425: Improve form field labels and placeholders.The form input labels and placeholders for company ID and name could be more descriptive.
Consider these improvements:
"companyId": { - "label": "Auðkenni (ID)", - "placeholder": "Auðkenni", + "label": "Auðkenni fyrirtækis (ID)", + "placeholder": "Sláðu inn einkvæmt auðkenni", "rules": { "required": "Skylda er að fylla út auðkenni" } }, "companyName": { - "label": "Nafn", - "placeholder": "Nafn", + "label": "Nafn fyrirtækis", + "placeholder": "Sláðu inn nafn fyrirtækis", "rules": { "required": "Skylda er að fylla út nafn" } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (33)
apps/skilavottord/web/auth/utils.ts(1 hunks)apps/skilavottord/web/components/NavigationLinks/NavigationLinks.tsx(1 hunks)apps/skilavottord/web/components/PageHeader/PageHeader.tsx(1 hunks)apps/skilavottord/web/components/Sidenav/Sidenav.tsx(3 hunks)apps/skilavottord/web/i18n/locales/en.json(5 hunks)apps/skilavottord/web/i18n/locales/is.json(5 hunks)apps/skilavottord/web/i18n/locales/translation.d.ts(6 hunks)apps/skilavottord/web/pages/en/municipalities/[id].tsx(1 hunks)apps/skilavottord/web/pages/en/municipalities/add.tsx(1 hunks)apps/skilavottord/web/pages/en/municipalities/index.tsx(1 hunks)apps/skilavottord/web/pages/municipalities/[id].tsx(1 hunks)apps/skilavottord/web/pages/municipalities/add.tsx(1 hunks)apps/skilavottord/web/pages/municipalities/index.tsx(1 hunks)apps/skilavottord/web/screens/AccessControl/AccessControl.tsx(8 hunks)apps/skilavottord/web/screens/AccessControl/components/AccessControlModal/AccessControlModal.tsx(1 hunks)apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanies.tsx(3 hunks)apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx(5 hunks)apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx(8 hunks)apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx(5 hunks)apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx(4 hunks)apps/skilavottord/web/utils/roleUtils.ts(1 hunks)apps/skilavottord/ws/migrations/202412013210715-addcolumns-recycling-partners-municipality.js(1 hunks)apps/skilavottord/ws/seeders/20201010211010-seed-recycling-request.js(6 hunks)apps/skilavottord/ws/src/app/modules/accessControl/accessControl.resolver.ts(2 hunks)apps/skilavottord/ws/src/app/modules/accessControl/accessControl.service.ts(2 hunks)apps/skilavottord/ws/src/app/modules/auth/auth.guard.ts(1 hunks)apps/skilavottord/ws/src/app/modules/auth/user.model.ts(1 hunks)apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.input.ts(2 hunks)apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.model.ts(1 hunks)apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts(4 hunks)apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts(2 hunks)apps/skilavottord/ws/src/app/modules/vehicle/vehicle.resolver.ts(1 hunks)apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (33)
apps/skilavottord/web/pages/en/municipalities/index.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/pages/municipalities/index.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/pages/municipalities/[id].tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/pages/en/municipalities/[id].tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/pages/en/municipalities/add.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/components/PageHeader/PageHeader.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/auth/user.model.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/components/NavigationLinks/NavigationLinks.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/accessControl/accessControl.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/pages/municipalities/add.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/accessControl/accessControl.resolver.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/migrations/202412013210715-addcolumns-recycling-partners-municipality.js (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.model.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/auth/auth.guard.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControl/components/AccessControlModal/AccessControlModal.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/utils/roleUtils.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/components/Sidenav/Sidenav.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.input.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanies.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/seeders/20201010211010-seed-recycling-request.js (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/auth/utils.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/i18n/locales/is.json (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControl/AccessControl.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/vehicle/vehicle.resolver.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/i18n/locales/en.json (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/i18n/locales/translation.d.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (32)
apps/skilavottord/web/screens/AccessControl/components/AccessControlModal/AccessControlModal.tsx (1)
Line range hint 13-28: Well-structured interface definition
The interface follows TypeScript best practices with proper type safety, clear prop definitions, and appropriate use of generics and utility types.
apps/skilavottord/ws/seeders/20201010211010-seed-recycling-request.js (2)
10-10: Verify the new vehicle ID format against validation rules
The vehicle ID format has been changed from lowercase with hyphens (e.g., 'ftm-522') to uppercase without hyphens (e.g., 'VM006', 'LT579'). Please ensure this new format:
- Matches any validation rules in the application
- Aligns with the actual vehicle registration number format in Iceland
Also applies to: 19-19, 28-28, 46-46, 55-55, 64-64
✅ Verification successful
Based on the search results, I can see that:
-
The codebase has both old and new vehicle ID formats:
- Old format: lowercase with hyphens (e.g., 'jfk-433', 'ftm-522', 'mhs-583')
- New format: uppercase without hyphens (e.g., 'VM006', 'LT579', 'AE135')
-
The database schema and indexes support both formats as they use VARCHAR type.
-
There are no explicit validation rules found for vehicle ID format in the codebase.
Let me generate the final response:
Vehicle ID format change appears safe to proceed
The new uppercase format without hyphens is consistent with other vehicle IDs in the seeder files and is supported by the database schema. No explicit validation rules were found that would conflict with this format change.
- The database uses VARCHAR type for vehicle_id columns, which accommodates both formats
- Indexes are format-agnostic:
idx_vehicle_idandidx_request_vehicle_id - The repeated use of 'AE135' appears intentional for testing scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for vehicle ID validation patterns in the codebase
rg -g '!*.{json,md,lock}' -i 'vehicle.*validation|vehicle.*pattern|vehicle.*format'
# Look for any existing vehicle ID usage to confirm format consistency
rg -g '!*/seeders/*' 'vehicle_id.*[A-Z0-9]{2,}'
Length of output: 43261
28-28: Confirm if repeated vehicle ID 'AE135' is intentional
The vehicle ID 'AE135' is used in three different recycling requests with different states:
- Line 28: Default state
- Line 46: 'pendingRecycle' state
- Line 55: 'handOver' state
This might be intentional for testing different states of the same vehicle, but please verify if this accurately represents your test scenarios.
Also applies to: 46-46, 55-55
✅ Verification successful
Multiple recycling requests for vehicle 'AE135' are intentionally seeded for testing state transitions
Looking at the seeder and migration files, this is a deliberate test data setup:
- The vehicle 'AE135' exists in the vehicle table (as 'aes-135')
- The recycling_request table has a foreign key constraint to vehicle table
- Multiple requests for the same vehicle show the progression through different states:
- Default state (initial request)
- 'pendingRecycle' state (in progress)
- 'handOver' state (completed)
This pattern is also seen with vehicle 'FZG90' which has both 'pendingRecycle' and 'handOver' states, confirming this is an intentional testing pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if multiple requests for the same vehicle are handled in the application
ast-grep --pattern 'function $FUNC($PARAMS) {
$$$
vehicle_id
$$$
}'
Length of output: 75
Script:
#!/bin/bash
# Search for files that handle recycling requests and vehicle IDs
rg -l "vehicle_id" --type js
# Check the content of relevant files for vehicle ID handling
rg "vehicle_id" -A 3 -B 3 --type js
# Look for any unique constraints or validations on vehicle_id
rg "unique.*vehicle_id|vehicle_id.*unique" --type js
Length of output: 15593
apps/skilavottord/web/pages/municipalities/index.tsx (2)
1-3: LGTM! Clean and well-structured imports.
The imports are properly organized using absolute paths, which is good for maintainability.
5-5: 🛠️ Refactor suggestion
Component name inconsistency and missing NextJS page structure.
- The component name
RecyclingCompaniesseems inconsistent with the page's purpose (municipalities). - The page lacks proper NextJS structure:
- Missing static or server-side data fetching methods
- Missing TypeScript interface for page props
- Type casting might indicate potential type safety issues
Consider refactoring to follow NextJS best practices:
import { Screen } from '@island.is/skilavottord-web/types'
import { withLocale } from '@island.is/skilavottord-web/i18n'
-import { RecyclingCompanies } from '@island.is/skilavottord-web/screens'
+import { Municipalities } from '@island.is/skilavottord-web/screens'
+
+interface MunicipalitiesPageProps {
+ // Add required props here
+}
+
+export const getServerSideProps = async () => {
+ // Fetch required data
+ return {
+ props: {
+ // Return fetched data
+ },
+ }
+}
-export default withLocale('is')(RecyclingCompanies as Screen)
+const MunicipalitiesPage: Screen<MunicipalitiesPageProps> = (props) => {
+ return <Municipalities {...props} />
+}
+
+export default withLocale('is')(MunicipalitiesPage)Let's verify the component naming consistency and type safety:
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (1)
36-37: Verify server-side handling of new mutation fields
The new fields isMunicipality and municipalityId have been added to the GraphQL mutation. Ensure that the server-side schema and resolvers are updated accordingly to handle these fields.
Run the following script to verify that the server-side schema includes the new fields:
✅ Verification successful
Server-side schema and resolvers are properly updated for new mutation fields
The server-side implementation correctly handles the new fields isMunicipality and municipalityId:
- The fields are properly defined in the database model (
RecyclingPartnerModel) - They are included in the mutation input type (
CreateRecyclingPartnerInput) - The resolver (
RecyclingPartnerResolver) is configured to handle these fields in the create mutation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the server-side GraphQL schema includes 'isMunicipality' and 'municipalityId' in the 'createSkilavottordRecyclingPartner' mutation.
# Fetch the GraphQL schema and search for the fields in the mutation input type.
curl -s http://localhost:3333/api/graphql -H "Content-Type: application/json" \
-d '{"query": "query { __type(name: \"CreateRecyclingPartnerInput\") { inputFields { name } } }"}' \
| jq '.data.__type.inputFields[].name' \
| grep -E 'isMunicipality|municipalityId'
Length of output: 259
Script:
#!/bin/bash
# Let's search for GraphQL schema files and mutations related to recycling partners
fd -e graphql -e gql | xargs cat
# Also search for TypeScript files that might contain the schema definition
rg -l "createSkilavottordRecyclingPartner|CreateRecyclingPartnerInput" --type=typescript
# Search for resolver implementations
ast-grep --pattern 'class $_ {
$$$
createSkilavottordRecyclingPartner($_) {
$$$
}
$$$
}'
Length of output: 69739
Script:
#!/bin/bash
# Let's try a different approach to find the schema and resolver files
fd -e ts | xargs rg -l "createSkilavottordRecyclingPartner|CreateRecyclingPartnerInput|isMunicipality|municipalityId"
# Also search for any GraphQL schema files
fd -e graphql | xargs rg "createSkilavottordRecyclingPartner|CreateRecyclingPartnerInput|isMunicipality|municipalityId"
Length of output: 4163
Script:
#!/bin/bash
# Let's examine the relevant files in the skilavottord app
cat apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.model.ts
cat apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.input.ts
cat apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts
Length of output: 6657
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanies.tsx (1)
80-88:
Handle potential undefined user role
When accessing user.role, ensure that user is defined to prevent potential runtime errors.
Apply this diff to add a null check for user:
const { data, error, loading } = useQuery<Query>(
SkilavottordAllRecyclingPartnersByTypeQuery,
{
variables: {
isMunicipality,
municipalityId: partnerId,
},
+ skip: !user,
},
)Likely invalid or redundant comment.
apps/skilavottord/web/pages/municipalities/add.tsx (1)
1-5: Verify language switching and apply consistent improvements
Similar improvements needed as the English version, plus ensure proper language switching is configured:
Example implementation:
- import { Screen } from '@island.is/skilavottord-web/types'
+ import { Screen } from '@types'
- import { withLocale } from '@island.is/skilavottord-web/i18n'
+ import { withLocale } from '@i18n'
- import { RecyclingCompanyCreate } from '@island.is/skilavottord-web/screens/RecyclingCompanies/RecyclingCompanyCreate'
+ import { RecyclingCompanyCreate } from '@screens/RecyclingCompanies/Create'
- export default withLocale('is')(RecyclingCompanyCreate as Screen)
+ export default withLocale('is')<Screen>(RecyclingCompanyCreate)Let's verify the language switching configuration:
✅ Verification successful
Language switching is properly configured, but import paths can be improved
The language switching implementation is correctly set up:
- Both English and Icelandic versions exist with proper locale configuration
- The
withLocaleHOC is correctly implemented and used - Both versions use the same component with different locale parameters
The suggested import path improvements are valid and can be applied:
@island.is/skilavottord-web/types→@types@island.is/skilavottord-web/i18n→@i18n@island.is/skilavottord-web/screens/RecyclingCompanies/RecyclingCompanyCreate→@screens/RecyclingCompanies/Create
The TypeScript generic syntax improvement (withLocale('is')<Screen>(RecyclingCompanyCreate)) is also valid and would provide better type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if language switching is properly configured in next.config.js
echo "Checking Next.js i18n configuration..."
cat next.config.js | grep -A 10 "i18n:"
# Verify that both language versions are properly linked
echo "Checking for language switcher component usage..."
rg -l "LanguageSwitcher" apps/skilavottord/web/
Length of output: 330
Script:
#!/bin/bash
# Let's try to find the Next.js config file with different possible names and locations
echo "Searching for Next.js config files..."
fd -t f "next.config" apps/skilavottord/web/
# Check for i18n configuration in package.json as an alternative
echo "Checking package.json for i18n config..."
fd -t f "package.json" apps/skilavottord/web/ -x cat {} | jq -r '.i18n'
# Let's check for language-related components and configurations
echo "Checking for language-related files..."
rg -l "i18n|locale|language" apps/skilavottord/web/
# Check for the English version of the same page
echo "Checking for English version of municipalities/add..."
fd -t f "add.tsx" apps/skilavottord/web/pages/en/
Length of output: 5243
Script:
#!/bin/bash
# Let's check the Next.js config content for i18n settings
echo "Checking Next.js config content..."
cat apps/skilavottord/web/next.config.js
# Let's examine the i18n implementation
echo "Checking i18n implementation..."
cat apps/skilavottord/web/i18n/withLocale.tsx
# Compare both language versions of the add page
echo "Comparing both language versions..."
echo "=== Icelandic version ==="
cat apps/skilavottord/web/pages/municipalities/add.tsx
echo "=== English version ==="
cat apps/skilavottord/web/pages/en/municipalities/add.tsx
Length of output: 3552
apps/skilavottord/web/pages/municipalities/[id].tsx (1)
1-5: Verify the reuse of RecyclingCompanyUpdate component
While the code follows NextJS conventions, we should verify if reusing the RecyclingCompanyUpdate component for municipalities is intentional and handles municipality-specific requirements correctly.
Additionally, consider adding error boundaries and loading states for better user experience.
✅ Verification successful
Let me gather more information about the RecyclingCompanyUpdate component to verify its usage.
Let me try a different approach to locate and verify the component usage.
Let me check the actual implementation of the RecyclingCompanyUpdate component.
Reuse of RecyclingCompanyUpdate component is intentional and properly implemented
The component is correctly designed to handle both recycling companies and municipalities through the isMunicipality flag. It includes:
- Proper route detection (
router.route === routes.municipalities.edit) - Municipality-specific UI adaptations (titles, breadcrumbs, navigation)
- Correct data handling for municipality role
- Proper permission checks
The component safely handles both use cases with appropriate conditional logic and type checking. No architectural changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if RecyclingCompanyUpdate component handles municipality role
ast-grep --pattern 'interface $_ {
$$$
municipality?: $_
$$$
}'
Length of output: 5527
Script:
#!/bin/bash
# Check RecyclingCompanyUpdate component implementation
ast-grep --pattern 'export const RecyclingCompanyUpdate = $_'
# Search for municipality-related props or types in the component
rg -A 5 "RecyclingCompanyUpdate" apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate.tsx
# Check if there are other municipality-specific components
fd "Municipality" apps/skilavottord/web
Length of output: 328
Script:
#!/bin/bash
# Find all files containing RecyclingCompanyUpdate
rg -l "RecyclingCompanyUpdate"
# Find municipality-related components and pages
fd -e tsx -e ts "municipality" -i
# Check imports in the municipality page
cat apps/skilavottord/web/pages/municipalities/[id].tsx
Length of output: 3650
Script:
#!/bin/bash
# Check the implementation of RecyclingCompanyUpdate component
cat apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx
# Check if there are separate components for municipalities
cat apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/index.tsx
Length of output: 6021
apps/skilavottord/ws/src/app/modules/auth/user.model.ts (1)
9-9: Verify role-based access control implementation
The addition of the municipality role needs careful verification of access control implementation across the application.
✅ Verification successful
Let me gather more specific information about the RBAC implementation.
Let me check the specific roles guard implementation for the skilavottord application.
Role-based access control is properly implemented for the municipality role
The municipality role is correctly integrated into the existing RBAC system:
- The RolesGuard implementation properly validates user roles
- The role is used consistently in @authorize decorators alongside other administrative roles (developer, recyclingFund)
- All recycling partner operations (queries and mutations) properly include the municipality role in their access control
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper RBAC implementation for municipality role
echo "Checking resolvers for municipality role authorization..."
rg -l "Role.municipality" --type ts
echo "\nChecking guards and decorators..."
ast-grep --pattern '@$_($$$)
export class $_ {
$$$
@$_([Role.municipality])
$$$
}'
Length of output: 1081
Script:
#!/bin/bash
# Check resolver implementations for municipality role
echo "Checking vehicle resolver implementation..."
rg -B 2 -A 5 "Role\.municipality" apps/skilavortord/ws/src/app/modules/vehicle/vehicle.resolver.ts
echo "\nChecking access control resolver implementation..."
rg -B 2 -A 5 "Role\.municipality" apps/skilavortord/ws/src/app/modules/accessControl/accessControl.resolver.ts
echo "\nChecking for role-based guards..."
rg -l "RolesGuard" --type ts
echo "\nChecking guard implementations..."
ast-grep --pattern 'class $_ implements CanActivate {
$$$
canActivate($$$) {
$$$
}
}'
Length of output: 9011
Script:
#!/bin/bash
# Check roles guard implementation in skilavottord
echo "Checking roles guard implementation..."
rg -B 2 -A 10 "class RolesGuard" apps/skilavottord/ws/src/app/modules/auth/roles.guard.ts
echo "\nChecking for roles decorator usage..."
rg -B 2 -A 5 "@Roles" apps/skilavottord/ws/src/app/modules/
echo "\nChecking recycling partner resolver for role checks..."
rg -B 2 -A 5 "Role\.municipality" apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts
Length of output: 2185
apps/skilavottord/ws/migrations/202412013210715-addcolumns-recycling-partners-municipality.js (1)
4-10: Consider adding foreign key constraint
The municipality_id column appears to reference municipalities but lacks a foreign key constraint.
Consider adding a foreign key constraint to ensure referential integrity:
ALTER TABLE recycling_partner
ADD CONSTRAINT fk_recycling_partner_municipality
FOREIGN KEY (municipality_id)
REFERENCES municipality(id);Let's verify if the municipality table exists:
apps/skilavottord/web/auth/utils.ts (3)
21-28: LGTM: Permission updates for recycling company roles
The removal of 'deregisterVehicleKM' from recycling company roles while maintaining essential permissions follows the principle of least privilege.
34-34: LGTM: New municipality role with appropriate permissions
The new municipality role has been granted appropriate permissions for viewing recycled vehicles, recycling companies, and access control.
28-34: Verify the impact of adding 'municipalities' permission
The addition of 'municipalities' to recyclingFund role's permissions needs verification to ensure all related UI components handle this new permission correctly.
✅ Verification successful
The addition of 'municipalities' permission is properly handled in the codebase
The codebase already has comprehensive support for the 'municipalities' permission:
- The permission is correctly added to the
recyclingFundrole inauth/utils.ts - The UI components handling municipalities are properly implemented with permission checks:
- Navigation links in
NavigationLinks.tsxhandle municipality-related routes - Dedicated screens for municipalities exist with proper permission checks
- Form components in
RecyclingCompanyFormhandle municipality-specific fields - Role-based access control is implemented throughout the codebase
- Navigation links in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UI components that might need updates for the new 'municipalities' permission
rg -t tsx -t ts "municipalities" apps/skilavottord/web/
Length of output: 83
Script:
#!/bin/bash
# Search for UI components that might need updates for the new 'municipalities' permission
# Using ripgrep without file type restrictions and checking the entire web directory
rg "municipalities" apps/skilavottord/web/
# Also search for any components or code handling permissions
rg -A 3 "hasPermission|usePermissions" apps/skilavottord/web/
# Check for any routes or components related to municipalities
rg -i "municipality|municipalities" apps/skilavottord/web/
Length of output: 29796
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.input.ts (1)
44-45: LGTM: Nullable configuration for municipalityId
The municipalityId field is correctly configured as nullable in both the decorator and type.
apps/skilavottord/web/components/Sidenav/Sidenav.tsx (3)
3-7: LGTM: Clean interface extension
The addition of the optional hidden property to SidenavSection interface follows TypeScript best practices.
16-16: LGTM: Type extension for municipality icon
The SidenavIcon type has been correctly extended to include the new municipality icon.
26-26: LGTM: Clean conditional rendering
The section rendering logic has been updated to handle the new hidden property while maintaining readability.
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts (1)
28-52: Verify the handling of null values
The current implementation might need additional validation for edge cases.
✅ Verification successful
Null handling is properly implemented in the codebase
The code correctly handles null values for both isMunicipality and municipalityId fields:
- The model definition explicitly marks
municipalityIdas nullable withallowNull: true - The GraphQL schema properly defines nullable fields with
@Field({ nullable: true }) - The service method correctly handles the OR condition for
isMunicipality: null - The resolver properly defines nullable arguments with
nullable: true
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential null handling issues in the codebase
# Check for explicit null checks in similar service methods
rg -A 2 "findAll.*null" apps/skilavottord/ws/src/app/modules
# Check for related model validations
ast-grep --pattern 'allowNull: true' apps/skilavottord/ws/src/app/modules
Length of output: 138
Script:
#!/bin/bash
# Let's check the model definition and related files for null handling
# Find the recycling partner model definition
fd "recyclingPartner.*model" apps/skilavottord/ws/src
# Check for null handling in the model and related files
rg -g "*.ts" "isMunicipality.*null|municipalityId.*null" apps/skilavottord/ws/src/app/modules
# Check for any validation or type definitions
rg -g "*.ts" "interface.*RecyclingPartner|type.*RecyclingPartner" apps/skilavottord/ws/src
Length of output: 1598
Script:
#!/bin/bash
# Let's examine the model definition and input types
# Check the model definition
cat apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.model.ts
# Check the input types
rg -g "*.ts" -A 5 "RecyclingPartnerInput|CreateRecyclingPartnerInput|UpdateRecyclingPartnerInput" apps/skilavottord/ws/src/app/modules/recyclingPartner/
Length of output: 10114
apps/skilavottord/ws/src/app/modules/auth/auth.guard.ts (1)
80-80:
Verify security implications of removing IdsUserGuard
The removal of IdsUserGuard from the UseGuards decorator could potentially impact authentication security. Please ensure this change aligns with the application's security requirements.
Consider keeping IdsUserGuard for additional security layer or document why it's safe to remove.
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (1)
20-20: Verify role-based access control implementation
The addition of Role.municipality to multiple methods needs verification:
- Ensure proper access restrictions
- Verify that municipality users can only access their own data
Also applies to: 55-55, 66-66, 87-87
✅ Verification successful
Let me gather more information about the access control implementation specific to municipality role.
Let me check the recycling partner service implementation to understand how municipality data access is restricted.
Role-based access control is properly implemented for municipality users
The codebase shows appropriate access restrictions for municipality users:
- Municipality users can only access recycling partners that are not municipalities and belong to their municipality (filtered by municipalityId)
- Vehicle queries are restricted by partnerId when the user has municipality role
- The @authorize decorator properly enforces role-based access across resolvers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for role checks in the codebase
rg "Role\.municipality" --type ts
# Look for access control implementations
ast-grep --pattern 'class $_ implements CanActivate {
$$$
canActivate($_) {
$$$
}
}'
Length of output: 4745
Script:
#!/bin/bash
# Check how municipality access is restricted in services
rg "Role\.municipality.*partnerId" -A 5 --type ts
# Look for partnerId checks in resolvers
ast-grep --pattern 'if ($user.role === Role.municipality) {
$$$
}'
# Check authorization decorator implementation
rg "@Authorize" -A 3 --type ts
Length of output: 9492
Script:
#!/bin/bash
# Check recycling partner service implementation
rg -A 10 "class RecyclingPartnerService" --type ts
# Look for municipality-specific filtering in the service
rg -A 10 "Role\.municipality.*return" --type ts
# Check how partnerId is used in queries
ast-grep --pattern 'findOne({
$$$
partnerId: $user.partnerId
$$$
})'
Length of output: 2637
apps/skilavottord/ws/src/app/modules/vehicle/vehicle.resolver.ts (1)
Line range hint 1-24: Verify access control implications of new municipality role
The addition of the municipality role requires verification of access control boundaries.
✅ Verification successful
Let me gather more information about the authorization patterns in the codebase.
Municipality role access control implementation is properly bounded
The municipality role has been consistently implemented with appropriate access controls across the codebase:
- Access to
skilavottordAllDeregisteredVehiclesis properly restricted to[Role.developer, Role.recyclingFund, Role.municipality] - Municipality role access is limited to specific endpoints in recycling partner operations
- The role's access is properly filtered using
partnerIdwhen querying vehicle data - No security implications found as the role follows the same authorization patterns as other roles in the system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential security implications of municipality role
# Look for other endpoints that might need municipality role
# Search for @Authorize decorators
rg -B 2 -A 2 "@Authorize\({"
# Search for role-based checks
rg "user\.role === "
Length of output: 4935
Script:
#!/bin/bash
# Search for Authorize decorators with better regex
rg "@Authorize\(" -A 2
# Search for municipality role usage
rg "Role\.municipality"
# Search for role-based mutations/queries in vehicle resolver
ast-grep --pattern $'@Authorize({ roles: [$$$] })
@$_($$$) $$$'
Length of output: 9010
apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx (2)
27-28: LGTM: Component modernization improves maintainability.
The replacement of Sidenav with NavigationLinks and Box with PageHeader aligns with modern component architecture, improving code maintainability and reusability.
Also applies to: 121-121, 141-141
31-36: Verify the impact of the required municipalityId parameter.
The GraphQL query now requires a municipalityId parameter. This change could affect existing query implementations.
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (2)
122-124: 🛠️ Refactor suggestion
Consider using useEffect dependency array properly.
The useEffect hook should include all dependencies it uses.
- React.useEffect(() => {
- setValue('isMunicipality', isMunicipality)
- }, [isMunicipality, setValue])
+ React.useEffect(() => {
+ setValue('isMunicipality', isMunicipality)
+ }, [isMunicipality, setValue])Likely invalid or redundant comment.
81-87: Verify role-based access control for municipality users.
The component now handles municipality-specific logic. Ensure proper access control is maintained.
✅ Verification successful
Let me analyze the role-based access control implementation for municipality users based on the search results. I can see comprehensive evidence of proper RBAC implementation across the codebase, but let me verify the specific authorization checks for recycling company operations.
Role-based access control for municipality users is properly implemented
The verification shows comprehensive and consistent RBAC implementation:
- Backend authorization is properly enforced through
@Authorizedecorators that explicitly includeRole.municipalityfor relevant recycling partner operations - Frontend components correctly handle municipality-specific logic by:
- Filtering recycling companies based on
partnerId - Implementing proper role checks before rendering components
- Maintaining proper access control in navigation and views
- Filtering recycling companies based on
The implementation in RecyclingCompanyUpdate.tsx aligns with the overall RBAC architecture of the system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for proper role definitions and access control
rg -A 5 "Role.municipality"
Length of output: 12054
Script:
#!/bin/bash
# Check for authorization decorators and role checks in recycling partner related files
ast-grep --pattern 'Authorize({ roles: [$$$] })'
Length of output: 5015
apps/skilavottord/web/i18n/locales/en.json (1)
638-642: LGTM: Well-structured route definitions.
The new municipality routes are properly structured and consistent with existing patterns.
apps/skilavottord/web/i18n/locales/translation.d.ts (5)
33-33: LGTM! Proper type definition for municipalities.
The municipalities property is correctly added to the Translation interface.
477-492: LGTM! Well-structured municipality interfaces.
The Municipalities and Municipality interfaces are properly defined with all necessary properties and follow the existing pattern.
517-517: LGTM! Optional municipality field in form inputs.
The municipality field is correctly added to the FormInputs interface as an optional field.
560-564: LGTM! Proper sidenav type definitions.
The municipalityTitle and municipalities properties are correctly added to the RecyclingFundSidenav interface.
577-577: LGTM! Route type definition for municipalities.
The municipalities property is correctly added to the Routes interface using the existing RecyclingCompaniesClass type.
...kilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx
Outdated
Show resolved
Hide resolved
...kilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx
Show resolved
Hide resolved
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanies.tsx
Outdated
Show resolved
Hide resolved
...tord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx
Outdated
Show resolved
Hide resolved
apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts
Outdated
Show resolved
Hide resolved
apps/skilavottord/ws/src/app/modules/accessControl/accessControl.resolver.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (3)
84-90: Consider a more robust route checkThe current route check for
isMunicipalityis tightly coupled to the exact route path. Consider using a more maintainable approach.Consider this alternative:
- const isMunicipality = router.route === routes.municipalities.edit + const isMunicipality = router.route.startsWith(routes.municipalities.baseRoute)
119-124: Move string literals to translation constantsConsider moving the hardcoded section number and route to translation constants for better maintainability.
Example approach:
const SECTIONS = { RECYCLING_COMPANIES: 2, MUNICIPALITIES: 1, } as const
Line range hint
156-164: Enhance error handling in mutation responseThe current error handling doesn't provide user feedback when the mutation fails.
Add error handling:
const { errors } = await updateSkilavottordRecyclingPartner({ variables: { input }, }) if (!errors) { router.push(routes.recyclingCompanies.baseRoute).then(() => { toast.success(t.recyclingCompany.view.updated) }) + } else { + toast.error(t.recyclingCompany.view.updateError) }🧰 Tools
🪛 Biome (1.9.4)
[error] 150-150: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
apps/skilavottord/web/components/NavigationLinks/NavigationLinks.tsx (2)
12-21: Consider adding null safety to role checkThe role check could be made more robust by adding null safety:
- if (hasMunicipalityRole(user?.role)) { + if (user?.role && hasMunicipalityRole(user.role)) {
23-52: Remove unnecessary template literalsTemplate literals are used unnecessarily for static strings in the sections array.
- title: `${sidenavText.recycled}`, - link: `${routes.recycledVehicles}`, + title: sidenavText.recycled, + link: routes.recycledVehicles,apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (1)
58-73: Consider extracting route comparison logicThe route comparison could be made more maintainable by using a constant.
+const MUNICIPALITY_ROUTE = routes.municipalities.add; + -const isMunicipality = router.route === routes.municipalities.add +const isMunicipality = router.route === MUNICIPALITY_ROUTEapps/skilavottord/web/screens/AccessControl/AccessControl.tsx (2)
123-145: Consolidate data fetching logic for recycling partnersThe current implementation uses two separate
useLazyQueryhooks (getAllRecyclingPartnerandgetAllRecyclingPartnersByMunicipality) to fetch recycling partner data based on the user's role. This can be simplified by combining the logic into a single hook or function that handles the condition internally. This will reduce code duplication and improve maintainability.Consider refactoring as follows:
- const [ - getAllRecyclingPartner, - { - data: recyclingPartnerData, - error: recyclingPartnerError, - loading: recyclingPartnerLoading, - }, - ] = useLazyQuery<Query>(SkilavottordAllRecyclingPartnersQuery, { - ssr: false, - }) - - const [ - getAllRecyclingPartnersByMunicipality, - { - data: recyclingPartnerByIdData, - error: recyclingPartnerByIdError, - loading: recyclingPartnerByIdLoading, - }, - ] = useLazyQuery<Query>(SkilavottordAllRecyclingPartnersByTypeQuery, { - ssr: false, - variables: { isMunicipality: false, municipalityId: user?.partnerId }, - }) + const [ + getRecyclingPartners, + { + data: recyclingPartnerData, + error: recyclingPartnerError, + loading: recyclingPartnerLoading, + }, + ] = useLazyQuery<Query>( + user.role === Role.municipality + ? SkilavottordAllRecyclingPartnersByTypeQuery + : SkilavottordAllRecyclingPartnersQuery, + { + ssr: false, + variables: + user.role === Role.municipality + ? { isMunicipality: false, municipalityId: user?.partnerId } + : {}, + }, + )
226-228: Handle undefinedmunicipalityIdsafely in labelsIn constructing the
labelforrecyclingPartners, the ternary check onpartner.municipalityIdshould ensure that undefined or null values are handled properly to avoid displayingundefined - companyName.Consider using a more robust nullish coalescing operator:
- label: partner.municipalityId + label: partner.municipalityId ?? ''Or adjust the ternary condition:
- label: partner.municipalityId + label: partner.municipalityId
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
apps/skilavottord/web/auth/utils.ts(1 hunks)apps/skilavottord/web/components/NavigationLinks/NavigationLinks.tsx(1 hunks)apps/skilavottord/web/screens/AccessControl/AccessControl.tsx(9 hunks)apps/skilavottord/web/screens/AccessControlCompany/AccessControlCompany.tsx(2 hunks)apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanies.tsx(3 hunks)apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx(5 hunks)apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx(8 hunks)apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx(5 hunks)apps/skilavottord/ws/src/app/modules/accessControl/accessControl.service.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/skilavottord/web/auth/utils.ts
👮 Files not reviewed due to content moderation or server errors (2)
- apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx
- apps/skilavottord/web/screens/AccessControlCompany/AccessControlCompany.tsx
🧰 Additional context used
📓 Path-based instructions (8)
apps/skilavottord/web/components/NavigationLinks/NavigationLinks.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/accessControl/accessControl.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControlCompany/AccessControlCompany.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControl/AccessControl.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanies.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
📓 Learnings (1)
apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (1)
Learnt from: birkirkristmunds
PR: island-is/island.is#17109
File: apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx:336-360
Timestamp: 2024-12-10T15:10:49.998Z
Learning: In the `RecyclingCompanyForm` component (`apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx`), the `isMunicipality` checkbox must remain in the DOM for all users since its value is stored in the database. However, only users with the developer role can manipulate it through the UI. This ensures that the checkbox value is maintained correctly while restricting interaction to authorized users.
🔇 Additional comments (12)
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (2)
Line range hint 169-205: Well-structured component rendering
The component rendering follows NextJS best practices with:
- Proper loading state handling
- Consistent layout structure
- Clear component composition
112-114: Add null check for partnerId in refetchQueries
The partnerId could be undefined when passed to the refetch query, which might cause unexpected behavior.
Let's verify the handling of undefined partnerId in the codebase:
apps/skilavottord/web/components/NavigationLinks/NavigationLinks.tsx (1)
1-11: LGTM! Clean imports and well-typed props.
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (1)
39-40: LGTM! Proper mutation field additions
The new fields isMunicipality and municipalityId are correctly added to the mutation response type.
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanies.tsx (3)
33-49: LGTM! Well-structured GraphQL query
The new query properly handles type-specific filtering with correct variable types.
77-81: Add null check for municipality role handling
This addresses a previously identified issue with partner ID assignment.
- if (hasMunicipalityRole(user?.role)) {
+ if (user?.role && hasMunicipalityRole(user.role)) {104-126: 🛠️ Refactor suggestion
Refactor routing logic and investigate BASE-PATH issue
- The routing logic is duplicated between
handleCreateandhandleUpdate - There's a comment about BASE-PATH duplication that needs investigation
Suggested refactor:
+ const getRoutePath = (type: 'create' | 'edit', id?: string) => {
+ const base = isMunicipality ? routes.municipalities : routes.recyclingCompanies;
+ const path = type === 'create' ? base.add : base.edit;
+ return id ? { pathname: path, query: { id } } : { pathname: path };
+ }
const handleCreate = () => {
- if (isMunicipality) {
- router.push({
- pathname: routes.municipalities.add,
- })
- } else {
- router.push({
- pathname: routes.recyclingCompanies.add,
- })
- }
+ router.push(getRoutePath('create'));
}
const handleUpdate = (id: string) => {
- if (isMunicipality) {
- router.push({
- pathname: routes.municipalities.edit,
- query: { id },
- })
- } else {
- router.push({
- pathname: routes.recyclingCompanies.edit,
- query: { id },
- })
- }
+ router.push(getRoutePath('edit', id));
}apps/skilavottord/web/screens/AccessControl/AccessControl.tsx (3)
221-224: Ensure consistency of partners data structure
Merging data from recyclingPartnerData and recyclingPartnerByIdData into partners may lead to inconsistent data structures if the returned data differs. Verify that both data sources return data in the same format and handle any discrepancies to prevent runtime errors.
234-247: Simplify roles filtering logic for better readability
The current roles filtering logic involves multiple filters and conditions, which can be simplified to enhance readability and maintainability.
Consider refactoring:
const roles = Object.keys(AccessControlRole)
.filter((role) => {
if (!hasDeveloperRole(user?.role) && role === Role.developer) {
return false
}
if (user.role === Role.municipality) {
return [Role.recyclingCompany, Role.recyclingCompanyAdmin, Role.municipality].includes(role)
}
return true
})
.map((role) => ({
label: getRoleTranslation(role as Role, activeLocale),
value: role,
}))255-263: Ensure partners data is fetched before opening modal
When opening the create access control modal, ensure that the partners data is fetched before rendering the modal to prevent potential undefined data issues.
Consider adding a loading state or disabling the button until data is ready.
apps/skilavottord/ws/src/app/modules/accessControl/accessControl.service.ts (2)
68-69: Optimize database query for large partnerIds arrays
Using the IN operator with a potentially large partnerIds array can lead to performance issues. Consider implementing pagination, batching, or alternative querying strategies for handling large datasets.
37-66:
Add safeguards to recursive getAllCompanyIds function
The recursive getAllCompanyIds function lacks protection against circular references and unchecked recursion depth, which could lead to stack overflows or performance issues.
Implement safeguards by tracking visited partnerIds and limiting recursion depth:
- const getAllCompanyIds = async (partnerId: string) => {
- partnerIds.push(partnerId)
- const subCompanies = await RecyclingPartnerModel.findAll({
- where: { municipalityId: partnerId },
- })
- for (const subCompany of subCompanies) {
- await getAllCompanyIds(subCompany.companyId)
- }
- }
+ const getAllCompanyIds = async (
+ partnerId: string,
+ visited = new Set<string>(),
+ depth = 0,
+ ) => {
+ const MAX_DEPTH = 100
+ if (depth > MAX_DEPTH || visited.has(partnerId)) return
+ visited.add(partnerId)
+ partnerIds.push(partnerId)
+ const subCompanies = await RecyclingPartnerModel.findAll({
+ where: { municipalityId: partnerId },
+ })
+ for (const subCompany of subCompanies) {
+ await getAllCompanyIds(subCompany.companyId, visited, depth + 1)
+ }
+ }
...kilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx
Show resolved
Hide resolved
...kilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx
Outdated
Show resolved
Hide resolved
...kilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (12)
apps/skilavottord/ws/src/app/modules/accessControl/accessControl.model.ts (1)
Line range hint
66-73: LGTM! The new partnerId field is well-structured.The implementation follows TypeScript, GraphQL, and Sequelize best practices with proper decorators and type safety.
Consider adding a JSDoc comment to document the purpose of this field and its relationship with RecyclingPartnerModel:
+ /** + * ID of the associated recycling partner. Used to establish relationship + * between access control entries and recycling partners for municipality management. + */ @Field({ nullable: true }) @ForeignKey(() => RecyclingPartnerModel) @Column({ type: DataType.STRING, field: 'partner_id', allowNull: true, }) partnerId?: stringapps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (2)
112-116: Add TypeScript types for query variablesConsider adding explicit TypeScript types for the query variables to improve type safety and maintainability.
+ interface RecyclingPartnersQueryVars { + isMunicipalityPage: boolean + municipalityId: string | null + } refetchQueries: [ { query: SkilavottordRecyclingPartnerQuery, variables: { input: { companyId: id } }, }, { query: SkilavottordRecyclingPartnersQuery, - variables: { - isMunicipalityPage: isMunicipalityPage, - municipalityId: partnerId, - }, + variables: { + isMunicipalityPage, + municipalityId: partnerId, + } as RecyclingPartnersQueryVars, }, ],
128-130: Add cleanup function to useEffectConsider adding a cleanup function to handle component unmounting, following React best practices.
React.useEffect(() => { setValue('isMunicipality', isMunicipalityPage) + return () => { + // Reset the form value on unmount if needed + setValue('isMunicipality', false) + } }, [isMunicipalityPage, setValue])apps/skilavottord/web/screens/DeregisterVehicle/Overview/Overview.tsx (1)
Line range hint
52-195: Excellent implementation following NextJS and TypeScript best practices.The component demonstrates:
- Proper error handling and loading states
- Type-safe implementation
- Efficient data fetching with pagination
- Clean and maintainable structure
Consider implementing React Query or SWR for better cache management and automatic revalidation of data in future iterations.
apps/skilavottord/web/i18n/locales/translation.d.ts (1)
478-493: Consider enhancing type safety for municipality interfaces.The new
MunicipalitiesandMunicipalityinterfaces reuse existing patterns, which is good. However, consider adding more specific types for municipality-specific fields that might have unique validation requirements.Example enhancement:
export interface Municipality { view: View add: Add - form: RecyclingCompanyForm + form: { + inputs: MunicipalityFormInputs + buttons: FormButtons + } } +export interface MunicipalityFormInputs extends FormInputs { + // Add municipality-specific fields with appropriate validation rules + region?: { + label: string + placeholder: string + rules: { + required: string + } + } +}apps/skilavottord/web/screens/AccessControl/components/AccessControlModal/AccessControlModal.tsx (2)
68-71: Initialize component states inuseEffectThe
useEffecthook initializes the visibility of company and municipality selection inputs. Confirm that this aligns with the intended initial state when the modal is opened.Consider whether the
useEffectis necessary here, as the initial state values are already set in theuseStatedeclarations.
73-91: SimplifyhandleRoleOnChangelogicThe
handleRoleOnChangefunction has nested conditions that could be refactored for clarity and maintainability. Consider simplifying the logic to reduce complexity.Here's a possible refactor:
const handleRoleOnChange = (e: Option) => { - if (hasMunicipalityRole(user?.role)) { - if (e && e.value === Role.municipality) { - setShowCompaniesSelection(false) - } else { - setShowCompaniesSelection(true) - } - setShowMunicipalitiesSelection(false) - } else if (hasRecyclingFundRole(user?.role)) { - if (e && e.value === Role.municipality) { - setShowCompaniesSelection(false) - setShowMunicipalitiesSelection(true) - } else { - setShowMunicipalitiesSelection(false) - setShowCompaniesSelection(true) - } - } + const isMunicipalityRoleSelected = e && e.value === Role.municipality + if (hasMunicipalityRole(user?.role)) { + setShowCompaniesSelection(!isMunicipalityRoleSelected) + setShowMunicipalitiesSelection(false) + } else if (hasRecyclingFundRole(user?.role)) { + setShowCompaniesSelection(!isMunicipalityRoleSelected) + setShowMunicipalitiesSelection(isMunicipalityRoleSelected) + } }apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx (2)
75-78: Consider extracting role-based logicThe inline role check creates tight coupling. Consider extracting this logic to a utility function for better maintainability.
- partnerId: - role.value === Role.municipality - ? municipalityId?.value - : partnerId?.value || null, + partnerId: getPartnerId(role.value, municipalityId?.value, partnerId?.value),
94-97: Simplify partner ID requirement checkThe current check could be simplified using array inclusion.
- partnerIdRequired={ - watch('role')?.value === Role.recyclingCompanyAdmin || - watch('role')?.value === Role.recyclingCompany - } + partnerIdRequired={[Role.recyclingCompanyAdmin, Role.recyclingCompany].includes(watch('role')?.value)}apps/skilavottord/web/screens/CompanyInfo/CompanyInfo.tsx (1)
28-28: Consider prop type validationThe PageHeader component is used without TypeScript prop validation. Ensure proper type checking.
Add prop types validation:
interface PageHeaderProps { title: string; info?: string; }Also applies to: 105-105
apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (1)
278-311: Consider enhancing error handling for municipality selection.While the implementation is clean, consider adding more specific error messages for municipality selection failures.
<Select name={name} label={t.recyclingCompany.form.inputs.municipality.label} placeholder={t.recyclingCompany.form.inputs.municipality.placeholder} size="md" value={municipalities?.find((option) => option.value === value)} hasError={!!errors?.municipality?.message} - errorMessage={errors?.municipality?.message} + errorMessage={errors?.municipality?.message || t.recyclingCompany.form.inputs.municipality.error} backgroundColor="blue" options={municipalities} onChange={onChange} isCreatable />apps/skilavottord/ws/src/app/modules/recyclingRequest/recyclingRequest.service.ts (1)
46-48: Consider enhancing error handling for disposalStationId retrieval.While the addition of disposalStationId retrieval is good, consider adding specific error handling for cases where the ID cannot be retrieved.
const disposalStationId = - await this.recyclingPartnerService.getGetPayingPartnerId(disposalStation) + await this.recyclingPartnerService.getGetPayingPartnerId(disposalStation) + .catch((error) => { + this.logger.error('Failed to retrieve disposal station ID', { error, disposalStation }) + throw new Error(`Invalid disposal station: ${disposalStation}`) + })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/skilavottord/web/auth/utils.ts(1 hunks)apps/skilavottord/web/i18n/locales/en.json(5 hunks)apps/skilavottord/web/i18n/locales/is.json(6 hunks)apps/skilavottord/web/i18n/locales/translation.d.ts(7 hunks)apps/skilavottord/web/screens/AccessControl/AccessControl.tsx(12 hunks)apps/skilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx(5 hunks)apps/skilavottord/web/screens/AccessControl/components/AccessControlModal/AccessControlModal.tsx(4 hunks)apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx(6 hunks)apps/skilavottord/web/screens/AccessControlCompany/AccessControlCompany.tsx(3 hunks)apps/skilavottord/web/screens/CompanyInfo/CompanyInfo.tsx(2 hunks)apps/skilavottord/web/screens/DeregisterVehicle/Overview/Overview.tsx(2 hunks)apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanies.tsx(3 hunks)apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx(5 hunks)apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx(7 hunks)apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx(5 hunks)apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx(4 hunks)apps/skilavottord/ws/src/app/modules/accessControl/accessControl.model.ts(1 hunks)apps/skilavottord/ws/src/app/modules/accessControl/accessControl.service.ts(2 hunks)apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts(4 hunks)apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts(3 hunks)apps/skilavottord/ws/src/app/modules/recyclingRequest/recyclingRequest.service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/skilavottord/ws/src/app/modules/accessControl/accessControl.service.ts
- apps/skilavottord/web/screens/AccessControlCompany/AccessControlCompany.tsx
🧰 Additional context used
📓 Path-based instructions (19)
apps/skilavottord/ws/src/app/modules/accessControl/accessControl.model.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/CompanyInfo/CompanyInfo.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/DeregisterVehicle/Overview/Overview.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/recyclingRequest/recyclingRequest.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanies.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControl/AccessControl.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/i18n/locales/is.json (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/i18n/locales/en.json (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/auth/utils.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/i18n/locales/translation.d.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControl/components/AccessControlModal/AccessControlModal.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
📓 Learnings (1)
apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (1)
Learnt from: birkirkristmunds
PR: island-is/island.is#17109
File: apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx:336-360
Timestamp: 2024-12-10T15:10:49.998Z
Learning: In the `RecyclingCompanyForm` component (`apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx`), the `isMunicipality` checkbox must remain in the DOM for all users since its value is stored in the database. However, only users with the developer role can manipulate it through the UI. This ensures that the checkbox value is maintained correctly while restricting interaction to authorized users.
🔇 Additional comments (52)
apps/skilavottord/ws/src/app/modules/accessControl/accessControl.model.ts (1)
Line range hint 66-77: Verify database migration for the new partnerId field.
The model changes look correct, but we should ensure there's a corresponding database migration for this new field.
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (1)
43-43: Add municipalityId to the mutation response
The municipalityId field is included in the query response but missing in the mutation response. This inconsistency could lead to stale data after updates.
Also applies to: 47-63
apps/skilavottord/web/screens/DeregisterVehicle/Overview/Overview.tsx (2)
3-4: LGTM! Well-organized imports following NextJS best practices.
The imports are logically grouped by source/type, enhancing code readability and maintainability.
Also applies to: 13-14, 21-21, 23-23, 27-27, 29-29, 32-32
171-171: Good refactoring to use the shared PageHeader component.
The change improves component reusability and standardizes header presentation across the application.
Let's verify the PageHeader component implementation:
✅ Verification successful
Let me gather more information about the PageHeader component implementation.
PageHeader component is well-implemented and consistently used
The PageHeader component is properly implemented using Island UI components and is consistently used across multiple screens in the application. It:
- Accepts required
titleandinfoprops - Uses appropriate layout components (Box, GridRow, GridColumn)
- Implements consistent styling with Text variant="h1" and Tooltip
- Shows consistent usage pattern across 7 different screens
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify PageHeader component implementation and usage
# Check PageHeader component implementation
ast-grep --pattern 'export const PageHeader = ({ title, info }: Props) => {
$$$
}'
# Check for consistent usage across the application
rg -A 2 '<PageHeader.*title=.*info=.*>'
Length of output: 2545
Script:
#!/bin/bash
# Search for PageHeader component definition
fd PageHeader.tsx --exec cat {}
Length of output: 974
apps/skilavottord/web/i18n/locales/translation.d.ts (5)
33-33: LGTM: Translation interface updated correctly.
The addition of the municipalities property to the Translation interface is consistent with the existing pattern and properly typed.
75-75: LGTM: ModalInputs interface updated correctly.
The addition of the municipality property to the ModalInputs interface follows the existing pattern by reusing the Name type.
469-469: LGTM: RecyclingCompaniesButtons interface updated correctly.
The addition of the addMunicipality property to the RecyclingCompaniesButtons interface maintains consistency with existing button properties.
561-561: LGTM: RecyclingFundSidenav interface updated correctly.
The addition of municipalityTitle and municipalities properties to the RecyclingFundSidenav interface follows the existing naming pattern.
Also applies to: 565-565
578-578: Verify the route configuration.
The addition of the municipalities property to the Routes interface using RecyclingCompaniesClass type is consistent. However, verify that the route structure matches the actual routing implementation.
✅ Verification successful
Route configuration is correctly implemented and matches the type definition
The route configuration for municipalities is properly implemented and used consistently throughout the codebase:
- The type definition in
translation.d.tscorrectly usesRecyclingCompaniesClasstype for themunicipalitiesroute - The route is actively used in components:
RecyclingCompanies.tsxusesroutes.municipalities.baseRoute,routes.municipalities.add, androutes.municipalities.editNavigationLinks.tsximplements the navigation usingroutes.municipalities.baseRoute- Route checks are properly implemented in components like
RecyclingCompanyUpdateandRecyclingCompanyCreate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify route configuration matches implementation
# Test: Search for municipality route definitions in the codebase
rg -t typescript -t javascript "municipalities.*route" ./apps/skilavottord/web
Length of output: 115
Script:
#!/bin/bash
# Search for route configurations and usages of municipalities
rg -g '*.{ts,tsx,js,jsx}' 'municipalities' ./apps/skilavottord/web
Length of output: 5320
Script:
#!/bin/bash
# Look for route definitions in common routing files
fd -e ts -e tsx -e js -e jsx "routes|router" ./apps/skilavottord/web --exec cat {}
Length of output: 1152
Script:
#!/bin/bash
# Check the type definition and its usage
ast-grep --pattern 'interface $_ {
$$$
municipalities: $_
$$$
}'
Length of output: 3776
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts (2)
9-9: LGTM!
The Op import from sequelize is correctly added and is necessary for the query conditions.
28-52: 🛠️ Refactor suggestion
Improve method implementation and add error handling
- The comment on line 32 is misleading as it doesn't match the actual logic
- The method could be simplified to use a single query with dynamic conditions
- Error handling should be added
Apply this diff to improve the implementation:
async findRecyclingPartners(
isMunicipalityPage: boolean,
municipalityId: string | null,
): Promise<RecyclingPartnerModel[]> {
- // If Role.municipality, return all recycling partners that are not municipalities
- if (municipalityId) {
- return await this.recyclingPartnerModel.findAll({
- where: {
- isMunicipality: false,
- municipalityId: municipalityId,
- },
- })
- }
-
- if (isMunicipalityPage) {
- return await this.recyclingPartnerModel.findAll({
- where: { isMunicipality: true },
- })
- }
- return await this.recyclingPartnerModel.findAll({
- where: {
- [Op.or]: [{ isMunicipality: false }, { isMunicipality: null }],
- },
- })
+ try {
+ const whereCondition = municipalityId
+ ? { isMunicipality: false, municipalityId }
+ : isMunicipalityPage
+ ? { isMunicipality: true }
+ : {
+ [Op.or]: [{ isMunicipality: false }, { isMunicipality: null }],
+ };
+
+ return await this.recyclingPartnerModel.findAll({
+ where: whereCondition,
+ });
+ } catch (error) {
+ throw new Error(`Failed to fetch recycling partners: ${error.message}`);
+ }
}apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (2)
20-20: LGTM!
The addition of Role.municipality to the authorization roles is consistent across all methods and aligns with the PR objectives.
Also applies to: 30-30, 55-55, 66-66, 87-87
22-27: 🛠️ Refactor suggestion
Align method name with GraphQL query name
The method name should match the GraphQL query name for consistency and clarity.
Apply this diff to fix the naming:
@Query(() => [RecyclingPartnerModel], {
name: 'skilavottordAllRecyclingPartners',
})
-async getAllRecyclingPartners(): Promise<RecyclingPartnerModel[]> {
+async skilavottordAllRecyclingPartners(): Promise<RecyclingPartnerModel[]> {
return this.recyclingPartnerService.findAll()
}apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (2)
123-131: Simplify success handling logic
The success handling code is duplicated and can be simplified.
Apply this diff to simplify the code:
- if (isMunicipalityPage) {
- router.push(routes.municipalities.baseRoute).then(() => {
- toast.success(t.recyclingCompany.add.added)
- })
- } else {
- router.push(routes.recyclingCompanies.baseRoute).then(() => {
- toast.success(t.recyclingCompany.add.added)
- })
- }
+ const route = isMunicipalityPage
+ ? routes.municipalities.baseRoute
+ : routes.recyclingCompanies.baseRoute
+ router.push(route).then(() => {
+ toast.success(t.recyclingCompany.add.added)
+ })75-85: 🛠️ Refactor suggestion
Improve form initialization and validation
The form initialization could be improved to handle null values and add validation.
Apply this diff to improve the form handling:
const {
control,
handleSubmit,
formState: { errors },
} = useForm({
mode: 'onChange',
- defaultValues: {
- isMunicipality: isMunicipalityPage,
- municipalityId: partnerId,
- },
+ defaultValues: isMunicipalityPage
+ ? { isMunicipality: true }
+ : { isMunicipality: false, municipalityId: partnerId || '' },
})apps/skilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx (6)
1-1: Imports are correctly added
The addition of useContext, useEffect, UserContext, and hasMunicipalityRole imports is appropriate for the new functionality related to municipalities and user roles.
Also applies to: 12-13
23-23: Include municipalities in props interface
Adding municipalities to the AccessControlCreateProps interface ensures that the component can receive and utilize the list of municipalities.
38-38: Retrieve user data using useContext
Using const { user } = useContext(UserContext) correctly accesses the user information from the context for role-based logic within the component.
65-73: Ensure all required fields are submitted
In the handleOnSubmit function, verify that all required fields, especially the newly added municipalityId, are correctly included in the submission data and validated.
77-80: Reset form state when modal visibility changes
The useEffect hook resets the form whenever the show prop changes, ensuring that the form is cleared when the modal is reopened. This enhances the user experience by preventing residual data from previous entries.
90-90: Pass municipalities prop to AccessControlModal
Providing the municipalities prop to AccessControlModal allows the modal to display municipality options, aligning with the new feature requirements.
apps/skilavottord/web/screens/AccessControl/components/AccessControlModal/AccessControlModal.tsx (6)
1-8: Imports are correctly updated
The inclusion of useContext, useEffect, useState, and kennitala imports supports the added functionality for role handling and national ID validation.
13-21: Update interface to include new props
Adding municipalities and currentPartner to AccessControlModalProps enables the component to handle municipalities and existing partner data effectively.
64-67: Retrieve user context and initialize state
Using useContext(UserContext) to access user, and initializing showCompanies and showMunicipalities states, sets up the component for dynamic rendering based on user roles.
208-211: Ensure handleRoleOnChange is called correctly
In the Select component for role, the onChange handler correctly updates the form value and triggers handleRoleOnChange to adjust the form based on the selected role.
216-249: Conditional rendering of partnerId selection
The partnerId selection is conditionally rendered based on showCompanies. Ensure that this logic accurately reflects when a user should select a recycling partner.
250-279: Add municipalityId selection for relevant roles
Introducing the municipalityId selection when showMunicipalities is true allows users with certain roles to select a municipality. Ensure that the validation rules and error handling for this field are correctly implemented.
apps/skilavottord/web/screens/AccessControl/AccessControl.tsx (6)
127-153: Optimize data fetching with useLazyQuery hooks
Replacing useQuery with useLazyQuery for fetching recycling partner data allows more controlled and efficient data retrieval based on user interactions and roles.
224-227: Ensure correct retrieval of accessControls data
The accessControls variable consolidates data from different queries. Verify that it correctly handles data from all potential sources to prevent missing or inconsistent access control entries.
255-267: Simplify roles filtering logic
The roles filtering logic can be simplified and made more readable.
Apply this diff to improve the filtering logic:
const roles = Object.keys(AccessControlRole)
- .filter((role) =>
- !hasDeveloperRole(user?.role) ? role !== Role.developer : role,
- )
- .filter((role) => {
- if (hasMunicipalityRole(Role.municipality)) {
- return (
- role === Role.recyclingCompany ||
- role === Role.recyclingCompanyAdmin ||
- role === Role.municipality
- )
- }
- return role
- })
+ .filter((role) => {
+ if (!hasDeveloperRole(user?.role) && role === Role.developer) {
+ return false
+ }
+ if (hasMunicipalityRole(user?.role)) {
+ return [
+ Role.recyclingCompany,
+ Role.recyclingCompanyAdmin,
+ Role.municipality,
+ ].includes(role as Role)
+ }
+ return true
+ })
.map((role) => ({
label: getRoleTranslation(role as Role, activeLocale),
value: role,
}))This suggestion aligns with a previous review comment.
276-284: Ensure partners data is fetched before rendering modal
When opening the create access control modal, ensure that the partners data is fetched based on the user's role to prevent any potential data issues.
339-347: Update layout components and pass new props
Replacing Sidenav with NavigationLinks and updating the PageHeader enhances the UI. Passing municipalities to AccessControlCreate aligns with the new feature implementation.
205-211:
Include all data sources in error and loading states
Ensure that the error, loading, and isData variables include all relevant data sources (recyclingPartnerData, recyclingPartnerByIdData, and accessControlsData) to accurately reflect the component's state.
Apply this diff to include all data sources:
const error =
- recyclingPartnerError || accessControlsError || recyclingPartnerByIdError
const loading =
- recyclingPartnerLoading || accessControlsLoading || recyclingPartnerByIdLoading
const isData =
- !!recyclingPartnerData || !!accessControlsData
+ recyclingPartnerError || accessControlsError || recyclingPartnerByIdError
+ recyclingPartnerLoading || accessControlsLoading || recyclingPartnerByIdLoading
+ !!recyclingPartnerData || !!recyclingPartnerByIdData || !!accessControlsDataThis comment addresses a similar concern mentioned in previous reviews.
apps/skilavottord/web/auth/utils.ts (3)
13-19: Introduce role-checking utility functions
Adding hasDeveloperRole, hasMunicipalityRole, and hasRecyclingFundRole functions improves code readability and reusability when checking user roles throughout the application.
27-40: Update hasPermission function with new roles and pages
Modifying the permittedRoutes object to include the municipality role and updating permitted pages for existing roles ensures that access control aligns with the application's updated role definitions.
13-16: Potential impact of removing 'deregisterVehicleKM'
Removing 'deregisterVehicleKM' from the Page type might affect parts of the application that rely on this page. Ensure that this page is deprecated and no longer in use.
Run the following script to check for any remaining references to 'deregisterVehicleKM':
✅ Verification successful
No references to deregisterVehicleKM found in the codebase
Since no references to deregisterVehicleKM were found in any form (camelCase, kebab-case, or case-insensitive variations) across the entire codebase, it appears that this page has been completely removed and is no longer in use. The removal of this value from the Page type is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any usages of 'deregisterVehicleKM' in the codebase.
# Test: Find all occurrences of 'deregisterVehicleKM'
rg 'deregisterVehicleKM'
Length of output: 24
Script:
#!/bin/bash
# Let's try a case-insensitive search and also look for kebab-case variations
rg -i "deregister.*vehicle.*km|deregister-vehicle-km"
# Also search for any files with similar naming patterns
fd -i "deregister.*vehicle.*km"
Length of output: 89
apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx (1)
60-63: Verify municipality ID mapping logic
The code assumes that municipality's companyId matches the recyclingPartner's companyId. This assumption might lead to incorrect mappings.
✅ Verification successful
Based on the codebase analysis, I can now provide a final response regarding the municipality ID mapping concern:
The municipality ID mapping is correctly implemented
The code is working as intended because:
- The municipalities array is populated from recycling partners that have
isMunicipalityflag set to true - The
companyIdis used consistently as the value field for both municipalities and recycling partners - The mapping between municipality and recycling partner is done through the
companyIdfield, which serves as a common identifier
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if municipality IDs match company IDs in the codebase
rg -A 5 "companyId.*municipality"
Length of output: 35
Script:
#!/bin/bash
# Let's check the schema/types and usages to understand the relationship
# First, let's find type definitions related to municipalities and recycling partners
ast-grep --pattern 'interface $_Municipality {
$$$
}'
# Also check for any type with companyId
ast-grep --pattern 'interface $_ {
companyId: $_
$$$
}'
# And check how municipalities are used with recycling partners
rg -B 3 -A 3 "municipalities.*recyclingPartner"
Length of output: 2088
Script:
#!/bin/bash
# Let's check the component's props and the municipalities type
ast-grep --pattern 'type $_Props = {
$$$
municipalities: $_
$$$
}'
# Check how municipalities are structured
rg -B 2 -A 2 "municipalities.*value.*label"
# Look for recyclingPartner type definition
ast-grep --pattern 'type $_RecyclingPartner = {
$$$
}'
Length of output: 179
Script:
#!/bin/bash
# Let's check the interface definitions and type declarations
ast-grep --pattern 'interface $_ {
municipalities: $_
$$$
}'
# Look for municipality option type definitions
rg -B 3 -A 3 "type.*Municipality"
# Check how the municipalities array is populated
rg -B 3 -A 3 "municipalities.*="
Length of output: 34477
apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx (1)
119-119: Verify component integration
The replacement of Sidenav with NavigationLinks and addition of PageHeader needs verification for proper role-based access.
Also applies to: 139-139
✅ Verification successful
Let me check the NavigationLinks component implementation and its role-based access control to complete the verification.
Let me check the implementation of the role-based utility function and the user context to complete the verification.
NavigationLinks component properly handles role-based access
The NavigationLinks component correctly implements role-based access control:
- It uses UserContext to access the user's role
- Implements proper role checks using hasMunicipalityRole utility
- Conditionally renders navigation items based on user roles
- Maintains the same role-based access control as the previous Sidenav implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check NavigationLinks usage across the codebase
rg -A 5 "NavigationLinks.*activeSection"
Length of output: 3591
Script:
#!/bin/bash
# Check NavigationLinks implementation
ast-grep --pattern 'const NavigationLinks = $_'
# Check for any auth/role checks in the component
rg -A 5 "auth|role|permission" apps/skilavottord/web/components/NavigationLinks
Length of output: 6366
Script:
#!/bin/bash
# Check role utility implementation
ast-grep --pattern 'export const hasMunicipalityRole = $_'
# Check UserContext implementation
ast-grep --pattern 'export const UserContext = $_'
# Check if there are any other role-based checks
rg "user?.role" apps/skilavottord/web
Length of output: 639
apps/skilavottord/web/screens/CompanyInfo/CompanyInfo.tsx (1)
105-105: Verify PageHeader accessibility
The PageHeader component replaces direct Text components. Ensure it maintains proper heading hierarchy and accessibility.
✅ Verification successful
PageHeader component maintains proper heading hierarchy and accessibility
The PageHeader component in apps/skilavottord/web/components/PageHeader/PageHeader.tsx correctly implements heading hierarchy by using:
Textcomponent withvariant="h1"andas="h1"props- Proper semantic structure with GridRow/GridColumn for layout
- Tooltip for additional information accessibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check PageHeader implementation for accessibility attributes
ast-grep --pattern 'export const PageHeader = $_ => {
$$$
return (
$$$
<h1 $_>$$$</h1>
$$$
)
}'
Length of output: 115
Script:
#!/bin/bash
# Let's try a broader search for PageHeader component
fd PageHeader --type f --exec cat {}
# Also search for any h1 tags in the codebase to understand heading hierarchy
ast-grep --pattern '<h1 $_>$$$</h1>'
# Search for PageHeader usage patterns
rg "PageHeader" -A 2
Length of output: 57260
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanies.tsx (3)
4-31: LGTM! Clean import organization.
The new imports for municipality support and UI components are well-structured and properly organized.
Line range hint 133-152: LGTM! Clean UI component integration.
The NavigationLinks and PageHeader components are well-integrated and improve the overall UI consistency.
80-84: Verify partnerId assignment for municipality users.
The partnerId assignment logic for municipality users should be verified to ensure it correctly handles all edge cases.
✅ Verification successful
Let me check the implementation of hasMunicipalityRole and related role checks to ensure the partnerId assignment is consistent.
Let me check for the implementation of hasMunicipalityRole and the Role enum specifically for the skilavottord application.
Let me check the complete Role enum definition and the usage of municipality role in the codebase.
The partnerId assignment for municipality users is correctly implemented.
The code is consistent with the rest of the codebase. The implementation shows:
hasMunicipalityRoleis properly defined inauth/utils.tsand checks forRole.municipality- The
Roleenum inuser.model.tsincludes themunicipalityrole - The partnerId assignment pattern is used consistently across the codebase:
- In vehicle resolver:
partnerId: user.role === Role.municipality ? user.partnerId : null - In recycling company screens:
if (hasMunicipalityRole(user?.role)) { partnerId = user?.partnerId }
- In vehicle resolver:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances of partnerId assignment for municipalities
rg "partnerId.*municipality" -A 3
Length of output: 1966
Script:
#!/bin/bash
# Check implementation of hasMunicipalityRole and related role checks
ast-grep --pattern 'function hasMunicipalityRole($_) { $$$ }'
# Check Role enum definition
rg "enum.*Role" -A 10
# Check other usages of user?.role with municipality
rg "user\?\.role.*municipality" -A 2
Length of output: 66127
Script:
#!/bin/bash
# Search for Role enum in skilavottord
rg "enum.*Role" -A 5 apps/skilavottord/
# Search for hasMunicipalityRole implementation
ast-grep --pattern 'const hasMunicipalityRole = $_' apps/skilavottord/web/
# Check other role-related functions in the same context
rg "Role\." -A 2 apps/skilavottord/web/screens/RecyclingCompanies/
Length of output: 889
Script:
#!/bin/bash
# Get complete Role enum definition
rg -A 10 "export enum Role" apps/skilavottord/
# Check all usages of municipality role
rg "Role\.municipality" apps/skilavottord/
# Check implementation of hasMunicipalityRole and its imports
rg -A 3 "hasMunicipalityRole" apps/skilavottord/
Length of output: 12227
apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (2)
Line range hint 25-45: LGTM! Well-typed interface changes.
The interface updates properly accommodate the new municipality functionality with appropriate typing.
336-361: LGTM! Proper implementation of developer-only UI control.
The implementation correctly maintains the checkbox in the DOM while restricting UI manipulation to developers, as per the established pattern.
apps/skilavottord/ws/src/app/modules/recyclingRequest/recyclingRequest.service.ts (1)
Line range hint 257-267: LGTM! Proper partner validation implementation.
The partner validation logic is well-implemented with clear error messages and proper logging.
apps/skilavottord/web/i18n/locales/en.json (4)
368-372: LGTM: Navigation and titles are properly translated.
The translations for municipality-related navigation items and titles are correctly implemented in English.
Line range hint 376-396: LGTM: Recycling companies section translations are accurate.
The translations for recycling companies section, including the new municipality-related buttons and status indicators, are properly implemented in English.
Line range hint 647-667: Add missing translations in municipality section.
The municipality section is missing essential translations such as table headers, status indicators, and action buttons.
Add the following translations:
"municipalities": {
"title": "Municipalities",
+ "info": "Information about registered municipalities.",
+ "tableHeaders": {
+ "name": "Name",
+ "address": "Address",
+ "postnumber": "Postal Code",
+ "email": "Email",
+ "status": "Status"
+ },
+ "status": {
+ "active": "Active",
+ "inactive": "Inactive"
+ },
+ "buttons": {
+ "add": "Add Municipality",
+ "edit": "Edit",
+ "delete": "Delete"
+ },
"municipality": {
"view": {
"title": "Municipality",
"breadcrumb": "View",
"updated": "Updated",
"deleted": "Deleted"
},
"add": {
"title": "Add Municipality",
"breadcrumb": "Add",
"added": "Added"
}
}
}653-653:
Fix non-English content in English locale file.
These strings are in Icelandic instead of English:
- Line 653: "Þú getur annaðhvort uppfært eða eytt móttökuaðila..."
- Line 657-660: "Þegar nýju sveitarfélagi er bætt við..."
Apply this diff to fix the translations:
- "info": "Þú getur annaðhvort uppfært eða eytt móttökuaðila. Vinsamlegast staðfestið að upplýsingar fyrirtækis séu rétt slegnar inn.",
+ "info": "You can either update or delete a municipality. Please verify that the municipality information is correct.",
- "info": "Þegar nýju sveitarfélagi er bætt við, verður hann aðgengilegur í aðgangsstýringu. Vinsamlegast staðfestið að upplýsingar fyrirtækis séu rétt slegnar inn.",
+ "info": "When a new municipality is added, it will be available in access control. Please verify that the municipality information is correct.",Also applies to: 657-660
apps/skilavottord/web/i18n/locales/is.json (3)
368-372: LGTM: Navigation and titles are properly translated.
The translations for municipality-related navigation items and titles are correctly implemented in Icelandic.
486-489:
Add validation rules for municipality field.
The municipality field in the recycling company form lacks validation rules, unlike other required fields.
Apply this diff to add validation rules:
"municipality": {
"label": "Sveitarfélag",
- "placeholder": "Sveitarfélag"
+ "placeholder": "Sveitarfélag",
+ "rules": {
+ "required": "Skylda er að fylla út sveitarfélag"
+ }
}651-667:
Complete the municipality management section.
The municipality section is missing several essential elements:
- Table headers for the municipality list view
- Status indicators
- Action button translations
- Form input definitions
Add the missing translations:
"municipalities": {
"title": "Sveitarfélög",
+ "info": "Upplýsingar um skráð sveitarfélög.",
"empty": "Ekkert sveitarfélag fannst",
+ "tableHeaders": {
+ "name": "Nafn",
+ "address": "Heimilisfang",
+ "postnumber": "Póstnúmer",
+ "email": "Netfang",
+ "status": "Staða"
+ },
+ "status": {
+ "active": "Virkt",
+ "inactive": "Óvirkt"
+ },
+ "buttons": {
+ "add": "Bæta við sveitarfélagi",
+ "edit": "Breyta",
+ "delete": "Eyða"
+ },
"municipality": {
"view": {
"title": "Sveitarfélag",
"breadcrumb": "Skoða",
- "info": "Þú getur annaðhvort uppfært eða eytt móttökuaðila. Vinsamlegast staðfestið að upplýsingar fyrirtækis séu rétt slegnar inn.",
+ "info": "Þú getur annaðhvort uppfært eða eytt sveitarfélagi. Vinsamlegast staðfestið að upplýsingar sveitarfélags séu rétt slegnar inn.",
"updated": "Uppfært",
"deleted": "Eytt"
},
"add": {
"title": "Bæta við sveitarfélagi",
"breadcrumb": "Bæta við",
"info": "Þegar nýju sveitarfélagi er bætt við, verður það aðgengilegt í aðgangsstýringu. Vinsamlegast staðfestið að upplýsingar sveitarfélags séu rétt slegnar inn.",
"added": "Bætt við"
- }
+ },
+ "form": {
+ "inputs": {
+ "name": {
+ "label": "Nafn",
+ "placeholder": "Nafn sveitarfélags",
+ "rules": {
+ "required": "Skylda er að fylla út nafn"
+ }
+ },
+ "email": {
+ "label": "Netfang",
+ "placeholder": "Netfang",
+ "rules": {
+ "required": "Skylda er að fylla út netfang",
+ "validate": "Netfang er ekki á réttu formi"
+ }
+ }
+ }
+ }
}
}
...kilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx
Outdated
Show resolved
Hide resolved
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts
Outdated
Show resolved
Hide resolved
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts
Outdated
Show resolved
Hide resolved
...kilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx
Outdated
Show resolved
Hide resolved
...kilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx
Outdated
Show resolved
Hide resolved
apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx
Outdated
Show resolved
Hide resolved
apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (3)
55-66: Add error handling for the data transformationThe municipality data transformation could be more robust by handling potential undefined values and empty arrays.
Consider updating the transformation:
- const municipalities = data?.skilavottordRecyclingPartners.map( + const municipalities = data?.skilavottordRecyclingPartners?.map( (municipality) => ({ label: municipality.companyName, value: municipality.companyId, }), - ) + ) ?? []
281-315: Enhance accessibility for the municipality select componentWhile the implementation is functionally correct, consider adding ARIA attributes for better accessibility.
Add these attributes to the Select component:
<Select name={name} label={t.recyclingCompany.form.inputs.municipality.label} placeholder={t.recyclingCompany.form.inputs.municipality.placeholder} + aria-label={t.recyclingCompany.form.inputs.municipality.label} + aria-required="true" size="md" value={municipalities?.find((option) => option.value === value)} hasError={!!errors?.municipality?.message} errorMessage={errors?.municipality?.message} backgroundColor="blue" options={municipalities} onChange={onChange} isCreatable isDisabled={hasMunicipalityRole(user?.role)} />
340-365: Consider using styled components instead of inline stylesWhile the implementation correctly maintains the checkbox in the DOM as required, the visibility control could be improved.
Consider using styled components or CSS classes:
- <div - style={{ - display: user?.role && hasDeveloperRole(user.role) ? 'block' : 'none', - }} - > + <Box + display={user?.role && hasDeveloperRole(user.role) ? 'block' : 'none'} + >apps/skilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx (1)
81-84: Consider preserving form state on modal closeThe current implementation clears the form whenever the modal is reopened. Consider if this is the desired behavior or if form state should be preserved in certain scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/skilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx(5 hunks)apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx(5 hunks)apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx(5 hunks)apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
📓 Learnings (1)
apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (1)
Learnt from: birkirkristmunds
PR: island-is/island.is#17109
File: apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx:336-360
Timestamp: 2024-12-10T15:10:49.998Z
Learning: In the `RecyclingCompanyForm` component (`apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx`), the `isMunicipality` checkbox must remain in the DOM for all users since its value is stored in the database. However, only users with the developer role can manipulate it through the UI. This ensures that the checkbox value is maintained correctly while restricting interaction to authorized users.
🔇 Additional comments (8)
apps/skilavottord/web/screens/RecyclingCompanies/components/RecyclingCompanyForm/RecyclingCompanyForm.tsx (2)
Line range hint 1-48: LGTM! Well-structured imports and interface updates
The new imports and interface changes are well-organized and properly typed. The optional isMunicipalityPage property with its default value follows TypeScript best practices.
379-380: LGTM! Improved button semantics
The button changes from 'arrowForward' to 'save' icon and updated text provide better clarity about the action being performed.
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (3)
22-27: Method name should match GraphQL query name
The method name getAllRecyclingPartners doesn't match the GraphQL query name skilavottordAllRecyclingPartners. This can lead to confusion during maintenance.
@Query(() => [RecyclingPartnerModel], {
name: 'skilavottordAllRecyclingPartners',
})
-async getAllRecyclingPartners(): Promise<RecyclingPartnerModel[]> {
+async skilavottordAllRecyclingPartners(): Promise<RecyclingPartnerModel[]> {
return this.recyclingPartnerService.findAll()
}32-45: Add input validation and error handling
The method should validate inputs and include proper error handling.
async getRecyclingPartners(
@Args('isMunicipalityPage', { type: () => Boolean, nullable: true })
isMunicipalityPage: boolean,
@Args('municipalityId', { type: () => String, nullable: true })
municipalityId: string | null,
): Promise<RecyclingPartnerModel[]> {
+ if (municipalityId && !municipalityId.trim()) {
+ throw new BadRequestException('municipalityId cannot be empty')
+ }
+ try {
return this.recyclingPartnerService.findRecyclingPartners(
isMunicipalityPage,
municipalityId,
)
+ } catch (error) {
+ throw new InternalServerErrorException(
+ `Failed to fetch recycling partners: ${error.message}`,
+ )
+ }
}20-20: Verify the municipality role permissions across the application
The addition of Role.municipality to multiple endpoints needs verification to ensure proper access control implementation.
Also applies to: 30-30, 55-55, 66-66, 87-87
✅ Verification successful
Let me gather more information about the authorization implementation to ensure proper role handling.
Let me check the authorization guard implementation to verify how the municipality role is handled.
Role.municipality authorization is properly implemented
The municipality role has been consistently implemented across the application:
- Correctly defined in the Role enum (apps/skilavottord/ws/src/app/modules/auth/user.model.ts)
- Properly used in @authorize decorators across multiple resolvers
- Specific handling in the recyclingPartner service for municipality-specific queries
- Access control is consistently applied in related modules (vehicle, recyclingPartner, accessControl)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent role implementation and guard usage
# Check for Role.municipality usage
rg -A 2 "Role\.municipality"
# Check for proper guard implementation
ast-grep --pattern 'class $_ implements CanActivate {
$$$
Role.municipality
$$$
}'
Length of output: 5947
Script:
#!/bin/bash
# Check for the authorization guard implementation
ast-grep --pattern 'class AuthorizeGuard implements CanActivate {
$$$
}'
# Check for role enum definition
rg -A 5 "export enum Role"
# Check for any custom municipality-specific guards
rg -l "municipality.*guard"
Length of output: 30419
Script:
#!/bin/bash
# Check for the authorization guard implementation
rg -A 10 "class.*Authorize.*Guard"
# Check for municipality-specific authorization logic
rg -A 5 "canActivate.*municipality"
# Check for the Authorize decorator implementation
ast-grep --pattern '@Authorize({
$$$
})'
Length of output: 5084
apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx (2)
91-94: Duplicate: Handle potential undefined role in partnerIdRequired
When using watch('role')?.value, ensure that watch('role') doesn't return undefined before accessing .value. Consider setting a default value or adding additional null checks.
57-60: Verify municipality mapping logic
The code assumes that municipality's companyId matches the recyclingPartner's companyId. This might lead to incorrect municipality assignments if the IDs don't match.
✅ Verification successful
Let me gather more information about the municipality and recycling partner data structures to verify this relationship.
Let me check one more thing to understand how the recycling partners are structured and their relationship with municipalities.
Based on the gathered evidence, I can now provide the final response:
The municipality mapping logic is correct and safe
The code correctly maps recycling partners to municipalities. The evidence shows that:
- The
skilavottordRecyclingPartnersquery includes municipality relationships and is used consistently across the codebase - The mapping between
companyIdand municipalities is handled properly in the RecyclingCompanyForm component, establishing the correct relationship - The same pattern is used in both AccessControl components, ensuring consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if municipality and recycling partner IDs match the assumed pattern
ast-grep --pattern 'companyId: string' # Search for company ID type definitions
rg -A 2 'municipality.*companyId|recyclingPartner.*companyId' # Search for ID usage patterns
Length of output: 2324
Script:
#!/bin/bash
# Check municipality and recycling partner type definitions and data transformations
ast-grep --pattern 'interface Municipality {
$$$
}'
ast-grep --pattern 'interface RecyclingPartner {
$$$
}'
# Check how municipalities are mapped in the codebase
rg -A 3 'municipalities.*map'
Length of output: 4958
Script:
#!/bin/bash
# Check recycling partner data structure and municipality relationships
ast-grep --pattern 'type RecyclingPartnerDTO = {
$$$
}'
# Look for municipality-recycling partner relationship definitions
rg -A 3 'skilavottordRecyclingPartners'
Length of output: 2896
apps/skilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx (1)
50-62:
Improve error handling in getPartnerId function
The function needs additional safeguards:
- Missing null check for
user?.partnerId - No error handling for invalid role combinations
const getPartnerId = (
- municipalityId: string,
- partnerId: string,
+ municipalityId: string | null | undefined,
+ partnerId: string | null | undefined,
role: Role,
) => {
+ if (!user) {
+ throw new Error('User context is required')
+ }
+
// If the user has municipality role, then he can only create a new access under the same municipality
if (hasMunicipalityRole(user?.role) && hasMunicipalityRole(role)) {
- return user.partnerId
+ return user.partnerId ?? null
}
// If selected role is municipality, use municipalityId, else use partnerId
- return hasMunicipalityRole(role) ? municipalityId : partnerId || null
+ return hasMunicipalityRole(role) ? municipalityId ?? null : partnerId ?? null
}Likely invalid or redundant comment.
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts
Show resolved
Hide resolved
...kilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx
Outdated
Show resolved
Hide resolved
...kilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx (1)
97-100:⚠️ Potential issueAdd null check for role value
The role value check should handle undefined cases.
partnerIdRequired={ - watch('role')?.value === Role.recyclingCompanyAdmin || - watch('role')?.value === Role.recyclingCompany + (watch('role')?.value === Role.recyclingCompanyAdmin || + watch('role')?.value === Role.recyclingCompany) ?? false }
🧹 Nitpick comments (15)
apps/skilavottord/web/utils/accessUtils.ts (3)
4-9: Add JSDoc documentation for better type information and usage guidance.The function signature is well-typed, but adding JSDoc would improve developer experience by documenting parameter requirements and return values.
+/** + * Determines the appropriate partner ID based on user role and provided parameters + * @param user - The current user object + * @param municipalityId - The ID of the municipality if applicable + * @param partnerId - The partner ID if applicable + * @param role - The role being evaluated + * @returns The determined partner ID or null + */ export const getPartnerId = ( user: SkilavottordUser | undefined, municipalityId: string | null, partnerId: string | null, role: Role, ) => {
10-13: Consider making the role check more explicit.The conditional logic is correct, but the comment could be more precise about the business rule being enforced.
- // If the user has municipality role, then he can only create a new access under the same municipality + // Municipality users can only create access within their assigned municipality if (hasMunicipalityRole(user?.role) && hasMunicipalityRole(role)) { return user.partnerId }
15-17: Consider making null coalescing more explicit with early returns.The ternary with null coalescing could be more readable with explicit conditions.
- return hasMunicipalityRole(role) ? municipalityId ?? null : partnerId ?? null + if (hasMunicipalityRole(role)) { + return municipalityId ?? null + } + return partnerId ?? nullapps/skilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx (1)
85-89: Simplify the partnerIdRequired logicThe current implementation can be made more concise and readable.
- partnerIdRequired={ - (watch('role')?.value === Role.recyclingCompanyAdmin || - watch('role')?.value === Role.recyclingCompany) ?? - false - } + partnerIdRequired={[Role.recyclingCompanyAdmin, Role.recyclingCompany].includes(watch('role')?.value ?? '')}apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts (1)
81-96: Enhance JSDoc documentationWhile the implementation is good, the JSDoc comments could be more descriptive.
/** * Get the id of the municipality or recyclingpartner that is paying for the recycling request - * @param companyId - * @returns + * @param {string} companyId - The unique identifier of the company + * @returns {Promise<string>} The ID of the paying partner (municipality or recycling partner) + * @throws {Error} When no partner is found for the given company ID */apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx (1)
61-64: Simplify municipality ID lookupThe find logic can be made more concise.
- municipalityId: municipalities.find( - (option) => - option.value === currentPartner?.recyclingPartner?.companyId, - ), + municipalityId: municipalities.find(option => option.value === currentPartner?.recyclingPartner?.companyId),apps/skilavottord/web/i18n/locales/en.json (2)
395-396: Consider more descriptive button labels.The "view" label could be more descriptive. Consider using "View details" or "View company" to provide more context to users.
647-662: Enhance descriptive text for municipality management.Consider adding more context to the following strings:
- Add a description of what municipalities are used for in the system
- Clarify the impact of adding/updating municipalities
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (1)
37-56: Add parameter documentation and validationWhile the implementation looks good, consider these improvements:
- Add JSDoc comments to document the parameters and their relationship
- Add validation for the case when municipalityId is provided
@Query(() => [RecyclingPartnerModel], { name: 'skilavottordRecyclingPartners', }) +/** + * Retrieves recycling partners based on municipality page status and ID + * @param isMunicipalityPage - Whether to fetch municipality-specific partners + * @param municipalityId - Optional municipality ID, can be null for recyclingFund role + */ async skilavottordRecyclingPartners( @Args('isMunicipalityPage', { type: () => Boolean, nullable: true }) isMunicipalityPage: boolean, @Args('municipalityId', { type: () => String, nullable: true }) municipalityId: string | null, ): Promise<RecyclingPartnerModel[]> { try { + if (municipalityId && !isMunicipalityPage) { + throw new BadRequestException( + 'municipalityId can only be provided when isMunicipalityPage is true', + ) + } return this.recyclingPartnerService.findRecyclingPartners( isMunicipalityPage, municipalityId, )apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (2)
15-19: Consider grouping related auth importsThe auth-related imports (
hasPermission,hasMunicipalityRole) could be combined into a single import statement for better organization.-import { - hasPermission, - hasMunicipalityRole, -} from '@island.is/skilavottord-web/auth/utils' +import { hasPermission, hasMunicipalityRole } from '@island.is/skilavottord-web/auth/utils'
161-161: Consider alternative to delete operatorUsing the delete operator can impact performance. Consider using undefined assignment instead.
-delete input.__typename +input.__typename = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 161-161: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts (3)
26-27: Add explicit type annotation for better type safetyThe
partnerIdsvariable should have an explicit type annotation to prevent potential type-related issues.- let partnerIds = [] + let partnerIds: string[] | null = []
28-48: Improve comment clarity for municipality partner handlingThe implementation looks good with proper error handling and query optimization. However, the comments could be more descriptive.
- // Get all sub recycling partners of the municipality - // else get all + // If partnerId is provided, fetch all sub-recycling partners associated with the municipality + // Otherwise, return null to fetch all partners
61-64: Fix indentation for better readabilityThe object spread operator's indentation is inconsistent with the surrounding code.
...(partnerIds ? { - recyclingPartnerId: partnerIds, + recyclingPartnerId: partnerIds, } : {}),apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (1)
122-130: Refactor duplicated success handlingThe success handling contains duplicated toast message code.
Apply this diff to reduce code duplication:
if (!errors) { - if (isMunicipalityPage) { - router.push(routes.municipalities.baseRoute).then(() => { - toast.success(t.recyclingCompany.add.added) - }) - } else { - router.push(routes.recyclingCompanies.baseRoute).then(() => { - toast.success(t.recyclingCompany.add.added) - }) - } + const route = isMunicipalityPage + ? routes.municipalities.baseRoute + : routes.recyclingCompanies.baseRoute + router.push(route).then(() => { + toast.success(t.recyclingCompany.add.added) + }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/skilavottord/web/i18n/locales/en.json(5 hunks)apps/skilavottord/web/i18n/locales/is.json(6 hunks)apps/skilavottord/web/screens/AccessControl/AccessControl.tsx(12 hunks)apps/skilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx(5 hunks)apps/skilavottord/web/screens/AccessControl/components/AccessControlModal/AccessControlModal.tsx(4 hunks)apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx(6 hunks)apps/skilavottord/web/screens/Overview/Overview.tsx(1 hunks)apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx(5 hunks)apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx(7 hunks)apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx(4 hunks)apps/skilavottord/web/utils/accessUtils.ts(1 hunks)apps/skilavottord/web/utils/index.ts(1 hunks)apps/skilavottord/web/utils/roleUtils.ts(1 hunks)apps/skilavottord/ws/migrations/202412013210715-addcolumns-recycling-partners-municipality.js(1 hunks)apps/skilavottord/ws/src/app/modules/accessControl/accessControl.resolver.ts(2 hunks)apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.input.ts(2 hunks)apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts(4 hunks)apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts(3 hunks)apps/skilavottord/ws/src/app/modules/recyclingRequest/recyclingRequest.service.ts(4 hunks)apps/skilavottord/ws/src/app/modules/vehicle/vehicle.resolver.ts(1 hunks)apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/skilavottord/web/screens/Overview/Overview.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/skilavottord/ws/migrations/202412013210715-addcolumns-recycling-partners-municipality.js
- apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.input.ts
- apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx
- apps/skilavottord/ws/src/app/modules/recyclingRequest/recyclingRequest.service.ts
- apps/skilavottord/ws/src/app/modules/vehicle/vehicle.resolver.ts
👮 Files not reviewed due to content moderation or server errors (3)
- apps/skilavottord/ws/src/app/modules/accessControl/accessControl.resolver.ts
- apps/skilavottord/web/screens/AccessControl/components/AccessControlModal/AccessControlModal.tsx
- apps/skilavottord/web/screens/AccessControl/AccessControl.tsx
🧰 Additional context used
📓 Path-based instructions (15)
apps/skilavottord/web/utils/index.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/utils/accessUtils.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/utils/roleUtils.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControl/components/AccessControlModal/AccessControlModal.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/accessControl/accessControl.resolver.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControl/AccessControl.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/i18n/locales/en.json (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/AccessControl/components/AccessControlUpdate/AccessControlUpdate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/i18n/locales/is.json (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
📓 Learnings (2)
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (1)
Learnt from: birkirkristmunds
PR: island-is/island.is#17109
File: apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts:32-45
Timestamp: 2024-12-16T12:25:52.236Z
Learning: In `apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts`, the `municipalityId` parameter in `getRecyclingPartners` can be `null` for users with the `recyclingFund` role.
apps/skilavottord/web/i18n/locales/is.json (1)
Learnt from: birkirkristmunds
PR: island-is/island.is#17109
File: apps/skilavottord/web/i18n/locales/is.json:0-0
Timestamp: 2024-12-16T12:38:27.802Z
Learning: In the Skilavottord web application, the form for recycling partners is reused for municipalities. Therefore, specific translations for municipalities' form inputs are not needed in `apps/skilavottord/web/i18n/locales/is.json`.
🪛 Biome (1.9.4)
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx
[error] 161-161: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (21)
apps/skilavottord/web/utils/index.ts (1)
5-5: LGTM! Export statement follows established pattern.
The new export follows the same pattern as other utility exports and maintains consistency in the barrel file structure.
apps/skilavottord/web/utils/roleUtils.ts (1)
18-19: Verify translation completeness across the application.
The municipality role translations are properly added. However, we should verify that these translations are consistently used across all relevant components.
✅ Verification successful
Translations for municipality role are consistent across the application
The verification shows that:
- All necessary translations for "Municipality"/"Sveitarfélag" are properly defined in both English (en.json) and Icelandic (is.json) locale files
- The Role.municipality usage is consistent across components, particularly in:
- AccessControl components for role checks
- AccessControlModal for role-specific logic
- roleUtils.ts for translation handling
- auth/utils.ts for role verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in municipality translations across the application
# Search for municipality-related translations in locale files
echo "Checking locale files for municipality translations..."
rg -i "municipality|sveitarfélag" apps/skilavottord/web/i18n/locales/
# Search for municipality role usage in components
echo "Checking components for municipality role usage..."
rg "Role\.municipality" apps/skilavottord/web/
Length of output: 4368
apps/skilavottord/web/screens/AccessControl/components/AccessControlCreate/AccessControlCreate.tsx (1)
68-71: LGTM! Good form reset implementation
The useEffect hook correctly resets the form when the modal is re-opened, with proper dependency array.
apps/skilavottord/web/i18n/locales/en.json (2)
368-372: LGTM: Navigation items for municipality management added correctly.
The new navigation items for municipality management are properly structured and consistent with the application's navigation pattern.
641-645: LGTM: Municipality routes added with consistent pattern.
The new routes for municipality management follow the established pattern used for other sections.
apps/skilavottord/web/i18n/locales/is.json (4)
368-372: LGTM: Municipality navigation items properly translated.
The Icelandic translations for municipality navigation items are accurate and consistent with the English version.
587-592: LGTM: Municipality field properly added to access control.
The municipality field in the access control section is properly structured with consistent validation rules.
646-650: LGTM: Municipality routes properly added.
The routes for municipality management are properly structured and consistent with other sections.
659-659:
Fix incorrect entity reference in municipality view.
The info text incorrectly refers to "móttökuaðila" (recycling company) instead of "sveitarfélag" (municipality).
Apply this fix:
-"info": "Þú getur annaðhvort uppfært eða eytt móttökuaðila. Vinsamlegast staðfestið að upplýsingar fyrirtækis séu rétt slegnar inn.",
+"info": "Þú getur annaðhvort uppfært eða eytt sveitarfélagi. Vinsamlegast staðfestið að upplýsingar sveitarfélags séu rétt slegnar inn.",Likely invalid or redundant comment.
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts (3)
1-6: LGTM: Appropriate error handling imports
The added imports from @nestjs/common provide the necessary error types for proper error handling throughout the resolver.
27-31: Maintain consistent method naming convention
The method name doesn't match the GraphQL query name pattern. The query uses the 'skilavottord' prefix while the method name doesn't.
@Query(() => [RecyclingPartnerModel], {
name: 'skilavottordAllRecyclingPartners',
})
-async getAllRecyclingPartners(): Promise<RecyclingPartnerModel[]> {
+async skilavottordAllRecyclingPartners(): Promise<RecyclingPartnerModel[]> {25-25: Verify role-based access control implementation
The addition of Role.municipality to authorized roles is consistent across all methods. However, we should verify the implementation of this role in the auth module and any UI components that use these endpoints.
Also applies to: 66-66, 77-77, 98-98
✅ Verification successful
Role.municipality implementation is properly integrated
The verification shows that the Role.municipality is:
- Correctly defined in the Role enum
- Consistently applied across all methods in recyclingPartner.resolver.ts
- Properly integrated in other resolvers and services with appropriate access controls
- Used in logical branching for specific municipality-related functionality
The implementation is complete and consistent with the codebase's role-based access control system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify role implementation and usage
# Check role definition
echo "Checking Role enum definition..."
rg -A 5 "export enum Role" apps/skilavottord/ws/src/app
# Check auth guard implementation
echo "Checking auth guard implementation..."
rg -A 10 "canActivate.*Role.municipality" apps/skilavottord/ws/src/app
# Check UI components using these endpoints
echo "Checking UI components..."
rg -A 5 "skilavottordRecyclingPartners|skilavottordAllRecyclingPartners" apps/skilavottord/web/src
Length of output: 1050
Script:
#!/bin/bash
# Let's verify the role implementation more thoroughly
# Check if municipality role is defined in the Role enum
echo "Checking Role enum for municipality..."
rg -p "municipality.*=.*'municipality'" apps/skilavottord/ws/src/app
# Check all usages of Role.municipality in the codebase
echo "Checking Role.municipality usage..."
rg -p "Role\.municipality" apps/skilavottord/ws/src/app
# Check auth guards and decorators
echo "Checking auth guards and decorators..."
rg -p "@UseGuards|@Roles" apps/skilavottord/ws/src/app/modules
# Check recyclingPartner resolver for role consistency
echo "Checking recyclingPartner resolver..."
rg -p "roles.*:.*\[.*\]" apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts
Length of output: 2450
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx (3)
152-157: LGTM! Clean implementation of navigation logic
The extracted navigation function improves code maintainability by removing duplication, as suggested in previous reviews.
163-167: LGTM! Robust type handling for municipalityId
The implementation follows the suggested approach from previous reviews for safer type handling.
87-91: Verify role-based access control implementation
The municipality role check is crucial for security. Let's verify that this implementation correctly restricts access.
✅ Verification successful
Let me analyze the role-based access control implementation further by checking the actual implementation of hasMunicipalityRole.
Based on the analysis of the code, I can now provide a final response:
Role-based access control implementation is correctly implemented
The implementation is secure and properly restricts access based on municipality roles:
- The
hasMunicipalityRolefunction is a simple, clear comparison that checks if the user's role exactly matchesRole.municipality - The Role enum is properly defined with distinct values for different roles including 'municipality'
- The role check is used consistently across the codebase to restrict access to municipality-specific functionality
- The partnerId assignment is properly guarded by the role check, ensuring only municipality users can access their specific data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of hasMunicipalityRole to ensure consistent usage
rg "hasMunicipalityRole" --type ts
# Search for role-based access patterns
ast-grep --pattern 'if ($CONDITION) { $PARTNER_ID = $USER.$FIELD }'
Length of output: 44435
Script:
#!/bin/bash
# Check the implementation of hasMunicipalityRole
ast-grep --pattern 'export const hasMunicipalityRole = $_'
# Check the Role enum definition
rg "enum Role" -A 10 --type ts
Length of output: 65785
apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts (1)
33-36: Verify database index for optimal query performance
The query filtering on municipalityId could benefit from an index to improve performance, especially with large datasets.
✅ Verification successful
Database query performance is adequately optimized
While there isn't a direct index on municipality_id, the current query pattern doesn't require one for optimal performance because:
- The table has an index on
is_municipalitywhich helps filter recycling partners efficiently - The
municipality_idcolumn was recently added (December 2024) and is used in a simple equality filter - The result set is limited to only the
companyIdattribute, reducing data transfer
The current implementation is sufficient for the expected data volume and query pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if an index exists on the municipalityId column
# Search for index definitions in migration files
fd -e ts -e js . | xargs rg -A 5 "CREATE.*INDEX.*municipalityId"
# Search for Sequelize index definitions
ast-grep --pattern 'index: {
$$$
fields: [$$$"municipalityId"$$$]
$$$
}'
Length of output: 147
Script:
#!/bin/bash
# Let's try a broader search to understand the database schema and existing indexes
# Search for the RecyclingPartner model definition
ast-grep --pattern 'class RecyclingPartnerModel extends Model {
$$$
}'
# Search for any migrations related to recycling partners
fd -e ts -e js . | xargs rg -B 2 -A 5 "createTable.*recycling.*partner"
# Look for any index definitions in a case-insensitive way
fd -e ts -e js . | xargs rg -i "index.*municipality"
Length of output: 11049
Script:
#!/bin/bash
# Let's check for any migrations related to municipalityId specifically
fd -e ts -e js . | xargs rg -B 2 -A 5 "municipality_id"
# Look for the specific migration file that added the municipality_id column
fd "202412013210715-addcolumns-recycling-partners-municipality.js" . | xargs cat
Length of output: 20304
apps/skilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx (5)
19-20: LGTM: New components properly imported
The NavigationLinks and PageHeader components are correctly imported from their respective paths.
58-73: LGTM: Clean separation of municipality logic
The code clearly differentiates between municipality and regular recycling company contexts, with proper variable assignments based on the route.
94-95: LGTM: Proper refetch query configuration
The refetch query is correctly configured with the necessary variables for municipality filtering.
143-143: LGTM: Clean component structure and proper prop handling
The component follows NextJS best practices with:
- Proper layout structure using PartnerPageLayout
- Clean implementation of PageHeader
- Correct prop passing to RecyclingCompanyForm
Also applies to: 167-167, 175-175
39-40: Verify GraphQL schema compatibility
The mutation has been updated to include municipality-related fields. Ensure these fields are properly defined in the GraphQL schema.
✅ Verification successful
GraphQL schema compatibility verified
The fields isMunicipality and municipalityId are properly defined in the GraphQL schema through the RecyclingPartnerModel class using @Field decorators. The model shows:
isMunicipalityis defined as a non-nullable Boolean fieldmunicipalityIdis defined as a nullable String field
Both fields are properly mapped to the database schema and exposed through GraphQL.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new fields are defined in the GraphQL schema
ast-grep --pattern 'type RecyclingPartner {
$$$
isMunicipality: Boolean
municipalityId: String
$$$
}'
Length of output: 111
Script:
#!/bin/bash
# Let's search for GraphQL schema files and check their content
fd -e graphql -x cat {}
# Also search for type definitions in TypeScript files as they might be using code-first approach
ast-grep --pattern 'type RecyclingPartner'
Length of output: 81450
Script:
#!/bin/bash
# Let's search for RecyclingPartner type definition in TypeScript files
ast-grep --pattern 'interface RecyclingPartner {
$$$
}'
# Also search for any GraphQL type definitions in .graphql files
fd -e graphql -e ts -x grep -l "RecyclingPartner" {}
# Let's also check for any mutation types that might use these fields
ast-grep --pattern 'mutation $_ {
$$$
RecyclingPartner {
$$$
isMunicipality
municipalityId
$$$
}
$$$
}'
Length of output: 2315
Script:
#!/bin/bash
# Let's check the recyclingPartner model and resolver for these fields
cat apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.model.ts
cat apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.resolver.ts
Length of output: 5681
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts
Outdated
Show resolved
Hide resolved
...kilavottord/web/screens/RecyclingCompanies/RecyclingCompanyUpdate/RecyclingCompanyUpdate.tsx
Show resolved
Hide resolved
...kilavottord/web/screens/RecyclingCompanies/RecyclingCompanyCreate/RecyclingCompanyCreate.tsx
Outdated
Show resolved
Hide resolved
birkirkristmunds
added
automerge
|
Affected services are: api,application-system-api,skilavottord-ws,skilavottord-web, Deployed services: application-system-api,api,skilavottord-web,skilavottord-ws,application-system-api-worker,consultation-portal,web,services-bff-portals-admin,services-bff-portals-my-pages. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts (3)
33-33: Update comment to match parameter namesThe comment references
Role.municipalitybut the parameter is namedisMunicipalityPage. Update the comment to match the actual parameter names for better clarity.- // If Role.municipality, return all recycling partners that are not municipalities + // If municipalityId is provided, return all recycling partners that are not municipalities
28-56: Add input validation and improve error handlingWhile the implementation is generally good, consider adding input validation and more specific error handling.
async findRecyclingPartners( isMunicipalityPage: boolean, municipalityId: string | null | undefined, ): Promise<RecyclingPartnerModel[]> { try { + // Validate municipalityId format if provided + if (municipalityId && !/^[A-Za-z0-9-]+$/.test(municipalityId)) { + throw new Error('Invalid municipality ID format'); + } + if (municipalityId) { return await this.recyclingPartnerModel.findAll({ where: { @@ -53,6 +58,8 @@ }, }) } catch (error) { + if (error.name === 'SequelizeConnectionError') + throw new Error('Database connection error'); throw new Error(`Failed to fetch recycling partners: ${error.message}`) } }
85-100: Enhance type safety with type narrowingThe implementation is good, but we can improve type safety.
async getPayingPartnerId(companyId: string): Promise<string> { + if (!companyId?.trim()) { + throw new Error('Company ID is required'); + } + const partner = await this.recyclingPartnerModel.findOne({ where: { companyId }, }) if (!partner) { throw new Error(`Partner not found for company ID: ${companyId}`) } + // Ensure both values are strings before using nullish coalescing + const municipalityId = partner.municipalityId?.toString(); + const partnerCompanyId = partner.companyId?.toString(); + + if (!municipalityId && !partnerCompanyId) { + throw new Error('Invalid partner data: missing both municipalityId and companyId'); + } + - return partner.municipalityId ?? partner.companyId + return municipalityId ?? partnerCompanyId; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/skilavottord/README.md(2 hunks)apps/skilavottord/web/i18n/locales/is.json(6 hunks)apps/skilavottord/web/i18n/locales/locales.spec.ts(2 hunks)apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/skilavottord/web/i18n/locales/locales.spec.ts
- apps/skilavottord/README.md
🧰 Additional context used
📓 Path-based instructions (2)
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/i18n/locales/is.json (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
📓 Learnings (1)
apps/skilavottord/web/i18n/locales/is.json (1)
Learnt from: birkirkristmunds
PR: island-is/island.is#17109
File: apps/skilavottord/web/i18n/locales/is.json:0-0
Timestamp: 2024-12-16T12:38:27.802Z
Learning: In the Skilavottord web application, the form for recycling partners is reused for municipalities. Therefore, specific translations for municipalities' form inputs are not needed in `apps/skilavottord/web/i18n/locales/is.json`.
🔇 Additional comments (5)
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.service.ts (1)
9-9: LGTM!
The Sequelize Op import is correctly placed and necessary for the logical operations in the new methods.
apps/skilavottord/web/i18n/locales/is.json (4)
368-372: LGTM! Navigation translations are well-structured.
The new municipality-related navigation items are properly integrated into the existing structure.
587-592: LGTM! Access control translations are complete and consistent.
The municipality field in the access control modal follows the established pattern for validation rules and translations.
646-650: LGTM! Route definitions are well-structured.
The municipality routes follow the established pattern and include all necessary paths.
486-489:
Fix validation message format inconsistency.
The validation message format for the municipality field is inconsistent with other fields.
Apply this fix:
"municipality": {
"label": "Sveitarfélag",
"placeholder": "Sveitarfélag",
- "required": "Skylda er að fylla út sveitarfélag"
+ "rules": {
+ "required": "Skylda er að fylla út sveitarfélag"
+ }
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/skilavottord/web/i18n/locales/is.json (1)
486-489:⚠️ Potential issueFix validation message format inconsistency.
The validation message format for the municipality field is inconsistent with other fields.
"municipality": { "label": "Sveitarfélag", "placeholder": "Sveitarfélag", - "required": "Skylda er að fylla út sveitarfélag" + "rules": { + "required": "Skylda er að fylla út sveitarfélag" + } }
🧹 Nitpick comments (1)
apps/skilavottord/web/i18n/locales/is.json (1)
376-376: Consider improving button accessibility.The buttons section could benefit from additional ARIA labels for better screen reader support.
"buttons": { - "add": "Bæta við móttökuaðila", - "view": "Skoða", - "addMunicipality": "Bæta við sveitarfélagi" + "add": { + "text": "Bæta við móttökuaðila", + "ariaLabel": "Bæta við nýjum móttökuaðila" + }, + "view": { + "text": "Skoða", + "ariaLabel": "Skoða upplýsingar" + }, + "addMunicipality": { + "text": "Bæta við sveitarfélagi", + "ariaLabel": "Bæta við nýju sveitarfélagi" + } }Also applies to: 395-396
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/skilavottord/web/i18n/locales/is.json(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/skilavottord/web/i18n/locales/is.json (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
📓 Learnings (1)
apps/skilavottord/web/i18n/locales/is.json (1)
Learnt from: birkirkristmunds
PR: island-is/island.is#17109
File: apps/skilavottord/web/i18n/locales/is.json:0-0
Timestamp: 2024-12-16T12:38:27.802Z
Learning: In the Skilavottord web application, the form for recycling partners is reused for municipalities. Therefore, specific translations for municipalities' form inputs are not needed in `apps/skilavottord/web/i18n/locales/is.json`.
🔇 Additional comments (3)
apps/skilavottord/web/i18n/locales/is.json (3)
368-372: LGTM: Navigation structure is well-organized.
The navigation items are logically grouped and consistently named, with clear separation between municipalities and companies.
587-592: LGTM: Access control validation is properly structured.
The municipality field in access control follows the correct validation pattern.
646-668: LGTM: Municipality section is well-structured.
The municipality section follows the established pattern for entity management, with proper breadcrumbs, actions, and status messages.
- Add municipality to overview
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx (1)
Line range hint
65-70: Add error handling for GraphQL query failures.The query implementation should handle potential GraphQL errors to provide better user feedback.
const { data, loading, fetchMore } = useQuery<Query>( SkilavottordVehiclesQuery, { notifyOnNetworkStatusChange: true, variables: { after: '' }, + onError: (error) => { + // Handle error appropriately + console.error('Failed to fetch vehicles:', error); + } }, )
🧹 Nitpick comments (4)
apps/skilavottord/web/i18n/locales/is.json (1)
383-383: Consider enhancing the info text description.While the translations are correct, the info text "Skráðir móttökuaðilar" could be more descriptive to match the English version's context about helping customers.
- "info": "Skráðir móttökuaðilar", + "info": "Upplýsingar um skráða móttökuaðila sem hjálpa viðskiptavinum að nýta sér þjónustuna.",Also applies to: 402-403
apps/skilavottord/ws/src/app/modules/municipality/municipality.model.ts (1)
7-20: Model definition looks good.The model aligns with Sequelize best practices. Ensure indices in the underlying table optimize lookups by ID or other unique fields.
apps/skilavottord/web/screens/RecyclingFund/Overview/components/CarsTable/CarsTable.tsx (2)
Line range hint
6-9: Consider using a more specific type for table titles.The
titlesarray could benefit from a literal type that matches the exact columns being displayed, improving type safety and making the component more maintainable.+type TableColumn = 'vehicleId' | 'vehicleType' | 'modelYear' | 'nameOfRequestor' | 'municipality' | 'deregistrationDate' interface TableProps { - titles: string[] + titles: TableColumn[] vehicles: Vehicle[] }
30-50: Consider optimizing the nested mapping for performance.The double mapping operation (vehicles → recyclingRequests) could impact performance with large datasets.
Consider flattening the data structure before rendering:
const flattenedData = vehicles.flatMap((vehicle) => (vehicle.recyclingRequests ?? []).map((request) => ({ ...vehicle, request, })) ); // Then map over flattenedData instead of the nested mapping
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/skilavottord/web/i18n/locales/en.json(5 hunks)apps/skilavottord/web/i18n/locales/is.json(6 hunks)apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx(5 hunks)apps/skilavottord/web/screens/RecyclingFund/Overview/components/CarsTable/CarsTable.tsx(2 hunks)apps/skilavottord/ws/migrations/202412013210715-add-view-municipality.js(1 hunks)apps/skilavottord/ws/src/app/modules/municipality/index.ts(1 hunks)apps/skilavottord/ws/src/app/modules/municipality/municipality.model.ts(1 hunks)apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.model.ts(2 hunks)apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.module.ts(1 hunks)apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/skilavottord/ws/src/app/modules/municipality/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.model.ts
🧰 Additional context used
📓 Path-based instructions (8)
apps/skilavottord/web/screens/RecyclingFund/Overview/components/CarsTable/CarsTable.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/municipality/municipality.model.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/migrations/202412013210715-add-view-municipality.js (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.module.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/i18n/locales/en.json (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/web/i18n/locales/is.json (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
📓 Learnings (1)
apps/skilavottord/web/i18n/locales/is.json (1)
Learnt from: birkirkristmunds
PR: island-is/island.is#17109
File: apps/skilavottord/web/i18n/locales/is.json:0-0
Timestamp: 2024-12-16T12:38:27.802Z
Learning: In the Skilavottord web application, the form for recycling partners is reused for municipalities. Therefore, specific translations for municipalities' form inputs are not needed in `apps/skilavottord/web/i18n/locales/is.json`.
🪛 Biome (1.9.4)
apps/skilavottord/ws/migrations/202412013210715-add-view-municipality.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (23)
apps/skilavottord/web/i18n/locales/en.json (5)
364-371: LGTM! Table columns updated to include municipality.
The addition of the "Municipality" column in the recycled vehicles table aligns with the new municipality management feature.
375-375: LGTM! Navigation items added for municipality management.
The addition of municipality-related navigation items provides access to the new municipality management features.
Also applies to: 378-379
402-403: LGTM! Buttons added for municipality management.
The addition of the "Add municipality" button enables associating municipalities with recycling companies.
648-652: LGTM! Routes added for municipality management.
The addition of municipality routes enables Create, Read, and Update operations for municipalities.
Line range hint 654-675: LGTM! Municipality management translations added.
Complete set of translations added for municipality management features, including views, forms, and status messages.
apps/skilavottord/web/i18n/locales/is.json (6)
364-371: LGTM! Table columns updated with municipality translation.
The Icelandic translation "Sveitarfélag" is correctly added to the table columns.
375-375: LGTM! Municipality navigation items translated correctly.
The translations "Sveitarfélag" and "Sveitarfélög" are appropriate for the navigation items.
Also applies to: 378-379
594-599: LGTM! Municipality field added to access control.
The municipality field is correctly implemented with proper validation rules and translations.
653-657: LGTM! Municipality routes added correctly.
The routes for municipality management are properly defined and consistent with the English version.
659-675: LGTM! Municipality management translations are complete and accurate.
The translations provide comprehensive coverage for all municipality management features, with clear and appropriate Icelandic text.
493-496: 🛠️ Refactor suggestion
Fix validation rules format for municipality field.
The validation rules format for the municipality field is inconsistent with other fields in the form.
"municipality": {
"label": "Sveitarfélag",
"placeholder": "Sveitarfélag",
- "required": "Skylda er að fylla út sveitarfélag"
+ "rules": {
+ "required": "Skylda er að fylla út sveitarfélag"
+ }
}Likely invalid or redundant comment.
apps/skilavottord/web/screens/RecyclingFund/Overview/Overview.tsx (4)
24-25: LGTM: Component imports align with the new navigation structure.
The new imports for NavigationLinks and PageHeader components align with the PR's objective of improving the municipality management interface.
124-124: LGTM: UI changes improve consistency and maintainability.
The replacement of custom navigation with reusable NavigationLinks and PageHeader components improves the overall consistency of the application's UI.
Also applies to: 144-144
Line range hint 1-170: Add test coverage for municipality-related functionality.
The PR description indicates that tests haven't been added yet. Given the importance of municipality management features, please add tests covering:
- Permission checks for municipality roles
- Municipality data retrieval and display
- Error scenarios
Would you like me to help create a test suite for this component?
46-53: Verify the completeness of municipality data retrieval.
The GraphQL query has been expanded to include municipality-related fields, but there might be missing fields that could be useful for municipality management (e.g., contact information, address).
apps/skilavottord/ws/migrations/202412013210715-add-view-municipality.js (1)
5-10: SQL view creation looks correct.
The migration creates a new view to fetch municipality partners. Be sure to verify that any dependent code references the new view name properly.
apps/skilavottord/ws/src/app/modules/recyclingPartner/recyclingPartner.module.ts (1)
10-13: Correct inclusion of MunicipalityModel.
By adding MunicipalityModel to SequelizeModule.forFeature, you ensure that the recyclingPartner module is aware of municipality data. This aligns with NestJS module design.
apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts (3)
32-48: Robust error handling for municipality sub-partners.
Your try-catch block properly logs and throws an error upon failure. The approach is reasonable. Confirm that upstream callers handle the error gracefully.
27-28: Use array initialization sparingly.
You initialize an empty array here and conditionally set it to null later. Make sure your calling logic can handle both states (null vs. array).
✅ Verification successful
Let me verify how this array is being used in the codebase by checking the implementation.
Let me check the complete context of how this variable is used in the function.
Array initialization is correctly handled in this context
The code shows a clear and intentional pattern where partnerIds is initialized as an empty array and then either:
- Populated with municipality partner IDs if
filter.partnerIdexists - Explicitly set to
nulliffilter.partnerIddoesn't exist
This is a valid pattern as it provides a clear distinction between "no partners" (empty array) and "all partners" (null) states, and the code handles both cases appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# First, let's see the full context of this file
rg -A 10 "let partnerIds = \[\]" apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts
# Let's also check where this variable is being used
ast-grep --pattern 'partnerIds'
Length of output: 1401
Script:
#!/bin/bash
# Get more context around the usage of partnerIds in vehicle.service.ts
rg -B 20 -A 30 "let partnerIds = \[\]" apps/skilavottord/ws/src/app/modules/vehicle/vehicle.service.ts
Length of output: 1539
72-81: Verify model associations for performance.
You are including MunicipalityModel under a left join. This typically avoids an N+1 pattern, but confirm that repeated queries won't degrade performance at scale or if you join additional relationships in the future.
✅ Verification successful
Let me gather information about the model associations and query patterns to verify the performance concerns.
Let me search for more specific information about the model definitions and query patterns.
Based on the analysis of the codebase, I can now provide a final response:
Left join with MunicipalityModel is appropriately implemented and optimized
The implementation shows that:
- The MunicipalityModel is used sparingly with minimal attributes selected (
companyNameonly) - The left join is properly configured with
required: false - No evidence of N+1 query patterns or nested relationships that could cause performance issues
- The model is consistently used across the codebase with similar join patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for MunicipalityModel definition and its associations
ast-grep --pattern 'class MunicipalityModel extends $_'
# Search for other places where MunicipalityModel is included in queries
rg "MunicipalityModel.*include" -A 5
# Look for other complex joins or relationships with MunicipalityModel
rg "model: MunicipalityModel" -A 5
Length of output: 717
Script:
#!/bin/bash
# Find the MunicipalityModel definition file
fd "municipality.model.ts"
# Look for the model definition and its relationships
rg "MunicipalityModel" -A 10
# Search for any query implementations using this model
ast-grep --pattern 'findAll({
$$$
include: [{
model: MunicipalityModel,
$$$
}]
$$$
})'
Length of output: 50597
apps/skilavottord/web/screens/RecyclingFund/Overview/components/CarsTable/CarsTable.tsx (3)
1-4: LGTM! Imports are well-organized and properly typed.
The imports follow TypeScript best practices and are appropriately scoped for the component's needs.
Line range hint 11-29: LGTM! Component structure follows React best practices.
The component is well-structured with proper TypeScript typing and clean table header implementation.
42-45: Verify municipality data flow and error states.
The new municipality display aligns with the PR objectives, but we should ensure proper error handling for various data states.
Run the following script to check for proper typing of the municipality field in the GraphQL schema:
✅ Verification successful
Optional chaining is correctly used for handling nullable data
The code follows proper error handling practices by using optional chaining (?.) for both recyclingPartner?.municipality?.companyName and recyclingRequests?.map. The fallback to nameOfRequestor using the nullish coalescing operator (??) provides a sensible default when municipality data is not available. This implementation safely handles various data states and follows React/TypeScript best practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check GraphQL schema for municipality field definition
ast-grep --pattern 'type Municipality {
$$$
companyName: String
$$$
}'
# Check for proper typing in the Vehicle type
ast-grep --pattern 'type Vehicle {
$$$
recyclingRequests: [RecyclingRequest]
$$$
}'
Length of output: 170
Script:
#!/bin/bash
# Let's try to find GraphQL schema files and type definitions
fd -e graphql -e gql . --exec cat {}
# Also search for type definitions in TypeScript files
rg -t typescript "type\s+(Municipality|RecyclingPartner|Vehicle|RecyclingRequest)" -A 5
Length of output: 65759
Script:
#!/bin/bash
# Let's search for type definitions in TypeScript files using a different approach
fd -e ts -e tsx . --exec rg "type\s+(Municipality|RecyclingPartner|Vehicle|RecyclingRequest)" -A 5 {}
# Also search for the specific component's usage of municipality
rg -l "recyclingPartner\?\.municipality" .
Length of output: 2930
Script:
#!/bin/bash
# Let's look for the interface or type definition of RecyclingPartner and Municipality
rg "interface.*Municipality|type.*Municipality|interface.*RecyclingPartner|type.*RecyclingPartner" -A 5 .
# Also search for the specific file's imports to understand the types
rg -l "CarsTable" . | xargs rg "^import|^export"
Length of output: 28926
Script:
#!/bin/bash
# Let's check the Vehicle type definition in the GraphQL schema
rg "type\s+Vehicle\s*{" -A 10 .
# Also check the specific component's imports and type definitions
cat ./apps/skilavottord/web/screens/RecyclingFund/Overview/components/CarsTable/CarsTable.tsx
Length of output: 1976
apps/skilavottord/ws/migrations/202412013210715-add-view-municipality.js
Outdated
Show resolved
Hide resolved
apps/skilavottord/web/screens/RecyclingFund/Overview/components/CarsTable/CarsTable.tsx
Outdated
Show resolved
Hide resolved
- Fix after code rabbit review
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/skilavottord/web/screens/RecyclingFund/Overview/components/CarsTable/CarsTable.tsx (1)
30-50: Consider optimizing nested map operations.While the implementation has been improved with proper key uniqueness and null handling, the nested map operations could impact performance with large datasets.
Consider these optimizations:
+ const memoizedRequests = useMemo(() => vehicles.flatMap(({ vehicleId, vehicleType, newregDate, recyclingRequests }) => recyclingRequests?.map(({ createdAt, nameOfRequestor, recyclingPartner }, index) => ({ key: `${vehicleId}-${index}`, vehicleId, vehicleType, modelYear: getYear(newregDate), deregistrationDate: getDate(createdAt), nameOfRequestor, companyName: recyclingPartner?.municipality?.companyName ?? nameOfRequestor })) ?? [] ), + [vehicles] + ) return ( <Table> <Head>{/* ... */}</Head> <Body> - {vehicles.map( - ({ vehicleId, vehicleType, newregDate, recyclingRequests }) => { - return recyclingRequests?.map( - ({ createdAt, nameOfRequestor, recyclingPartner }, index) => { - const modelYear = getYear(newregDate) - const deregistrationDate = getDate(createdAt) + {memoizedRequests.map(({ + key, + vehicleId, + vehicleType, + modelYear, + deregistrationDate, + nameOfRequestor, + companyName + }) => ( return ( - <Row key={`${vehicleId}-${index}`}> + <Row key={key}> <Data> <Text variant="eyebrow">{vehicleId}</Text> </Data> <Data>{vehicleType}</Data> <Data>{modelYear ?? ''}</Data> <Data>{nameOfRequestor}</Data> <Data> - {recyclingPartner?.municipality?.companyName ?? - nameOfRequestor} + {companyName} </Data> <Data>{deregistrationDate ?? ''}</Data> </Row> ) - }, - ) - }, - )} + ))} </Body> </Table> )This optimization:
- Flattens the nested maps into a single operation
- Memoizes the data transformation
- Simplifies the JSX rendering logic
- Maintains all the null checks and fallbacks
apps/skilavottord/ws/migrations/202412230017109-add-view-municipality.js (1)
2-9: Consider using "CREATE OR REPLACE VIEW" for seamless updates and versioning.Currently, you are using "CREATE VIEW" to introduce the municipality_view, which will fail if the view already exists. If you expect to update or modify this view in future migrations or if the view might exist from earlier iterations, you could use "CREATE OR REPLACE VIEW" to avoid errors during deployment.
apps/skilavottord/ws/migrations/202412010017109-addcolumns-recycling-partners-municipality.js (2)
5-9: Column addition for is_municipality seems correct.The column name, type, and default value are suitably configured. Consider verifying if “is_municipality” follows your naming conventions for boolean fields throughout your codebase.
10-13: Consider foreign key constraints.Adding the “municipality_id” column as a string is fine, but if you have a separate municipality table, a foreign key constraint could help maintain referential integrity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/skilavottord/web/screens/RecyclingFund/Overview/components/CarsTable/CarsTable.tsx(2 hunks)apps/skilavottord/ws/migrations/202412010017109-addcolumns-recycling-partners-municipality.js(1 hunks)apps/skilavottord/ws/migrations/202412230017109-add-view-municipality.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/skilavottord/web/screens/RecyclingFund/Overview/components/CarsTable/CarsTable.tsx (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/migrations/202412010017109-addcolumns-recycling-partners-municipality.js (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/skilavottord/ws/migrations/202412230017109-add-view-municipality.js (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (7)
apps/skilavottord/web/screens/RecyclingFund/Overview/components/CarsTable/CarsTable.tsx (3)
1-4: LGTM! Clean and well-organized imports.
The imports follow NextJS best practices and include proper TypeScript types.
Line range hint 6-9: LGTM! Well-defined TypeScript interface.
The interface is clear, properly typed, and follows TypeScript best practices.
Line range hint 11-29: LGTM! Clean component structure.
The component follows React and TypeScript best practices, with proper typing and efficient table header implementation.
apps/skilavottord/ws/migrations/202412230017109-add-view-municipality.js (1)
11-15: Migration reversal logic looks good.
Using "DROP VIEW IF EXISTS" is a safe approach to avoid breaking migrations if the view has already been removed, or does not exist. This is a recommended way to gracefully handle reversals. No further action needed.
apps/skilavottord/ws/migrations/202412010017109-addcolumns-recycling-partners-municipality.js (3)
1-2: Looks good.
Importing DataTypes from sequelize is standard for defining column types in a migration.
3-4: Module export is coherent.
Exporting the up/down methods aligns with typical Sequelize migration patterns.
14-16: Index creation is sensible.
Indexing “is_municipality” may improve query performance. Verify usage patterns to ensure this index is beneficial.
✅ Verification successful
Let me search for usage patterns of the is_municipality column to verify if the index would be beneficial.
Let me search for any queries or service methods that might be filtering or searching by this column to better understand its usage patterns.
Based on the search results, I can now provide a final response about the index's usefulness:
Index on is_municipality is beneficial for query performance
The codebase shows frequent filtering on the is_municipality column in several critical scenarios:
- Used in a dedicated view
municipality_viewthat filters byis_municipality = true - Frequently used in service layer queries to filter recycling partners:
- To find all municipalities (
where: { isMunicipality: true }) - To find non-municipality partners (
where: { isMunicipality: false }) - In complex OR conditions (
[Op.or]: [{ isMunicipality: false }, { isMunicipality: null }])
- To find all municipalities (
- Used in UI components to separate municipality and non-municipality partners
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for SQL queries or ORM operations involving is_municipality
rg -i "is_municipality" --type sql --type js --type ts -A 3 -B 3
Length of output: 4498
Script:
#!/bin/bash
# Search for any service methods or queries using isMunicipality
rg -i "isMunicipality|where.*municipality" --type ts --type js -A 5 -B 5
Length of output: 57679
| down: async (queryInterface) => { | ||
| /* await queryInterface.removeIndex( | ||
| 'recycling_partner', | ||
| 'idx_recycling_partner_is_municipality', | ||
| ) | ||
| await queryInterface.removeColumn('recycling_partner', 'is_municipality') | ||
| await queryInterface.removeColumn('recycling_partner', 'municipality_id')*/ | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reinstate the “down” method for reliable rollbacks.
Re-enable the code that removes the index and columns to allow proper reversal of this migration if necessary.
There was a problem hiding this comment.
This is how I wan't it to be. I dont want to loose data if when migrate undo
There was a problem hiding this comment.
@birkirkristmunds Understood. We’ll leave the "down" method commented out so that the columns remain and no data is lost on rollback, as per your preference.
TS-936
What
Adding CRU for municipality so recycling companies can be put under a municipality
Add now municipality role
Handle the what municipality user with municipality role can do and see
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Enhancements
AccessControlcomponent to handle dynamic role checks and visibility.RecyclingCompaniescomponent to differentiate between municipalities and recycling companies.Bug Fixes
Documentation