Fix create camp without template #7902
Conversation
The intermediate value 'none' sneaked into the post-payload. It's often a good idea to keep the intermediate form state and the actual payload separate. Then you can define an explicit mapping between the two representations.
|
@CodeRabbit review this PR |
🧩 Analysis chainI'll review this PR for you. Let me examine the changes to understand the fix for creating camps without a template. 🏁 Script executed: Length of output: 11323 @BacLuc I'll review this PR that fixes camp creation without a template. OverviewThis is a well-structured fix that addresses the core issue where the intermediate form value ✅ StrengthsFrontend ArchitectureThe separation of concerns in
watchEffect(() => {
if (['other', 'none', null].includes(selectedPrototypeValue.value)) {
localCamp.campPrototype = null
} else {
localCamp.campPrototype = selectedPrototypeValue.value
}
})Comprehensive E2E Testing
Consistent Data-testid ImplementationAll interactive elements now have proper test identifiers, improving testability across the application. 🔍 Minor ObservationsCode QualityThe implementation is clean and follows Vue 3 composition API best practices. The reactive state management is appropriate. Test StructureThe e2e test covers the happy path thoroughly. The date handling using Type SafetyThe ConclusionThis is a solid fix that not only resolves the immediate bug but also improves the overall architecture by properly separating form state from API payloads. The comprehensive e2e test ensures the fix works end-to-end and prevents regression. The changes are focused, well-implemented, and follow good practices. Great work! 🐰✨ ✅ Actions performedReview triggered.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughA new Cypress end-to-end test was added to automate camp creation without a prototype. Multiple frontend components were updated to include Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Cypress
participant App (Frontend)
participant Backend
User->>Cypress: Trigger test run
Cypress->>App (Frontend): Automate login as test user
App (Frontend)->>Backend: Authenticate user
Backend-->>App (Frontend): Return authentication response
Cypress->>App (Frontend): Navigate to camps page
Cypress->>App (Frontend): Click "create camp" button
Cypress->>App (Frontend): Fill camp details (title, organizer, motto, dates)
Cypress->>App (Frontend): Proceed to next step
Cypress->>App (Frontend): Select "Keine Vorlage" (no prototype)
Cypress->>App (Frontend): Submit camp creation
App (Frontend)->>Backend: Create camp API call
Backend-->>App (Frontend): Return camp creation result
Cypress->>App (Frontend): Verify camp info page and title field
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (8)
frontend/src/components/campAdmin/CampSettings.vue (1)
13-14: Lint error: put each attribute on its own line to satisfy ESLint rulesThe CI lint warning points to this exact line. Split the attributes to one-per-line (Vuetify style) to unblock the pipeline.
- <api-text-field path="title" vee-rules="required|max:32" :disabled="disabled" data-testid="title" /> + <api-text-field + path="title" + vee-rules="required|max:32" + :disabled="disabled" + data-testid="title" + />frontend/src/views/Camps.vue (1)
23-25: Same attribute-per-line rule applies to keep ESLint green- <button-add data-testid="create-camp-button" icon="mdi-plus" :to="{ name: 'camps/create' }"> + <button-add + data-testid="create-camp-button" + icon="mdi-plus" + :to="{ name: 'camps/create' }" + >frontend/src/components/campCreate/CampCreateStep1.vue (3)
12-14: Format attributes to pass lint & keep diff noise low- data-testid="create-camp-title-input" - required + data-testid="create-camp-title-input" + required(The essential part is breaking before
data-testid; adjust to project’s prettier / ESLint config.)
15-16: Two more lint violations – break attributes- <e-text-field v-model="localCamp.organizer" path="organizer" data-testid="create-camp-organizer" /> + <e-text-field + v-model="localCamp.organizer" + path="organizer" + data-testid="create-camp-organizer" + />Apply the same pattern to the
mottofield right below.
28-30: Attribute order: putdata-testidafter other props & on new line- <ButtonContinue v-if="valid" @click="$emit('next-step')" data-testid="create-camp-next-step" /> + <ButtonContinue + v-if="valid" + @click="$emit('next-step')" + data-testid="create-camp-next-step" + />frontend/src/components/campAdmin/CreateCampPeriods.vue (1)
53-56: Nit: keep attribute layout consistent- data-testid="start-date-picker" - required + data-testid="start-date-picker" + requiredDo the same for the end-date picker at Line 66.
e2e/specs/zz-createCamp.cy.js (1)
29-29: Fix formatting issue with extra space.There's an extra space before the selector that should be removed for consistent formatting.
- cy.get( '.v-select__selections > [data-testid="prototype-select"]').click() + cy.get('.v-select__selections > [data-testid="prototype-select"]').click()frontend/src/components/campCreate/CampCreateStep2.vue (1)
181-181: Fix attribute formatting for better readability.The ESLint formatter suggests breaking the attributes onto separate lines for better readability and consistency.
- <ButtonAdd v-if="valid" type="submit" :loading="isSaving" data-testid="create-camp-button"> + <ButtonAdd + v-if="valid" + type="submit" + :loading="isSaving" + data-testid="create-camp-button" + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/cypress.config.js(1 hunks)e2e/specs/zz-createCamp.cy.js(1 hunks)frontend/src/components/campAdmin/CampSettings.vue(1 hunks)frontend/src/components/campAdmin/CreateCampPeriods.vue(2 hunks)frontend/src/components/campCreate/CampCreateStep1.vue(2 hunks)frontend/src/components/campCreate/CampCreateStep2.vue(8 hunks)frontend/src/views/Camps.vue(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint: Frontend (ESLint)
frontend/src/components/campAdmin/CampSettings.vue
[failure] 13-13:
Replace ·path="title"·vee-rules="required|max:32"·:disabled="disabled"·data-testid="title" with ⏎··········path="title"⏎··········vee-rules="required|max:32"⏎··········:disabled="disabled"⏎··········data-testid="title"⏎·······
frontend/src/views/Camps.vue
[failure] 23-23:
Replace ·data-testid="create-camp-button"·icon="mdi-plus"·:to="{·name:·'camps/create'·}" with ⏎··············data-testid="create-camp-button"⏎··············icon="mdi-plus"⏎··············:to="{·name:·'camps/create'·}"⏎············
frontend/src/components/campCreate/CampCreateStep1.vue
[failure] 16-16:
Replace ·v-model="localCamp.motto"·path="motto"·data-testid="create-camp-motto" with ⏎··············v-model="localCamp.motto"⏎··············path="motto"⏎··············data-testid="create-camp-motto"⏎···········
[failure] 15-15:
Replace ·v-model="localCamp.organizer"·path="organizer"·data-testid="create-camp-organizer" with ⏎··············v-model="localCamp.organizer"⏎··············path="organizer"⏎··············data-testid="create-camp-organizer"⏎···········
[warning] 28-28:
Attribute "data-testid" should go before "@click"
[failure] 28-28:
Replace ·v-if="valid"·@click="$emit('next-step')"·data-testid="create-camp-next-step" with ⏎··············v-if="valid"⏎··············@click="$emit('next-step')"⏎··············data-testid="create-camp-next-step"⏎···········
frontend/src/components/campCreate/CampCreateStep2.vue
[failure] 181-181:
Replace ·v-if="valid"·type="submit"·:loading="isSaving"·data-testid="create-camp-button" with ⏎················v-if="valid"⏎················type="submit"⏎················:loading="isSaving"⏎················data-testid="create-camp-button"⏎··············
🪛 GitHub Check: Lint: e2e (ESLint)
e2e/specs/zz-createCamp.cy.js
[failure] 29-29:
Delete ·
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests: End-to-end run / Tests: End-to-end (firefox)
- GitHub Check: Tests: End-to-end run / Tests: End-to-end (edge)
- GitHub Check: Tests: End-to-end run / Tests: End-to-end (chrome)
🔇 Additional comments (15)
e2e/cypress.config.js (1)
14-14:experimentalStudiois deprecated – confirm Cypress version and roadmap
experimentalStudiowas removed in Cypress v13.0.0. If the project upgrades, this flag will be ignored and the Studio workflow will disappear.
- Pin the Cypress major version or migrate away from Studio-generated specs.
- Alternatively, gate this option behind a version check or environment variable.
e2e/specs/zz-createCamp.cy.js (4)
3-7: Good dynamic date handling for test robustness.Using dynamic date calculation ensures the test remains valid regardless of when it's executed, avoiding failures due to hardcoded past dates.
9-12: Well-structured test with clear naming.The test structure follows Cypress best practices with descriptive naming that clearly indicates the scenario being tested.
13-28: Comprehensive e2e test flow with reliable element selection.The test properly covers the complete user journey from login to form submission, using data-testid attributes for stable element targeting and appropriate Swiss date formatting.
30-35: Appropriate test assertions for verification.The test properly verifies both successful navigation to the camp info page and data persistence by checking the title field value.
frontend/src/components/campCreate/CampCreateStep2.vue (10)
217-217: Good addition of Vue 3 composition API import.Adding
watchEffectimport supports the new reactive state management pattern for separating form state from API payload.
241-248: Excellent architectural improvement with clean state separation.The introduction of
selectedPrototypeValueand thewatchEffectprovides a clean separation between the intermediate form state and the actual API payload. The logic correctly handles the edge cases where'other','none', andnullshould result in anullprototype in the payload.
10-10: Correct v-model update for separated state management.The v-model correctly binds to
selectedPrototypeValueinstead of directly to the API payload, supporting the clean state separation.
18-18: Good addition of data-testid for e2e testing.The
data-testid="prototype-select"attribute enables reliable element selection in the e2e test.
21-21: Consistent template update for separated state.The conditional correctly uses
selectedPrototypeValueinstead of the API payload property, maintaining consistency with the architectural changes.
260-260: Consistent state update in clipboard entity handler.The
onEntityLoadedhandler correctly updatesselectedPrototypeValueinstead of directly modifying the API payload, maintaining consistency with the new state management.
264-264: Proper error handling with consistent state management.The error handler correctly updates
selectedPrototypeValuewhile maintaining the existing logic for handling failed clipboard entity loads.
304-304: Consistent computed property updates for new state management.The computed properties correctly use
selectedPrototypeValueinstead of the API payload property, maintaining the same functionality while being consistent with the architectural changes.Also applies to: 312-312, 320-321
348-348: Correct watcher update for new reactive state.The watcher properly observes
selectedPrototypeValueinstead of the API payload property, maintaining the existing UI behavior for the "other" prototype selection.
278-278: Proper exposure of reactive state to template.Adding
selectedPrototypeValueto the return statement makes it available to the template, which is necessary for the v-model binding and computed properties.
Prefix it with zz because it creates a new camp. Use data-testid attribute to find the elements. The test was scaffolded with cypress studio.
BacLuc
force-pushed
the
fix-create-camp-without-template
branch
from
862d368 to
3809981
Compare
⛔ Feature branch deployment currently inactive.If the PR is still open, you can add the |
3 participants
frontend: fix creating camp without a prototype
The intermediate value 'none' sneaked into the post-payload.
It's often a good idea to keep the intermediate form state
and the actual payload separate.
Then you can define an explicit mapping between the two representations.
e2e: add test for creating a camp without a prototype
Prefix it with zz because it creates a new camp.
Use data-testid attribute to find the elements.
The test was scaffolded with cypress studio.