-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
New Contacts Module and Salutation and Gender in Contact #3079
Conversation
update_gender_and_salutation() | ||
|
||
def update_gender_and_salutation(): | ||
default_genders = ["Male", "Female", "Other"] |
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.
duplicate code 😢
|
||
default_genders = ["Male", "Female", "Other"] | ||
|
||
default_salutations = ["Dr.", "Mr.", "Ms.", "Mx.", "Sir", "Dame", "Esq.", "Lady", "Lord", "Mrs.", "Sire", "Madam", "Miss.", "Master", "Proff.", "Captain", "Mistress", "Gentleman", "President", "Principal"] |
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.
Lets not add the period .
also Professor is Prof
not Proff
... Mistress
?? 😜
Lets only keep the common ones
ed06c3c
to
3fa2615
Compare
update_gender_and_salutation() | ||
|
||
def update_gender_and_salutation(): | ||
records = get_genders_and_salutations() |
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.
even insertion should be common
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.
Done
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.
get_genders_and_salutations()
and add_genders_and_salutations(record)
can be the same function!
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.
for better readability and clarity :P
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.
6aa2243
to
eaea35c
Compare
records = [{'doctype': 'Gender', 'gender': d} for d in default_genders] | ||
records += [{'doctype': 'Salutation', 'salutation': d} for d in default_salutations] | ||
for record in records: | ||
doc_name = record.get("doctype") |
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.
have you tested if this works?
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.
Tested and removed the if exist code, not required since we are checking DuplicateEntryError
f63580d
to
6e4138e
Compare
1d5023d
to
6f34582
Compare
6f34582
to
915c40d
Compare
This reverts commit f52e389.
Also merge frappe/erpnext#9199