Fix socials in adminjs #1881
Fix socials in adminjs #1881
Conversation
CarlosQ96
force-pushed
the
hotfix_adminjs_editing_projects
branch
from
d487090 to
5728a2a
Compare
WalkthroughThe changes in this pull request introduce a new resource tab, Changes
Possibly related PRs
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/server/adminJs/tabs/projectSocialMediaTab.ts (1)
7-64: Configure pagination and sorting for better performance.The resource configuration lacks pagination and sorting settings, which could impact performance with large datasets.
Add these configurations to the options object:
options: { + sort: { + sortBy: 'createdAt', + direction: 'desc', + }, + listProperties: ['type', 'link', 'project'], + perPage: 20, // ... rest of the options },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/server/adminJs/adminJs.ts(2 hunks)src/server/adminJs/tabs/projectSocialMediaTab.ts(1 hunks)src/server/adminJs/tabs/projectsTab.ts(1 hunks)
🔇 Additional comments (2)
src/server/adminJs/adminJs.ts (1)
34-34: LGTM! Clean integration of the new social media tab.
The import and resource array addition are properly implemented.
Also applies to: 155-155
src/server/adminJs/tabs/projectsTab.ts (1)
801-811: Verify the relationship with ProjectSocialMedia entity.
The configuration looks correct, but let's verify the relationship is properly defined in the Project entity.
✅ Verification successful
The relationship between Project and ProjectSocialMedia entities is properly defined
The verification results show that:
- In
src/entities/project.ts, there's a proper@OneToManyrelationship defined from Project to ProjectSocialMedia - In
src/entities/projectSocialMedia.ts, there's a corresponding@ManyToOnerelationship defined from ProjectSocialMedia to Project - The AdminJS configuration in
projectsTab.tscorrectly references the 'ProjectSocialMedia' entity
The relationship is bi-directional and properly configured in both entities, matching the AdminJS configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the relationship is properly defined in the Project entity
# and if ProjectSocialMedia entity exists with correct fields
# Check Project entity for socialMedia relationship
ast-grep --pattern 'class Project {
$$$
@OneToMany(() => ProjectSocialMedia, $_ => $_)
socialMedia: ProjectSocialMedia[];
$$$
}'
# Check ProjectSocialMedia entity definition
ast-grep --pattern 'class ProjectSocialMedia {
$$$
@ManyToOne(() => Project, $_ => $_)
project: Project;
$$$
}'
Length of output: 260
Script:
#!/bin/bash
# Let's try a different approach to find these entities and their relationships
# Find files containing Project and ProjectSocialMedia
fd -e ts -e js | xargs rg "ProjectSocialMedia|@Entity.*Project"
# Look for any file containing OneToMany and ManyToOne relationships between these entities
rg -A 3 -B 3 "@(OneToMany|ManyToOne).*(Project|ProjectSocialMedia)"
Length of output: 15901
| type: { | ||
| isVisible: true, | ||
| }, | ||
| link: { | ||
| isVisible: true, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type field with predefined social media platforms.
The type field could benefit from a predefined list of social media platforms to ensure consistency.
Consider adding available values:
type: {
isVisible: true,
+ availableValues: [
+ { value: 'twitter', label: 'Twitter' },
+ { value: 'facebook', label: 'Facebook' },
+ { value: 'instagram', label: 'Instagram' },
+ { value: 'linkedin', label: 'LinkedIn' },
+ { value: 'github', label: 'GitHub' },
+ ],
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type: { | |
| isVisible: true, | |
| }, | |
| link: { | |
| isVisible: true, | |
| }, | |
| type: { | |
| isVisible: true, | |
| availableValues: [ | |
| { value: 'twitter', label: 'Twitter' }, | |
| { value: 'facebook', label: 'Facebook' }, | |
| { value: 'instagram', label: 'Instagram' }, | |
| { value: 'linkedin', label: 'LinkedIn' }, | |
| { value: 'github', label: 'GitHub' }, | |
| ], | |
| }, | |
| link: { | |
| isVisible: true, | |
| }, |
| list: { | ||
| isVisible: true, | ||
| isAccessible: ({ currentAdmin }) => | ||
| canAccessMainCategoryAction({ currentAdmin }, ResourceActions.LIST), | ||
| }, | ||
| show: { | ||
| isVisible: true, | ||
| isAccessible: ({ currentAdmin }) => | ||
| canAccessMainCategoryAction({ currentAdmin }, ResourceActions.SHOW), | ||
| }, | ||
| delete: { | ||
| isVisible: false, | ||
| isAccessible: ({ currentAdmin }) => | ||
| canAccessMainCategoryAction({ currentAdmin }, ResourceActions.DELETE), | ||
| }, | ||
| new: { | ||
| isVisible: false, | ||
| isAccessible: ({ currentAdmin }) => | ||
| canAccessMainCategoryAction({ currentAdmin }, ResourceActions.NEW), | ||
| }, | ||
| edit: { | ||
| isVisible: false, | ||
| isAccessible: ({ currentAdmin }) => | ||
| canAccessMainCategoryAction({ currentAdmin }, ResourceActions.EDIT), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Add input validation for social media links.
The actions configuration lacks input validation for social media links, which could lead to security issues or invalid data being stored.
Consider adding validation in the edit action:
edit: {
isVisible: false,
isAccessible: ({ currentAdmin }) =>
canAccessMainCategoryAction({ currentAdmin }, ResourceActions.EDIT),
+ before: async (request) => {
+ const { link } = request.payload;
+ if (link && !isValidUrl(link)) {
+ throw new Error('Invalid social media URL format');
+ }
+ return request;
+ },
},Committable suggestion skipped: line range outside the PR's diff.
CarlosQ96
force-pushed
the
hotfix_adminjs_editing_projects
branch
from
5728a2a to
ae6c910
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/server/adminJs/tabs/projectsTab.ts (1)
801-811: LGTM: Improved social media data structureThe change from
socialstosocialMediaproperty with a proper entity reference improves the data structure and management of social media information. The configuration looks correct with appropriate visibility settings.This change suggests better data modeling by:
- Using a dedicated entity for social media data
- Supporting multiple social media entries through
isArray: true- Maintaining proper data relationships through entity references
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/server/adminJs/adminJs.ts(2 hunks)src/server/adminJs/tabs/projectSocialMediaTab.ts(1 hunks)src/server/adminJs/tabs/projectsTab.ts(1 hunks)src/services/chains/index.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/adminJs/adminJs.ts
- src/server/adminJs/tabs/projectSocialMediaTab.ts
🔇 Additional comments (1)
src/services/chains/index.test.ts (1)
1001-1018:
Verify if RAY token test should remain commented out
The test case for RAY token transfer on Solana mainnet has been commented out. This could reduce test coverage for Solana mainnet transactions.
Consider either:
- Re-enabling the test if RAY token transfers still need to be supported
- Adding a comment explaining why the test was disabled
- Adding alternative test cases to maintain coverage for Solana mainnet token transfers
Related to: https://github.com/orgs/Giveth/projects/9/views/1?sliceBy%5Bvalue%5D=CarlosQ96&pane=issue&itemId=80410999&issue=Giveth%7Cgiveth-dapps-v2%7C4759
Summary by CodeRabbit
New Features
ProjectSocialMediatab in the AdminJS interface for managing social media resources.socialMediaproperty to project management, enhancing how social media information is displayed.Bug Fixes
socialsproperty to streamline project data management.Documentation