feat(settings): add global email template configuration #120
Conversation
📝 WalkthroughWalkthroughThis change implements a proposal management system with email template fallbacks at both global and event levels, adds talk/speaker proposal acceptance flow with post-acceptance editing capabilities, and introduces a new dashboard interface for viewing and managing proposals with edit functionality. Changes
Sequence DiagramssequenceDiagram
participant User
participant Service as Sponsorship/Ticket Service
participant EventConfig as Event Config
participant GlobalConfig as Buzz Settings
participant EmailTemplate as Email Template
participant MailSender
User->>Service: Trigger send email
Service->>EventConfig: Check event-level template
alt Event Template Set
EventConfig-->>Service: template_name
else Event Template Not Set
Service->>GlobalConfig: Fetch default template
GlobalConfig-->>Service: template_name
end
alt No Template Found
Service-->>User: Log error, return
else Template Found
Service->>EmailTemplate: Fetch template content
EmailTemplate-->>Service: subject, content
Service->>Service: Gather CC, Reply-To<br/>(event > global)
Service->>MailSender: Send with params
MailSender-->>User: Email delivered
end
sequenceDiagram
participant User
participant ProposalList as ProposalsList Page
participant ProposalDetails as ProposalDetails Page
participant EditDialog as ProposalEditDialog
participant Backend as Frappe Backend
participant EventAPI as Event API
User->>ProposalList: View proposals list
ProposalList->>Backend: Fetch Talk Proposals<br/>(submitted_by=current user)
Backend-->>ProposalList: Proposal records
ProposalList-->>User: Display list
User->>ProposalList: Click proposal
ProposalList->>ProposalDetails: Navigate with proposalId
ProposalDetails->>Backend: Fetch proposal details
ProposalDetails->>EventAPI: Fetch event data<br/>(allow_editing_talks_after_acceptance)
Backend-->>ProposalDetails: Proposal data
EventAPI-->>ProposalDetails: Event config
alt Status = Accepted
ProposalDetails->>Backend: Fetch Event Talk
Backend-->>ProposalDetails: Event Talk record
ProposalDetails-->>User: Show edit button
User->>EditDialog: Click Edit
EditDialog-->>EditDialog: Load Event Talk data
else Other Status
ProposalDetails-->>User: Show details only
end
User->>EditDialog: Update title/description
EditDialog->>Backend: Call frappe.client.set_value
Backend-->>EditDialog: Success
EditDialog-->>ProposalDetails: Emit updated
ProposalDetails->>ProposalDetails: Reload data
ProposalDetails-->>User: Display updated content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (1)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting 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. Comment |
![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 (3)
buzz/ticketing/doctype/event_ticket/event_ticket.py (1)
83-87: Consider usingget_cached_docfor consistency with other settings access patterns.The fallback logic is correct. However,
sponsorship_enquiry.pyusesfrappe.get_cached_doc("Buzz Settings")to access global settings, while this code usesfrappe.db.get_single_value. Usingget_cached_docwould be more consistent and beneficial if you need to access multiple settings fields in the future.♻️ Optional refactor for consistency
# Fallback to global setting if event-level not set if not ticket_template: - ticket_template = frappe.db.get_single_value("Buzz Settings", "default_ticket_email_template") + settings = frappe.get_cached_doc("Buzz Settings") + ticket_template = settings.default_ticket_email_templatebuzz/proposals/doctype/sponsorship_enquiry/test_sponsorship_enquiry.py (1)
69-71: Consider using_for unused lambda parameter to clarify intent.The
nameparameter is required to matchget_cached_doc's signature but isn't used. Using_as the parameter name is a Python convention that signals the value is intentionally ignored and silences the linter warning.♻️ Suggested fix (apply to all similar lambdas)
- mock_get_cached_doc.side_effect = lambda doctype, name=None: ( + mock_get_cached_doc.side_effect = lambda doctype, _=None: ( mock_event if doctype == "Buzz Event" else mock_settings )buzz/events/doctype/buzz_settings/buzz_settings.json (1)
130-135: Consider adding email validation for CC field.The CC field uses
Small Textwhich allows free-form input. If users enter invalid email addresses, they'll only discover the issue when emails fail to send. Consider adding a description hint about expected format (comma-separated emails) or validation via a custom script.♻️ Optional: Add description for CC format
{ "depends_on": "eval:doc.auto_send_pitch_deck", "fieldname": "default_sponsor_deck_cc", "fieldtype": "Small Text", - "label": "Default CC" + "label": "Default CC", + "description": "Comma-separated email addresses" },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
buzz/events/doctype/buzz_settings/buzz_settings.jsonbuzz/proposals/doctype/sponsorship_enquiry/sponsorship_enquiry.pybuzz/proposals/doctype/sponsorship_enquiry/test_sponsorship_enquiry.pybuzz/ticketing/doctype/event_ticket/event_ticket.pybuzz/ticketing/doctype/event_ticket/test_event_ticket.py
🧰 Additional context used
🧬 Code graph analysis (1)
buzz/ticketing/doctype/event_ticket/test_event_ticket.py (1)
buzz/ticketing/doctype/event_ticket/event_ticket.py (1)
send_ticket_email(79-120)
🪛 Ruff (0.14.10)
buzz/proposals/doctype/sponsorship_enquiry/test_sponsorship_enquiry.py
69-69: Unused lambda argument: name
(ARG005)
93-93: Unused lambda argument: name
(ARG005)
129-129: Unused lambda argument: name
(ARG005)
172-172: Unused lambda argument: name
(ARG005)
209-209: Unused lambda argument: name
(ARG005)
236-236: Unused lambda argument: name
(ARG005)
268-268: Unused lambda argument: name
(ARG005)
⏰ 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). (1)
- GitHub Check: Server
🔇 Additional comments (5)
buzz/proposals/doctype/sponsorship_enquiry/sponsorship_enquiry.py (2)
84-86: Verify: Globalauto_send_pitch_deckoverrides event-level disable.The current logic sends emails if either event or global
auto_send_pitch_deckis enabled. This means a global enable will send pitch decks for all events, even those withauto_send_pitch_deck = False. If the intent is for event-level settings to take precedence (i.e., an explicit event disable should block sending), the logic would need adjustment.If the current OR behavior is intentional (global acts as a fallback enabler), this is fine as-is. Please confirm this matches the expected product behavior.
88-112: LGTM on template and CC/Reply-To fallback logic.The precedence logic correctly uses event-level values when set, with graceful fallback to global settings. The error logging when no template is configured is good defensive coding.
buzz/ticketing/doctype/event_ticket/test_event_ticket.py (1)
25-173: LGTM! Comprehensive test coverage for template fallback logic.The tests effectively cover the key scenarios:
- Event-level template usage when configured
- Fallback to global template when event-level is absent
- Event-level precedence over global
- Inline default template when neither is configured
The assertions properly verify both positive behavior (correct values used) and negative behavior (fallback not called when not needed).
buzz/proposals/doctype/sponsorship_enquiry/test_sponsorship_enquiry.py (1)
25-285: LGTM! Thorough test coverage for sponsorship email fallback logic.Excellent coverage of edge cases including:
- Both toggles disabled
- Event-level settings usage
- Global fallback
- Precedence verification
- Global auto_send enabling email when event disabled
- Error logging when no template
- Partial fallback with mixed settings
The partial fallback test (lines 247-285) is particularly valuable for validating that individual fields fall back independently.
buzz/events/doctype/buzz_settings/buzz_settings.json (1)
76-135: LGTM! Well-structured settings schema with appropriate conditional visibility.The field definitions properly implement:
- Conditional visibility via
depends_onfor sponsor settings- Mandatory enforcement via
mandatory_depends_onwhen auto-send is enabled- Email validation on
default_sponsor_deck_reply_tovia"options": "Email"- Clear descriptions explaining the override behavior
Add Communications tab to Buzz Settings with global defaults for: - Ticket email template - Sponsorship pitch deck email template, CC, and reply-to Event-level settings take precedence over global settings. When event-level is not configured, falls back to global defaults. Includes unit tests for the fallback logic. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
NagariaHussain
force-pushed
the
feat/global-email-template-settings
branch
from
25cdbb8 to
86d4916
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
buzz/proposals/doctype/sponsorship_enquiry/test_sponsorship_enquiry.py (2)
16-31: Add defensive check in tearDownClass in case setUpClass fails.If
setUpClassfails (e.g., no event with route "test-route" exists),tearDownClasswill error when accessingcls.test_event. Consider adding a guard.♻️ Suggested improvement
@classmethod def tearDownClass(cls): + if hasattr(cls, 'test_event'): - cls.test_event.auto_send_pitch_deck = False - cls.test_event.sponsor_deck_email_template = None - cls.test_event.sponsor_deck_cc = None - cls.test_event.sponsor_deck_reply_to = None - cls.test_event.save() + cls.test_event.auto_send_pitch_deck = False + cls.test_event.sponsor_deck_email_template = None + cls.test_event.sponsor_deck_cc = None + cls.test_event.sponsor_deck_reply_to = None + cls.test_event.save() settings = frappe.get_doc("Buzz Settings")
75-83: Consider adding a test for partial enable scenario.The current
test_no_email_when_disabledonly tests when both event and global are disabled. Based on the implementation insponsorship_enquiry.py(line 84:if not event.auto_send_pitch_deck and not settings.auto_send_pitch_deck), email is sent if either is enabled.Consider adding a test to verify that enabling only the global setting triggers the email even when the event-level is disabled.
buzz/ticketing/doctype/event_ticket/test_event_ticket.py (2)
13-26: Missing tearDownClass to reset shared state after all tests.Unlike
TestSponsorshipEnquiryEmail, this class lacks atearDownClassmethod to restoretest_event.ticket_email_templateand global settings after tests complete. This could leave the test event in a modified state affecting other test classes.♻️ Add tearDownClass for consistency
@classmethod def tearDownClass(cls): if hasattr(cls, 'test_event'): cls.test_event.ticket_email_template = None cls.test_event.save() settings = frappe.get_doc("Buzz Settings") settings.default_ticket_email_template = None settings.save() super().tearDownClass()
125-137: Inconsistent cleanup pattern: consider adding try/finally for consistency.Other tests use
try/finallyblocks to ensure cleanup even if assertions fail. While this test sets values toNone(which is the desired state), wrapping intry/finallywould maintain consistency with other tests in this class.♻️ Optional: Add try/finally for consistency
@patch("frappe.sendmail") def test_uses_inline_template_when_none_configured(self, mock_sendmail): - self.test_event.ticket_email_template = None - self.test_event.save() - - settings = frappe.get_doc("Buzz Settings") - settings.default_ticket_email_template = None - settings.save() - - self.test_ticket.send_ticket_email(now=True) - - mock_sendmail.assert_called_once() - self.assertEqual(mock_sendmail.call_args[1]["template"], "ticket") + try: + self.test_event.ticket_email_template = None + self.test_event.save() + + settings = frappe.get_doc("Buzz Settings") + settings.default_ticket_email_template = None + settings.save() + + self.test_ticket.send_ticket_email(now=True) + + mock_sendmail.assert_called_once() + self.assertEqual(mock_sendmail.call_args[1]["template"], "ticket") + finally: + # Values already set to None, but follows pattern of other tests + pass
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
buzz/events/doctype/buzz_settings/buzz_settings.jsonbuzz/proposals/doctype/sponsorship_enquiry/sponsorship_enquiry.pybuzz/proposals/doctype/sponsorship_enquiry/test_sponsorship_enquiry.pybuzz/ticketing/doctype/event_ticket/event_ticket.pybuzz/ticketing/doctype/event_ticket/test_event_ticket.py
🚧 Files skipped from review as they are similar to previous changes (2)
- buzz/proposals/doctype/sponsorship_enquiry/sponsorship_enquiry.py
- buzz/ticketing/doctype/event_ticket/event_ticket.py
🧰 Additional context used
🧬 Code graph analysis (2)
buzz/proposals/doctype/sponsorship_enquiry/test_sponsorship_enquiry.py (2)
buzz/ticketing/doctype/event_ticket/test_event_ticket.py (1)
_create_template(52-62)buzz/proposals/doctype/sponsorship_enquiry/sponsorship_enquiry.py (1)
send_pitch_deck(80-113)
buzz/ticketing/doctype/event_ticket/test_event_ticket.py (1)
buzz/ticketing/doctype/event_ticket/event_ticket.py (1)
send_ticket_email(79-120)
⏰ 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). (1)
- GitHub Check: Server
🔇 Additional comments (4)
buzz/events/doctype/buzz_settings/buzz_settings.json (1)
76-135: LGTM! Well-structured field definitions with appropriate conditional visibility.The new Communications tab fields are correctly configured:
depends_onandmandatory_depends_onconditions properly gate the sponsorship email settings behindauto_send_pitch_deck- CC is correctly optional while template and reply_to are mandatory when auto-send is enabled
- Field types (Link, Data with Email option, Small Text) are appropriate for their purposes
buzz/proposals/doctype/sponsorship_enquiry/test_sponsorship_enquiry.py (1)
43-53: LGTM! Helper method correctly creates templates with appropriate subject syntax.The template subject uses
event.titlewhich correctly matches the context passed insponsorship_enquiry.pyline 93:{"doc": self, "event": event}.buzz/ticketing/doctype/event_ticket/test_event_ticket.py (2)
52-62: LGTM! Helper correctly usesevent_titlematching the ticket email context.The template subject uses
event_titlewhich correctly matches the context passed inevent_ticket.pyline 91:"event_title": event_title.
64-123: Good test coverage for the email template fallback logic.The tests comprehensively cover:
- Event-level template usage
- Fallback to global settings
- Event settings taking precedence over global
- Proper cleanup in
finallyblocks
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @buzz/events/doctype/buzz_settings/buzz_settings.json:
- Around line 118-127: The field "default_sponsor_deck_reply_to" currently has
both an unconditional "reqd": 1 and a conditional "mandatory_depends_on":
"eval:doc.auto_send_pitch_deck", which conflicts; remove the unconditional
"reqd": 1 from the buzz_settings JSON so the field is only required when the
conditional mandatory_depends_on evaluates true (keep "mandatory_depends_on" and
"depends_on" as-is).
🧹 Nitpick comments (1)
buzz/events/doctype/buzz_settings/buzz_settings.py (1)
21-25: Type annotation mismatch for conditionally-required field.
default_sponsor_deck_reply_tois typed asDF.Data(non-nullable), but inbuzz_settings.jsonit usesmandatory_depends_on: "eval:doc.auto_send_pitch_deck", meaning the field is only required when auto-send is enabled. When disabled, the field can be empty/None.Consider updating the type to
DF.Data | Nonefor consistency with the conditional requirement, though this may be auto-generated and regenerate on the next schema sync.Suggested type annotation
- default_sponsor_deck_reply_to: DF.Data + default_sponsor_deck_reply_to: DF.Data | None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
buzz/events/doctype/buzz_event/buzz_event.jsonbuzz/events/doctype/buzz_event/buzz_event.pybuzz/events/doctype/buzz_settings/buzz_settings.jsonbuzz/events/doctype/buzz_settings/buzz_settings.py
⏰ 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). (1)
- GitHub Check: Server
🔇 Additional comments (5)
buzz/events/doctype/buzz_event/buzz_event.json (2)
258-262: Verify intentional default value difference between event and global settings.Event-level
auto_send_pitch_deckdefaults to"1"(enabled), while inbuzz_settings.json(Line 99) the globalauto_send_pitch_deckdefaults to"0"(disabled). This means new events will auto-send pitch decks by default even if global auto-send is off.If this is intentional (event-level opt-out rather than opt-in), the behavior is fine. Otherwise, consider aligning defaults for consistency.
276-282: LGTM!The added description clearly communicates the fallback behavior to users, which aligns well with the global template configuration feature.
buzz/events/doctype/buzz_event/buzz_event.py (1)
26-27: LGTM!Auto-generated type declarations for the tax-related fields and
meta_imageare correctly added within theTYPE_CHECKINGblock, providing better IDE support without runtime overhead.Also applies to: 39-39, 53-55
buzz/events/doctype/buzz_settings/buzz_settings.json (2)
76-92: LGTM!The Communications tab structure is well-organized with clear descriptions explaining the fallback behavior ("Can be overridden per event"). The
default_ticket_email_templateLink field is properly configured.
93-117: LGTM!The sponsorship email settings section uses appropriate
depends_onconditions for conditional visibility andmandatory_depends_onfor conditional requirements. The structure cleanly separates the auto-send toggle from its dependent settings.
| { | ||
| "depends_on": "eval:doc.auto_send_pitch_deck", | ||
| "fieldname": "default_sponsor_deck_reply_to", | ||
| "fieldtype": "Data", | ||
| "in_list_view": 1, | ||
| "label": "Default Reply To", | ||
| "mandatory_depends_on": "eval:doc.auto_send_pitch_deck", | ||
| "options": "Email", | ||
| "reqd": 1 | ||
| }, |
There was a problem hiding this comment.
Conflicting reqd and mandatory_depends_on on the same field.
The default_sponsor_deck_reply_to field has both "reqd": 1 (Line 126) and "mandatory_depends_on": "eval:doc.auto_send_pitch_deck" (Line 124). The reqd: 1 makes the field unconditionally required, which conflicts with the conditional mandatory_depends_on and the depends_on visibility condition.
If the intent is to require this field only when auto_send_pitch_deck is enabled, remove "reqd": 1:
Suggested fix
{
"depends_on": "eval:doc.auto_send_pitch_deck",
"fieldname": "default_sponsor_deck_reply_to",
"fieldtype": "Data",
"in_list_view": 1,
"label": "Default Reply To",
"mandatory_depends_on": "eval:doc.auto_send_pitch_deck",
- "options": "Email",
- "reqd": 1
+ "options": "Email"
},📝 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.
| { | |
| "depends_on": "eval:doc.auto_send_pitch_deck", | |
| "fieldname": "default_sponsor_deck_reply_to", | |
| "fieldtype": "Data", | |
| "in_list_view": 1, | |
| "label": "Default Reply To", | |
| "mandatory_depends_on": "eval:doc.auto_send_pitch_deck", | |
| "options": "Email", | |
| "reqd": 1 | |
| }, | |
| { | |
| "depends_on": "eval:doc.auto_send_pitch_deck", | |
| "fieldname": "default_sponsor_deck_reply_to", | |
| "fieldtype": "Data", | |
| "in_list_view": 1, | |
| "label": "Default Reply To", | |
| "mandatory_depends_on": "eval:doc.auto_send_pitch_deck", | |
| "options": "Email" | |
| }, |
🤖 Prompt for AI Agents
In @buzz/events/doctype/buzz_settings/buzz_settings.json around lines 118 - 127,
The field "default_sponsor_deck_reply_to" currently has both an unconditional
"reqd": 1 and a conditional "mandatory_depends_on":
"eval:doc.auto_send_pitch_deck", which conflicts; remove the unconditional
"reqd": 1 from the buzz_settings JSON so the field is only required when the
conditional mandatory_depends_on evaluates true (keep "mandatory_depends_on" and
"depends_on" as-is).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dashboard/src/pages/Account.vue (1)
3-9: Clicking tabs won't navigate—missing route navigation logic.The
Tabscomponent is bound totabIndexviav-model, but clicking a tab no longer triggers navigation sinceuseRouterandrouter.pushwere removed. Therouteproperty on each tab appears to be for your own reference but the frappe-uiTabscomponent doesn't auto-navigate based on it.You need to add an
@changehandler to navigate when the user clicks a different tab:Proposed fix
<script setup> import { Tabs } from "frappe-ui"; import { ref, watch } from "vue"; -import { useRoute } from "vue-router"; +import { useRoute, useRouter } from "vue-router"; import ProfileView from "@/components/ProfileView.vue"; import LucideCalendarDays from "~icons/lucide/calendar-days"; import LucideTicket from "~icons/lucide/ticket"; import LucideMegaphone from "~icons/lucide/megaphone"; import LucideCircleDollarSign from "~icons/lucide/circle-dollar-sign"; const route = useRoute(); +const router = useRouter();<template> <ProfileView /> - <Tabs as="div" v-model="tabIndex" :tabs="tabs"> + <Tabs as="div" v-model="tabIndex" :tabs="tabs" `@update`:modelValue="handleTabChange"> <template `#tab-panel`> <div class="py-5"> <router-view></router-view> </div> </template> </Tabs> </template>Add the handler after the watcher:
const handleTabChange = (newIndex) => { const targetRoute = tabs[newIndex]?.route; if (targetRoute && route.path !== targetRoute) { router.push(targetRoute); } };Also applies to: 24-46
🤖 Fix all issues with AI agents
In `@dashboard/src/pages/ProposalDetails.vue`:
- Around line 321-326: onProposalUpdated currently calls proposal.reload() but
immediately checks proposal.doc?.status, causing a race where the old status may
be read; change onProposalUpdated to await the async reload (or attach a .then
callback) before checking proposal.doc?.status and calling
eventTalkResource.fetch(), or remove this logic and rely on the existing watcher
that already triggers fetch when status becomes "Accepted" to avoid duplicate
checks; update the onProposalUpdated function accordingly (reference:
onProposalUpdated, proposal.reload, eventTalkResource.fetch).
🧹 Nitpick comments (3)
dashboard/src/pages/ProposalsList.vue (2)
37-44: Unreachable code—this block will never render.When
proposals.datais an empty array ([]), it's still truthy, so thev-if="proposals.data"on Line 4 evaluates totrueand theListViewrenders (using itsemptyStateoption). Thisv-else-ifbranch is dead code.You can safely remove this block since
ListView'semptyStatealready handles the empty case.Proposed fix
<div v-else-if="proposals.loading" class="w-4"> <Spinner /> </div> - - <div v-else-if="proposals.data && proposals.data.length === 0" class="text-center py-8"> - <div class="text-ink-gray-5 text-lg mb-2"> - {{ __("No proposals yet") }} - </div> - <div class="text-ink-gray-4 text-sm"> - {{ __("Your talk proposals will appear here") }} - </div> - </div> </div>
33-35: Consider widening the spinner container.The
class="w-4"(1rem/16px) appears quite narrow for a loading spinner. Consider using a wider container or centering it for better visibility.Suggested improvement
- <div v-else-if="proposals.loading" class="w-4"> + <div v-else-if="proposals.loading" class="flex justify-center py-8"> <Spinner /> </div>dashboard/src/pages/ProposalDetails.vue (1)
169-172: Consider sanitizing the error message displayed to users.Displaying
proposal.get.errordirectly may expose internal error details. Consider providing a generic message or sanitizing the error content.🔧 Suggested improvement
<div v-else-if="proposal.get.error" class="text-center py-8"> <div class="text-ink-red-3 text-lg mb-2">{{ __("Error loading proposal details") }}</div> - <div class="text-ink-gray-4 text-sm">{{ proposal.get.error }}</div> + <div class="text-ink-gray-4 text-sm">{{ __("Please try again or contact support if the issue persists.") }}</div> </div>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dashboard/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
buzz/events/doctype/buzz_event/buzz_event.jsonbuzz/events/doctype/event_talk/event_talk.jsonbuzz/proposals/doctype/talk_proposal/talk_proposal.jsbuzz/proposals/doctype/talk_proposal/talk_proposal.jsonbuzz/proposals/doctype/talk_proposal/talk_proposal.pydashboard/auto-imports.d.tsdashboard/components.d.tsdashboard/package.jsondashboard/src/components/ProposalEditDialog.vuedashboard/src/index.cssdashboard/src/pages/Account.vuedashboard/src/pages/ProposalDetails.vuedashboard/src/pages/ProposalsList.vuedashboard/src/router.js
✅ Files skipped from review due to trivial changes (1)
- dashboard/src/index.css
🧰 Additional context used
📓 Path-based instructions (2)
**/*.vue
📄 CodeRabbit inference engine (.github/instructions/frappe-ui.instructions.md)
**/*.vue: UsecreateResource()from frappe-ui to make backend API calls with configuration for url, params, and methods likefetch()andsubmit()in Vue components
UsecreateDocumentResource()to manage individual document records with support for whitelisted methods, events (onError, onSuccess, transform), and methods likereload(),setValue(),delete(), and custom whitelisted method calls
UseuseList()hook to create a list resource for fetching records of a DocType from Frappe backend with options for fields, filters, orderBy, pageLength, caching, and events (onError, onSuccess, transform)
UseListViewcomponent with propscolumns,rows,row-key, andoptionsto render tabular data with support for selection, tooltips, resizable columns, and custom rendering slots
Define ListView columns with requiredlabelandkeyproperties, optionalwidth(as multiplier or px/rem),align(start/left, center/middle, end/right), and custom attributes for rendering
Define ListView rows with unique identifier matching therow-keyprop, and field values as strings or objects withlabelproperty for custom rendering
For grouped ListView rows, structure data as array of objects withgroupstring,collapsedboolean, androwsarray containing row objects
UseBadgecomponent with propsvariant('solid', 'subtle', 'outline', 'ghost'),theme('gray', 'blue', 'green', 'orange', 'red'), andsize('sm', 'md', 'lg') for displaying status indicators
UseFormControlcomponent withtypeprop ('text', 'number', 'email', 'date', 'password', 'search', 'textarea', 'select', 'autocomplete', 'checkbox'),size('sm', 'md', 'lg', 'xl'),variant('subtle', 'outline'), and optionalprefixandsuffixslots
Usetoastfrom frappe-ui for notifications with methodstoast.success(),toast.warning(), andtoast.error()to provide user feedback
UseDropdowncomponent withoptionsprop containing action items withlabel,icon(lucide i...
Files:
dashboard/src/pages/ProposalDetails.vuedashboard/src/components/ProposalEditDialog.vuedashboard/src/pages/ProposalsList.vuedashboard/src/pages/Account.vue
{main.js,**/*.vue}
📄 CodeRabbit inference engine (.github/instructions/frappe-ui.instructions.md)
For Options API components using resources, register the
resourcesPluginin main.js and declare resources under theresourceskey as functions, accessible viathis.$resources.[name]
Files:
dashboard/src/pages/ProposalDetails.vuedashboard/src/components/ProposalEditDialog.vuedashboard/src/pages/ProposalsList.vuedashboard/src/pages/Account.vue
🧠 Learnings (6)
📚 Learning: 2025-12-30T06:17:13.961Z
Learnt from: CR
Repo: BuildWithHussain/buzz PR: 0
File: .github/instructions/frappe-ui.instructions.md:0-0
Timestamp: 2025-12-30T06:17:13.961Z
Learning: Applies to **/*.vue : Use `Dialog` component with `options` prop for title, message, size ('sm', 'lg', '4xl'), icon, and actions, or use slots (`body-title`, `body-content`, `actions`) for custom content layouts
Applied to files:
dashboard/components.d.tsdashboard/src/pages/ProposalDetails.vuedashboard/src/components/ProposalEditDialog.vuedashboard/src/pages/ProposalsList.vue
📚 Learning: 2025-12-30T06:17:13.961Z
Learnt from: CR
Repo: BuildWithHussain/buzz PR: 0
File: .github/instructions/frappe-ui.instructions.md:0-0
Timestamp: 2025-12-30T06:17:13.961Z
Learning: Applies to **/*.vue : Use `toast` from frappe-ui for notifications with methods `toast.success()`, `toast.warning()`, and `toast.error()` to provide user feedback
Applied to files:
dashboard/package.json
📚 Learning: 2025-12-30T06:17:13.961Z
Learnt from: CR
Repo: BuildWithHussain/buzz PR: 0
File: .github/instructions/frappe-ui.instructions.md:0-0
Timestamp: 2025-12-30T06:17:13.961Z
Learning: Applies to **/*.vue : Use `createDocumentResource()` to manage individual document records with support for whitelisted methods, events (onError, onSuccess, transform), and methods like `reload()`, `setValue()`, `delete()`, and custom whitelisted method calls
Applied to files:
dashboard/src/components/ProposalEditDialog.vue
📚 Learning: 2025-12-30T06:17:13.961Z
Learnt from: CR
Repo: BuildWithHussain/buzz PR: 0
File: .github/instructions/frappe-ui.instructions.md:0-0
Timestamp: 2025-12-30T06:17:13.961Z
Learning: Applies to **/*.vue : Use `ListView` component with props `columns`, `rows`, `row-key`, and `options` to render tabular data with support for selection, tooltips, resizable columns, and custom rendering slots
Applied to files:
dashboard/src/pages/ProposalsList.vuedashboard/src/pages/Account.vue
📚 Learning: 2025-12-30T06:17:13.961Z
Learnt from: CR
Repo: BuildWithHussain/buzz PR: 0
File: .github/instructions/frappe-ui.instructions.md:0-0
Timestamp: 2025-12-30T06:17:13.961Z
Learning: Applies to **/*.vue : Define ListView columns with required `label` and `key` properties, optional `width` (as multiplier or px/rem), `align` (start/left, center/middle, end/right), and custom attributes for rendering
Applied to files:
dashboard/src/pages/ProposalsList.vue
📚 Learning: 2025-12-30T06:17:13.961Z
Learnt from: CR
Repo: BuildWithHussain/buzz PR: 0
File: .github/instructions/frappe-ui.instructions.md:0-0
Timestamp: 2025-12-30T06:17:13.961Z
Learning: Applies to **/*.vue : Define ListView rows with unique identifier matching the `row-key` prop, and field values as strings or objects with `label` property for custom rendering
Applied to files:
dashboard/src/pages/ProposalsList.vue
🪛 Ruff (0.14.11)
buzz/proposals/doctype/talk_proposal/talk_proposal.py
24-24: Syntax error in forward annotation: Unexpected token at the end of an expression
(F722)
24-24: Undefined name Shortlisted
(F821)
24-24: Undefined name Accepted
(F821)
24-24: Undefined name Rejected
(F821)
🔇 Additional comments (23)
dashboard/auto-imports.d.ts (1)
10-11: LGTM!Auto-generated declarations for
LucideMicandLucideRadiofollow the established pattern and are consistent with existing icon imports.buzz/proposals/doctype/talk_proposal/talk_proposal.json (3)
42-42: Status option change looks consistent.The status option change from "Approved" to "Accepted" aligns with the corresponding updates in the JS and Python files. This ensures consistent terminology across the Talk Proposal workflow.
107-131: Permission structure looks appropriate.The new permission blocks follow the expected role hierarchy:
- Event Manager: Full CRUD with
selectpermission, appropriate for managing all proposals.- Buzz User: Restricted with
if_owner: 1, allowing users to manage only their own proposals.Note that Buzz User lacks
deletepermission (unlike Event Manager), which is a reasonable restriction.
137-142: State definition aligns with the new status.The "Accepted" state with green color provides visual feedback in the UI, consistent with the status options defined above.
buzz/proposals/doctype/talk_proposal/talk_proposal.js (1)
6-13: Status terminology is consistent across the workflow.The changes correctly update all three touchpoints:
- Condition check (line 6)
- Button label (line 7)
- Status assignment (line 13)
This ensures the "Accept" terminology is used consistently throughout the Talk Proposal acceptance flow.
buzz/proposals/doctype/talk_proposal/talk_proposal.py (1)
24-24: Type annotation update is correct; static analysis hints are false positives.The status literal change from "Approved" to "Accepted" is consistent with the DocType JSON definition.
The Ruff errors (F722, F821) are false positives—this is auto-generated Frappe type annotation code within a
TYPE_CHECKINGblock.DF.Literal[...]is a valid Frappe-specific type hint pattern that Ruff doesn't recognize. No action needed.dashboard/package.json (1)
17-17: Dependency update is appropriate.The
frappe-uiversion0.1.254exists on npm and is a reasonable bump from0.1.248. The caret versioning (^0.1.254) is appropriate for this package and allows compatible patch updates.Note: Version
0.1.255is currently available; consider updating to the latest if desired, though0.1.254is stable and recent.buzz/events/doctype/buzz_event/buzz_event.json (2)
224-235: LGTM! Well-structured talks configuration section.The new
talks_sectionandallow_editing_talks_after_acceptancefields are properly defined with appropriate defaults ("0") and a clear description explaining the behavior. This aligns with the dashboard UI changes for proposal editing.
290-296: Good UX improvement with the fallback description.Adding the description "Default template will be used if not set" helps users understand the global fallback behavior introduced in this PR. The removal of
mandatory_depends_oncorrectly allows the field to be optional when global defaults exist.dashboard/src/router.js (1)
75-85: LGTM! Routes follow established patterns.The new proposal routes are consistent with the existing structure for bookings, tickets, and sponsorships. Lazy loading and
props: truefor the detail route are correctly applied.dashboard/components.d.ts (1)
31-31: LGTM!The
ProposalEditDialogcomponent declaration follows the established pattern and maintains alphabetical ordering.dashboard/src/pages/ProposalsList.vue (1)
59-74: The field alias syntax"event.title as event_title"is correct and fully supported by Frappe. This is the documented approach for fetching linked document fields with aliases usingget_list. The same syntax is already used in multiple files across the codebase (TicketsList.vue, BookingsList.vue). No action needed.dashboard/src/components/ProposalEditDialog.vue (6)
1-56: LGTM!The template structure follows frappe-ui conventions correctly. The Dialog uses appropriate slots (
body-title,body-content,actions), and the form controls are properly configured with validation states and loading indicators.
58-79: LGTM!Props are well-defined with correct types and appropriate defaults. The factory function for
initialDataprevents shared reference issues.
81-92: LGTM!The
v-modelpattern for the dialog's open state is correctly implemented using a computed property with getter/setter.
94-111: LGTM!The
createResourceconfiguration correctly usesfrappe.client.set_valuewith appropriate success and error handlers. Error handling includes a fallback message pattern.
113-141: LGTM!The save handler correctly distinguishes between updating an Event Talk vs Talk Proposal based on
eventTalkId. The validation aligns with the button's disabled state.
143-171: LGTM!The watchers correctly handle form initialization on dialog open and synchronization when
initialDatachanges asynchronously. Thedeep: trueoption is appropriate for the object prop.dashboard/src/pages/ProposalDetails.vue (5)
1-8: LGTM!The header correctly integrates navigation and conditionally displays the edit button based on computed edit permissions.
20-98: LGTM!The status alert banners are well-structured with appropriate colors, icons, and messaging for each status. The
v-if/v-else-ifchain ensures mutual exclusivity.
174-186: LGTM!The dialog integration correctly handles both editing modes (Talk Proposal vs Event Talk) with appropriate data sourcing for
initialData.
189-212: LGTM!Imports are well-organized and the prop definition is appropriately minimal.
214-264: LGTM!The document resource and dependent resources are correctly structured. The watchers appropriately trigger fetches based on available data and status changes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| { | ||
| "create": 1, | ||
| "email": 1, | ||
| "export": 1, | ||
| "if_owner": 1, | ||
| "print": 1, | ||
| "read": 1, | ||
| "report": 1, | ||
| "role": "Buzz User", | ||
| "select": 1, | ||
| "share": 1, | ||
| "write": 1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare Buzz User permissions between talk_proposal.json and event_talk.json
echo "=== Talk Proposal Buzz User permissions ==="
cat buzz/proposals/doctype/talk_proposal/talk_proposal.json | jq '.permissions[] | select(.role == "Buzz User")'
echo ""
echo "=== Event Talk Buzz User permissions ==="
cat buzz/events/doctype/event_talk/event_talk.json | jq '.permissions[] | select(.role == "Buzz User")'Repository: BuildWithHussain/buzz
Length of output: 479
Clarify permission inconsistency for Buzz User role.
Event Talk Buzz User includes select: 1, but the corresponding Buzz User permission in Talk Proposal does not. Align these permissions across both DocTypes or document why the difference is intentional (e.g., if Event Talk requires select for Link field functionality).
| const onProposalUpdated = () => { | ||
| proposal.reload(); | ||
| if (proposal.doc?.status === "Accepted") { | ||
| eventTalkResource.fetch(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Race condition in onProposalUpdated handler.
After calling proposal.reload(), the subsequent check for proposal.doc?.status will evaluate the old status before the reload completes. If the status changed, the eventTalk fetch may not trigger correctly.
🔧 Suggested fix using callback or awaiting reload
-const onProposalUpdated = () => {
- proposal.reload();
- if (proposal.doc?.status === "Accepted") {
- eventTalkResource.fetch();
- }
+const onProposalUpdated = async () => {
+ await proposal.reload();
+ if (proposal.doc?.status === "Accepted") {
+ eventTalkResource.fetch();
+ }
};Alternatively, rely on the existing watcher at lines 256-264 which already handles fetching eventTalkResource when status becomes "Accepted".
📝 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.
| const onProposalUpdated = () => { | |
| proposal.reload(); | |
| if (proposal.doc?.status === "Accepted") { | |
| eventTalkResource.fetch(); | |
| } | |
| }; | |
| const onProposalUpdated = async () => { | |
| await proposal.reload(); | |
| if (proposal.doc?.status === "Accepted") { | |
| eventTalkResource.fetch(); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@dashboard/src/pages/ProposalDetails.vue` around lines 321 - 326,
onProposalUpdated currently calls proposal.reload() but immediately checks
proposal.doc?.status, causing a race where the old status may be read; change
onProposalUpdated to await the async reload (or attach a .then callback) before
checking proposal.doc?.status and calling eventTalkResource.fetch(), or remove
this logic and rely on the existing watcher that already triggers fetch when
status becomes "Accepted" to avoid duplicate checks; update the
onProposalUpdated function accordingly (reference: onProposalUpdated,
proposal.reload, eventTalkResource.fetch).
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Other Updates
✏️ Tip: You can customize this high-level summary in your review settings.