158 link the contribution model to osbl creation #159
Conversation
- Implemented intersection observer to dynamically show/hide bottom submit button - Commented out unused alert and icon imports - Added state and ref management for button visibility - Improved form usability by providing a persistent submit action
…rmation - Added OsblDataTransformer service to handle file uploads for logo and documents - Created OsblCreation model to store OSBL contribution data - Updated Contribution model to support new contributable types - Implemented file processing using ActiveStorage for logo and document attachments - Added database migrations for OsblCreation and contribution status - Enhanced contributions controller to use new data transformation logic - Updated specs to cover new contribution creation workflow
…andling - Updated ContributionsController to handle nested contribution parameters - Modified frontend types to support nested OSBL creation data - Created a proxy method to handle nested form data and errors - Updated form components to work with new nested data structure - Improved error handling and validation for contribution form - Added new factories for supporting models and test data
WalkthroughThis pull request refactors and extends the contributions functionality. The controller now extracts OSBL data from a nested parameter structure and validates it before creating a Contribution. Frontend forms have been updated to use a nested data structure with improved validation and state management. On the backend, the Contribution model has been modified to support new contributable types and file attachments, and a new OsblCreation model is introduced. Several database migrations, schema updates, seed modifications, and additional tests (including factories and request specs) complete the integration, linking contributions to OSBL creation and ensuring proper file processing via a new transformation service. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as ContributionsController
participant Transformer as OsblDataTransformer
participant DB as Database
Client->>Controller: POST /create with nested contribution.osbl data
Controller->>Controller: Extract contribution_params & validate OSBL data
Controller->>Transformer: Transform OSBL parameters (logo & documents)
Transformer->>Transformer: Process logo and document attachments
Transformer-->>Controller: Return transformed OSBL data (signed IDs)
Controller->>DB: Create Contribution with transformed OSBL data
DB-->>Controller: Persist record (or return error)
Controller-->>Client: Redirect to contributions list or new contribution form with errors
Possibly related issues
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 RuboCop (1.69.1)spec/requests/contributions_spec.rb[!] There was an error parsing from /Gemfile:17-------------------------------------------# Windows does not include zoneinfo files, so bundle the tzinfo-data gem
-------------------------------------------/usr/lib/ruby/3.1.0/bundler/dsl.rb:357:in app/controllers/users/contributions_controller.rb[!] There was an error parsing from /Gemfile:17-------------------------------------------# Windows does not include zoneinfo files, so bundle the tzinfo-data gem
-------------------------------------------/usr/lib/ruby/3.1.0/bundler/dsl.rb:357:in app/models/osbl.rb[!] There was an error parsing from /Gemfile:17-------------------------------------------# Windows does not include zoneinfo files, so bundle the tzinfo-data gem
-------------------------------------------/usr/lib/ruby/3.1.0/bundler/dsl.rb:357:in
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
🪧 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: 8
🧹 Nitpick comments (13)
app/controllers/users/contributions_controller.rb (1)
52-114: Rename:typekeys to avoid confusion with Rails single-table inheritance.Your strong parameters include attributes named
:type(e.g., lines 78, 82). Rails interpretstypeas a special column name for STI, which can lead to subtle bugs. Consider renaming these attributes to something more descriptive likedocument_typeorlocation_typeto prevent conflicts.app/frontend/pages/Contribution/New.tsx (3)
2-5: Remove commented-out imports to keep the codebase clean.The lines commented out for
Alert,AlertDescription, andGoodIdeacan be safely removed if no longer needed, improving code readability and maintainability.
24-38: Double-check the error message fortax_reduction.While the schema uses an
enumfortax_reduction, your message references "Veuillez sélectionner un pourcentage," which might be confusing to users if the values are concepts like"intérêt_général"or"aide_aux_personnes_en_difficulté". Update the error message to clarify the required selection.
82-114: Provide a fallback for the Intersection Observer usage.Older browsers without an
IntersectionObserverAPI may need a polyfill or alternate logic. While this is often acceptable for modern browsers, you might want to confirm your user base’s requirements or add a small fallback for improved accessibility.spec/requests/contributions_spec.rb (1)
92-147: Avoid hardcoding factory IDs to prevent collisions in parallel tests.The test suite sets
create(:cause, id: 13), etc. In parallel or large test runs, ID collisions might occur if the database already uses those IDs. Prefer letting the factory auto-generate IDs or use asequenceto ensure uniqueness.spec/factories/osbl_creations.rb (1)
1-11: Enhance factory with sequences and traits for better test flexibility.Consider the following improvements:
- Use sequence for unique names to avoid conflicts.
- Add traits for different tax reduction types.
- Add traits for different causes.
Apply this diff to enhance the factory:
FactoryBot.define do factory :osbl_creation do + sequence(:name) { |n| "OSBL #{n}" } + osbl_data do { - name: "OSBL 1", + name: name, tax_reduction: "intérêt_général", osbls_causes_attributes: [{cause_id: 1}] } end + + trait :utilité_publique do + osbl_data do + { + name: name, + tax_reduction: "utilité_publique", + osbls_causes_attributes: [{cause_id: 1}] + } + end + end + + trait :multiple_causes do + osbl_data do + { + name: name, + tax_reduction: "intérêt_général", + osbls_causes_attributes: [ + {cause_id: 1}, + {cause_id: 2} + ] + } + end + end end endspec/factories/contributions.rb (1)
7-9: Remove redundant trait.The
:osbl_creationtrait is redundant as it sets the samecontributablevalue as the default factory definition.Remove the redundant trait or modify it to provide different behavior:
- trait :osbl_creation do - contributable { build(:osbl_creation) } - endIf you need traits, consider adding ones that provide different states or behaviors:
trait :with_utilité_publique do contributable { build(:osbl_creation, :utilité_publique) } end trait :with_multiple_causes do contributable { build(:osbl_creation, :multiple_causes) } enddb/migrate/20250205100116_add_status_to_contribution.rb (1)
3-4: Consider adding an index on the status column.Since the status column will likely be used for filtering and querying contributions, adding an index would improve query performance.
- add_column :contributions, :status, :integer, default: 0 + add_column :contributions, :status, :integer, default: 0 + add_index :contributions, :statusapp/models/contribution.rb (2)
14-14: Consider adding file validations.The
has_many_attached :filesassociation looks good, but consider adding validations for:
- File size limits
- Allowed file types
- Maximum number of attachments
has_many_attached :files + validates :files, + content_type: ['application/pdf', 'image/png', 'image/jpeg'], + size: { less_than: 10.megabytes }, + limit: { max: 5 }
16-17: Remove commented validation code.The validation is now enforced by the database constraint, so this commented code can be safely removed.
- # db_constraints enforcing : - # validates :body, presence: true, if: -> { %w[Feedback FeatureRequest BugReport CorrectionRequest Other].include?(contributable_type) }app/services/OsblDataTransformer.rb (1)
2-4: Add type checking for params.Consider adding type checking to ensure params is a valid object that responds to
to_h.def initialize(params) + raise ArgumentError, "params must respond to to_h" unless params.respond_to?(:to_h) @params = params.to_h endspec/models/contribution_spec.rb (1)
31-39: Expand test coverage for non-required body types.The commented code suggests
osbl_updateshould also be tested. Consider:
- Uncommenting and testing
osbl_update- Adding edge cases for body presence
- # %i[osbl_creation osbl_update].each do |type| - %i[osbl_creation].each do |type| + %i[osbl_creation osbl_update].each do |type| it "allows nil body for #{type}" do contribution = build(:contribution, contributable: build(type)) expect { contribution.save }.not_to raise_error end + + it "allows non-nil body for #{type}" do + contribution = build(:contribution, contributable: build(type, body: "Some content")) + expect { contribution.save }.not_to raise_error + end enddb/seeds.rb (1)
555-645: Consider removing commented-out code.The benefactorum_params hash is well-structured and aligns with the TypeScript interfaces. However, there are several blocks of commented-out code related to file handling that should be addressed.
Consider either:
- Removing the commented-out code if it's no longer needed
- Implementing the file handling if it's required
- Adding a TODO comment explaining why the code is commented out and when it will be implemented
- # "logo" => - # { - # "tempfile" => "#<File:0x00007836ee3e6f50>", - # "content_type" => "image/png", - # "original_filename" => "Benefactorum-Logo.png", - # "headers" => "Content-Disposition: form-data; name=\"logo\"; filename=\"Benefactorum-Logo.png\"\r\nContent-Type: image/png\r\n" - # }, + # TODO: Implement logo file handling in the next PRSimilarly for the document_attachments_attributes block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/controllers/users/contributions_controller.rb(2 hunks)app/frontend/pages/Contribution/New.tsx(5 hunks)app/frontend/pages/Contribution/types.ts(2 hunks)app/frontend/tests/forms/SignUpForm.test.tsx(1 hunks)app/frontend/tests/pages/auth/SignIn.test.tsx(1 hunks)app/frontend/tests/pages/auth/SignUp.test.tsx(1 hunks)app/models/contribution.rb(1 hunks)app/models/osbl_creation.rb(1 hunks)app/services/OsblDataTransformer.rb(1 hunks)db/migrate/20250205100116_add_status_to_contribution.rb(1 hunks)db/migrate/20250205100704_create_osbl_creations.rb(1 hunks)db/schema.rb(3 hunks)db/seeds.rb(2 hunks)spec/factories/contributions.rb(1 hunks)spec/factories/osbl_creations.rb(1 hunks)spec/models/contribution_spec.rb(1 hunks)spec/requests/contributions_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/frontend/tests/forms/SignUpForm.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy
🔇 Additional comments (22)
app/frontend/tests/pages/auth/SignUp.test.tsx (1)
22-25: LGTM! Verify the component's new location.The mock path update aligns with the component's new location in the directory structure.
Let's verify that the SignUpForm component exists at the new location:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the SignUpForm component exists at the new location. # Test: Search for the SignUpForm component file. fd SignUpForm.tsx -p app/frontend/components/pages/auth/signUp/Length of output: 122
LGTM! Verified the component's new location.
The SignUpForm component exists at the expected path:
app/frontend/components/pages/auth/signUp/SignUpForm.tsxapp/frontend/tests/pages/auth/SignIn.test.tsx (1)
22-25: LGTM! Verify the component's new location.The mock path update aligns with the component's new location in the directory structure.
Let's verify that the SignInForm component exists at the new location:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the SignInForm component exists at the new location. # Test: Search for the SignInForm component file. fd SignInForm.tsx -p app/frontend/components/pages/auth/signIn/Length of output: 122
LGTM! Verified that the SignInForm component exists at the new location as expected.
The file is correctly located atapp/frontend/components/pages/auth/signIn/SignInForm.tsx.app/controllers/users/contributions_controller.rb (1)
28-30: Handle missing or emptyosblparameters gracefully.If no
osbldata is provided or if it's empty,osbl_paramswould benilhere, resulting inOsbl.new(nil). Consider defaulting to{}or adding a guard clause to avoid potentialArgumentErroror undesired application behavior.app/frontend/pages/Contribution/New.tsx (1)
49-79: Confirm handling of unmapped errors increateOsblProxy.This function smartly reroutes nested errors but ignores those that don’t match the
/^contribution\.osbl\./pattern. If other error paths might be relevant, consider logging or surfacing them for debugging, ensuring no unexpected errors go unnoticed.spec/requests/contributions_spec.rb (3)
66-71: Theonly_for_current_usershared example is consistent with security checks.This partial ensures that only the owner can create a new contribution. The usage looks properly aligned with the rest of the codebase’s approach.
161-224: Comprehensive test coverage for nested attributes.These lines thoroughly validate the transformed file parameters,
osbldata, attachments, and nested relationships. Great job ensuring important associations (causes, keywords, documents, locations) are properly persisted and tested!
233-245: Redirect expectations are well tested.The error scenario confirms the system redirects to the new contribution form and surfaces the expected
osbls_causesattribute error. This approach ensures robust negative path coverage.db/migrate/20250205100116_add_status_to_contribution.rb (1)
6-9: Verify the check constraint logic.The check constraint correctly enforces body presence for specific contribution types. However, consider:
- Using an array-based check for better maintainability
- Adding a comment explaining the constraint's purpose
add_check_constraint :contributions, - "NOT (contributable_type IN ('Feedback', 'FeatureRequest', 'BugReport', 'CorrectionRequest', 'Other')) OR body IS NOT NULL", + "NOT (contributable_type = ANY(ARRAY['Feedback', 'FeatureRequest', 'BugReport', 'CorrectionRequest', 'Other'])) OR body IS NOT NULL", name: "body_required_for_specific_types" + + # This constraint ensures that contributions of specific types must have a body✅ Verification successful
Error: No shell scripts or web queries found
Review Confirmation: Check Constraint Logic and Suggested Enhancements
- The check constraint correctly forces a non‑NULL value in
bodywhencontributable_typeis one of the specified types.- Switching from the
INoperator to usingANY(ARRAY[...])is functionally equivalent and can improve maintainability, especially if the list needs to be updated later.- Adding an inline comment to explain the purpose of the constraint increases code clarity.
app/models/contribution.rb (1)
4-12: Verify the impact of removing Discussion type.The delegated_type changes look good, but ensure that:
- All existing Discussion contributions are migrated or handled
- Frontend code is updated to remove Discussion type references
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for Discussion type references in the codebase rg -i "discussion.*contribution"Length of output: 34
🏁 Script executed:
#!/bin/bash echo "Searching for all references to 'Discussion' (case insensitive) across the repository:" rg -i "\bdiscussion\b" echo echo "Searching for any references to 'Discussion' in the Contribution model (app/models/contribution.rb):" rg -i "Discussion" app/models/contribution.rb echo echo "Searching for Discussion references in migration files (db/migrate):" rg -i "discussion" db/migrate echo echo "Searching for Discussion references in frontend code (app/javascript and src/):" rg -i "discussion" app/javascript rg -i "discussion" srcLength of output: 1002
No residual Discussion dependencies detected.
After searching the entire codebase—including the Contribution model, migration directories, and potential frontend directories—there are no remaining references to the Discussion type. This indicates that the removal is safe in the code. Please ensure that any legacy Discussion contributions in the database are appropriately migrated or handled separately.app/frontend/pages/Contribution/types.ts (9)
1-7: LGTM!The interface correctly defines the contribution structure and aligns with the schema changes.
9-13: LGTM!The interface correctly defines the fund record structure.
15-23: LGTM!The interface correctly defines the annual finance structure and properly uses the
FundRecordinterface.
25-31: LGTM!The interface correctly defines the document structure.
33-41: LGTM!The interface correctly defines the address structure.
43-49: LGTM!The interface correctly defines the location structure and properly uses the
Addressinterface.
51-57: LGTM!The interface correctly defines the OSBL creation data structure and properly uses the
OsblDatainterface.
59-76: LGTM!The interface correctly defines the OSBL data structure, properly uses other interfaces for nested attributes, and aligns with the schema changes.
78-84: LGTM!The interface correctly defines the form props structure and properly uses the
OsblDatainterface.db/schema.rb (3)
13-13: LGTM!The schema version update is correct and follows the migration timestamp format.
93-97: LGTM!The changes to the contributions table are well-structured:
- Added
statuscolumn with a default value- Added
bodycolumn- Added check constraint to ensure
bodyis not null for specific contribution types
185-187: LGTM!The new osbl_creations table is well-structured with a non-nullable osbl_data column and aligns with the TypeScript interfaces.
db/seeds.rb (1)
63-67: LGTM!The new keywords are relevant to the application domain and follow the naming convention.
- Updated content type validation for SVG files from 'image/svg' to 'image/svg+xml' - Modified logo validation in Osbl and Label models to use consistent SVG MIME type - Adjusted frontend file input and validation to match new SVG content type - Simplified contributions controller error handling - Updated related specs to use new SVG MIME type
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
spec/factories/osbls.rb (1)
10-14: LGTM! Consider adding flexibility for multiple documents.The trait implementation is clean and follows factory_bot best practices. For enhanced flexibility, you might want to consider allowing the trait to accept a count parameter for creating multiple documents.
Here's how you could enhance the trait:
trait :with_document do + transient do + documents_count { 1 } + end + after(:build) do |osbl| - osbl.documents << build(:document) + documents_count.times { osbl.documents << build(:document) } end endUsage example:
create(:osbl, :with_document, documents_count: 3)spec/models/osbl_spec.rb (1)
59-72: Add more test cases for document validations.While the invalid document test is good, consider adding these test cases for better coverage:
- Valid document with file
- Multiple documents (both valid and invalid cases)
describe "associated documents" do it "osbl is valid with a valid document" do osbl = build(:osbl, document_attachments_attributes: [{ document_attributes: { type: "Autre", name: "Test document", file: fixture_file_upload('spec/fixtures/files/test.pdf', 'application/pdf') } }]) expect(osbl).to be_valid end it "handles multiple documents correctly" do osbl = build(:osbl, document_attachments_attributes: [ { document_attributes: { type: "Autre", name: "Valid document", file: fixture_file_upload('spec/fixtures/files/test.pdf', 'application/pdf') } }, { document_attributes: { type: "Autre", name: "Invalid document" # file missing } } ]) expect(osbl).not_to be_valid expect(osbl.errors[:"document_attachments.document.file"]).to be_present end endapp/frontend/pages/Contribution/New.tsx (2)
22-38: Consider using branded validation messages.The validation messages are currently in French but could be moved to a localization file for better maintainability.
Consider creating a separate messages file:
// messages.ts export const validationMessages = { url: 'Veuillez entrer une URL valide.', fileSize: 'La taille du fichier doit être inférieure à 1 MB.', fileType: 'Le type de fichier est invalide. Format accepté : SVG, PNG, WEBP.', // ... etc }
40-70: Strengthen type safety in createOsblProxy function.The function uses
anytype for the value parameter in setData, which could lead to type-related bugs.Consider this type-safe implementation:
function createOsblProxy<T extends OsblCreationData>( data: T, setData: <K extends keyof T['contribution']['osbl']>( key: K, value: T['contribution']['osbl'][K] ) => void, errors: Record<string, string>, clearErrors: (field: 'contribution') => void, setError: (field: 'contribution', message: string) => void ): FormProps { // ... rest of the implementation }spec/requests/contributions_spec.rb (1)
169-224: Consider breaking down the large test into smaller, focused examples.The test covers multiple aspects of contribution creation in a single example. This could make it harder to identify the source of failures.
Consider splitting into focused examples:
context "with valid parameters" do # ... setup ... it "creates a contribution with basic attributes" do expect { subject }.to change(Contribution, :count).by(1) contribution = Contribution.last expect(contribution.body).to eq("Je suis le créateur de Benefactorum...") expect(contribution.files).to be_attached end it "creates an osbl with correct attributes" do subject osbl = Osbl.create!(Contribution.last.osbl_creation.osbl_data) expect(osbl.name).to eq("Benefactorum") # ... other osbl assertions end it "creates associated records correctly" do subject osbl = Osbl.create!(Contribution.last.osbl_creation.osbl_data) expect(osbl.osbls_causes.count).to eq(1) expect(osbl.osbls_keywords.count).to eq(4) # ... other association assertions end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/controllers/users/contributions_controller.rb(2 hunks)app/frontend/components/pages/contribution/new/OsblHeader.tsx(1 hunks)app/frontend/pages/Contribution/New.tsx(4 hunks)app/models/label.rb(1 hunks)app/models/osbl.rb(1 hunks)app/models/osbl_creation.rb(1 hunks)app/services/OsblDataTransformer.rb(1 hunks)spec/factories/osbls.rb(1 hunks)spec/models/document_spec.rb(1 hunks)spec/models/osbl_spec.rb(1 hunks)spec/requests/contributions_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/osbl_creation.rb
- app/services/OsblDataTransformer.rb
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy
🔇 Additional comments (4)
app/models/label.rb (1)
2-13: LGTM! Consistent validation rules.The validation rules for the logo attachment are well-defined and consistent with the Osbl model:
- Proper MIME types including the correct
image/svg+xml- Reasonable 1MB file size limit
- Appropriate file formats for logos (SVG, PNG, WEBP)
app/models/osbl.rb (1)
57-57: LGTM! Correct MIME type for SVG files.The MIME type has been properly updated from
image/svgtoimage/svg+xml, which is the correct MIME type for SVG files.app/frontend/components/pages/contribution/new/OsblHeader.tsx (1)
42-42: LGTM! Frontend validation matches backend.The file input's accept attribute correctly matches the backend validation rules, ensuring consistent file type validation across the stack.
spec/models/document_spec.rb (1)
64-64: LGTM!The MIME type update from
image/svgtoimage/svg+xmlis correct and follows standard specifications.
Summary by CodeRabbit
OsblCreationclass for better management of OSBL-related contributions.OsblDataTransformerclass to manage logo and document uploads.