-
Notifications
You must be signed in to change notification settings - Fork 12
feat: login required functionality v1 #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a1dcdcc
3758472
68f7b4d
98ce49e
919e5f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| import frappe | ||
| from frappe.core.doctype.navbar_settings.navbar_settings import get_app_logo | ||
|
|
||
|
|
||
| @frappe.whitelist(allow_guest=True) | ||
| def get_brand_logo() -> str: | ||
| """ | ||
| Get the brand logo for the form. | ||
|
|
||
| Returns: | ||
| str: The brand logo of the website or the default app logo if brand logo is not set. | ||
| """ | ||
| return str(get_app_logo()) | ||
|
Comment on lines
+1
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find the get_app_logo function definition in the Frappe codebase
find . -type f -name "*.py" | head -20Repository: BuildWithHussain/forms_pro Length of output: 917 🏁 Script executed: # Search for get_app_logo function definition
rg -n "def get_app_logo" --type=py -A 15Repository: BuildWithHussain/forms_pro Length of output: 52 🏁 Script executed: # Search for imports of get_app_logo and usage patterns
rg -n "get_app_logo" --type=py | head -30Repository: BuildWithHussain/forms_pro Length of output: 234 🌐 Web query:
💡 Result: Short answer — use website context or a guest-allowed endpoint. Steps:
References:
If you want, tell me whether you need an example update_website_context implementation or a whitelisted method snippet. 🌐 Web query:
💡 Result: The function is implemented in the Navbar Settings DocType class at: Sources:
🌐 Web query:
💡 Result: I searched GitHub for Frappe references to get_app_logo and navbar/navbar_settings. Relevant repo issues and threads discussing navbar/app logo behavior and Website Settings include:
If you want, I can (a) search for the exact function/implementation file that computes/returns the app logo within the Frappe codebase, or (b) open the specific files in the frappe/erpnext repos to locate where navbar_settings or get_app_logo are implemented. Which would you like next? 🏁 Script executed: # Try to find if get_app_logo is documented in Frappe or if there's any error handling pattern
rg -i "get_app_logo|navbar_settings" --type=pyRepository: BuildWithHussain/forms_pro Length of output: 229 🌐 Web query:
💡 Result: I couldn't find a definitive copy of navbar_settings.py/get_app_logo across public Frappe sources. Which Frappe version or repository (GitHub URL or your site) should I look in? I can fetch and paste the exact function once you point me to the correct branch/repo/version. 🌐 Web query:
💡 Result: I couldn't find a function named def get_app_logo in the Frappe codebase. Frappe exposes app/logo via hooks and website settings instead — e.g. the login page uses frappe.get_website_settings("app_logo") or the hooks value app_logo_url (frappe.get_hooks("app_logo_url")[-1]). See the hooks docs and the forum discussion for the code references. [1][2] Sources:
🏁 Script executed: # Look for test files that might exercise this code
find . -name "*test*.py" | xargs grep -l "get_brand_logo\|settings" 2>/dev/null | head -10Repository: BuildWithHussain/forms_pro Length of output: 52 🏁 Script executed: # Check if there are any requirements or setup files specifying Frappe version
find . -name "requirements.txt" -o -name "setup.py" -o -name "pyproject.toml" | xargs cat 2>/dev/nullRepository: BuildWithHussain/forms_pro Length of output: 1103 🏁 Script executed: # Search for any error handling or try-catch around the get_app_logo import
rg -B5 -A5 "get_app_logo" --type=pyRepository: BuildWithHussain/forms_pro Length of output: 756 Verify The web search could not find Confirm the import path and add error handling: @frappe.whitelist(allow_guest=True)
def get_brand_logo() -> str:
"""
Get the brand logo for the form.
Returns:
str: The brand logo of the website or the default app logo if brand logo is not set.
"""
try:
return str(get_app_logo())
except Exception as e:
# Return a fallback or log the error
frappe.logger().warning(f"Failed to get app logo: {e}")
return ""🤖 Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It exists in frappe's codebase There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| <script setup> | ||
| import { formFields } from "@/utils/form_fields"; | ||
| <script setup lang="ts"> | ||
| import { FormFields, formFields, FormFieldType } from "@/utils/form_fields"; | ||
| import { computed } from "vue"; | ||
|
|
||
| const props = defineProps({ | ||
| field: { | ||
| type: Object, | ||
|
|
@@ -10,15 +11,9 @@ const props = defineProps({ | |
|
|
||
| const value = defineModel(); | ||
| const getComponent = computed(() => { | ||
| return formFields.find((field) => field.name === props.field.fieldtype); | ||
| }); | ||
|
|
||
| const getBinds = computed(() => { | ||
| return { | ||
| ...props.field, | ||
| ...props.field.options, | ||
| ...props.field.default, | ||
| }; | ||
| return formFields.find( | ||
| (field: FormFields) => field.name === props.field.fieldtype | ||
| ) as FormFieldType; | ||
| }); | ||
|
Comment on lines
13
to
17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential runtime error when field type is not found.
🔎 Proposed fix with fallback handling const getComponent = computed(() => {
- return formFields.find(
+ const found = formFields.find(
(field: FormFields) => field.name === props.field.fieldtype
- ) as FormFieldType;
+ );
+ if (!found) {
+ console.warn(`Unknown field type: ${props.field.fieldtype}`);
+ return DataField; // fallback to DataField
+ }
+ return found;
});You'll need to import
🤖 Prompt for AI Agents |
||
| </script> | ||
| <template> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,11 +10,11 @@ const submissionFormStore = useSubmissionForm(); | |
| </div> | ||
| <div v-if="submissionFormStore.formResource.data" class="flex flex-col gap-4 text-gray-800"> | ||
| <h1 class="text-3xl font-bold"> | ||
| {{ submissionFormStore.formResource.data?.title ?? "" }} | ||
| {{ submissionFormStore.formResource.data.title }} | ||
| </h1> | ||
| <div | ||
| v-html="submissionFormStore.formResource.data?.description" | ||
| class="form-description" | ||
| v-html="submissionFormStore.formResource.data.description" | ||
| class="form-description !px-0" | ||
| ></div> | ||
|
Comment on lines
11
to
18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # Find the submission form store and formResource definition
fd -e ts -e vue -e js | head -20
# Search for formResource usage
rg -n 'formResource' --type vue --type ts -B2 -A2
# Look for the API endpoints
fd -name '*.py' | xargs grep -l 'form.*resource\|frappe.whitelist'Repository: BuildWithHussain/forms_pro Length of output: 1241 🏁 Script executed: # Find all Vue and TypeScript files with formResource
rg 'formResource' --type-list | head
# Search for formResource in the codebase
rg 'formResource' -B2 -A2
# Find the submission form store
fd 'store|submission' --type f | grep -E '\.(ts|js|vue)$'
# Look for API endpoints
fd '\.py$' | xargs grep -l 'form' | head -20Repository: BuildWithHussain/forms_pro Length of output: 17886 🏁 Script executed: # Check the API implementation for get_form_by_route
cat frontend/src/stores/submissionForm.ts | head -50
# Look at the form.py API
cat forms_pro/api/form.py
# Check the Form doctype definition
fd 'form.json' | head -5Repository: BuildWithHussain/forms_pro Length of output: 5302 🏁 Script executed: # Read the Form doctype JSON
cat forms_pro/forms_pro/doctype/form/form.json | grep -A10 -B2 '"fieldname": "title"' | head -20
# Also check for description field
cat forms_pro/forms_pro/doctype/form/form.json | grep -A10 -B2 '"fieldname": "description"' | head -20
# Get file size first to decide how to read it
wc -l forms_pro/forms_pro/doctype/form/form.jsonRepository: BuildWithHussain/forms_pro Length of output: 549 Add optional chaining for the The 🤖 Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not required There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @harshtandiya, understood. I'll note your decision on this. |
||
| <hr /> | ||
| </div> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| <script setup lang="ts"> | ||
| import { Button, createResource, Popover } from "frappe-ui"; | ||
| import { session } from "@/data/session"; | ||
| import { useRoute } from "vue-router"; | ||
| import Avatar from "../ui/Avatar.vue"; | ||
|
|
||
| const route = useRoute(); | ||
| const brandLogo = createResource({ | ||
| url: "forms_pro.api.settings.get_brand_logo", | ||
| auto: true, | ||
| }); | ||
|
Comment on lines
+8
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Add error handling and loading state for brand logo. The brand logo is rendered directly without handling loading or error states. If 🔎 Proposed improvements <template>
<div class="max-w-screen-md mx-auto flex justify-between items-center">
- <img :src="brandLogo.data" alt="Brand Logo" class="w-10 h-10" />
+ <img
+ v-if="brandLogo.data"
+ :src="brandLogo.data"
+ alt="Brand Logo"
+ class="w-10 h-10"
+ />
+ <div v-else class="w-10 h-10 bg-gray-200 rounded animate-pulse"></div>
<div>Also applies to: 19-19 🤖 Prompt for AI Agents |
||
|
|
||
| function redirectToLogin() { | ||
| window.location.href = `/login?redirect-to=${route.fullPath}`; | ||
| } | ||
|
Comment on lines
+13
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. URL-encode the redirect path. The 🔎 Proposed fix function redirectToLogin() {
- window.location.href = `/login?redirect-to=${route.fullPath}`;
+ window.location.href = `/login?redirect-to=${encodeURIComponent(route.fullPath)}`;
}🤖 Prompt for AI Agents |
||
| </script> | ||
| <template> | ||
| <div class="max-w-screen-md mx-auto flex justify-between items-center"> | ||
| <img :src="brandLogo.data" alt="Brand Logo" class="w-10 h-10" /> | ||
| <div> | ||
| <Popover v-if="session.user"> | ||
| <template #target="{ togglePopover }"> | ||
| <Button variant="ghost" @click="togglePopover"> | ||
| <Avatar :userId="session.user" size="lg" /> | ||
| </Button> | ||
| </template> | ||
| <template #body-main> | ||
| <div class="flex flex-col gap-2 bg-white rounded-lg p-2"> | ||
| <Button | ||
| variant="ghost" | ||
| theme="red" | ||
| @click="session.logout.submit" | ||
| label="Log out" | ||
| icon-left="log-out" | ||
| /> | ||
| </div> | ||
| </template> | ||
| </Popover> | ||
| <Button v-else @click="redirectToLogin">Login</Button> | ||
| </div> | ||
| </div> | ||
| </template> | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix docstring parameter name and verify None handling.
Two concerns:
form_idin the args section, but the parameter is namedroute.frappe.db.get_valuereturnsNone(route doesn't exist),bool(None)returnsFalse, making non-existent forms appear as "login not required". Consider if this is the desired behavior or if you should explicitly handle the non-existent route case.🔎 Proposed fix for docstring
""" Check if login is enabled for a form. args: - form_id: str - The ID of the form to check. + route: str - The route of the form to check. returns: bool - True if login is required, False otherwise. """📝 Committable suggestion
🤖 Prompt for AI Agents