Setup wizard refactor #3548
Setup wizard refactor #3548
Conversation
Pull Request SummaryImage or animted GIF Not AddedPlease add an image or animated GIF as proof that you have manually tested this contribution. Hint: use LiceCAP to capture animated GIFs. Test Case Not Added / UpdatedSince you have changed a Python file, you must update the relevant python test case. If there is no test coverage for this code, then please add it. Large PatchThis is a very large pull request, unless there is a very good reason, please try and break it down to smaller changes. Read this strategy on how it can be done Result
This summary was automatically generated based on this script |
|
|
||
| .setup-wizard-slide .img-overlay { | ||
| display: flex; | ||
| align-items: center; |
| } | ||
|
|
||
| @frappe.whitelist() | ||
| def load_country(): | ||
| import requests |
There was a problem hiding this comment.
Remove this. We should not rely on 3rd party services as they are likely to have rate limits. And if users are on a VM, this is not accessible.
There was a problem hiding this comment.
Found frappe.sessions.get_geo_ip_country for this
|
|
||
| @frappe.whitelist() | ||
| def load_user_details(): | ||
| return { |
There was a problem hiding this comment.
Will stay undefined, right? So no autofilling
| $("body").css({"padding-top":"30px"}); | ||
|
|
||
| $('header').append(`<div class="setup-wizard-brand""> | ||
| <img src="/assets/frappe_theme/img/erp-icon.svg" class="brand-icon" |
There was a problem hiding this comment.
dependency on frappe_theme app !!
Also, ERPNext logo should not be included in frappe app, may be we can extend the icon if erpnext app is not installed then show only frappe icon
| ], | ||
| help: __('The first user will become the System Manager (you can change this later).'), | ||
| onload: function(slide) { | ||
| if(user!=="Administrator") { |
There was a problem hiding this comment.
user is deprecated use frappe.session.user
|
Please push the changes and reopen the PR |
|
Frappe header image: |
| @frappe.whitelist() | ||
| def load_country(): | ||
| from frappe.sessions import get_geo_ip_country | ||
| return get_geo_ip_country(frappe.local.request_ip) if frappe.local.request_ip else None |
There was a problem hiding this comment.
do we load geoip database by default?
With frappe/erpnext#9441