Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Add multiple phonePrefix #1524

Closed
wants to merge 5 commits into from
Closed

feat: Add multiple phonePrefix #1524

wants to merge 5 commits into from

Conversation

jakiuncle
Copy link
Member

@jakiuncle jakiuncle commented Feb 6, 2023 •
edited by hsluoyz

Fix: #1397

@jakiuncle
feat: add multiple phonePrefix
4f822ec
@jakiuncle
feat: Add phone region control
edbe40b
@jakiuncle
feat: Add migrate
b927732
@jakiuncle
Merge branch 'phoneprefix' of https://github.com/jakiuncle/casdoor in…
6408297 
…to phoneprefix

� Conflicts:
�	object/organization.go
�	web/src/OrganizationEditPage.js
�	web/src/auth/SignupPage.js
@casbin-bot
Copy link
Contributor

@seriouszyx @ComradeProgrammer @Resulte please review

leo220yuyaodog
leo220yuyaodog reviewed Feb 6, 2023
View reviewed changes
object/permission.go
@@ -296,6 +296,29 @@ func MigratePermissionRule() {
}
}

func MigratePhonePrefix() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create a new file named migrate is better.

controllers/verification.go
@@ -142,7 +143,11 @@ func (c *ApiController) SendVerificationCode() {
return
}

dest = fmt.Sprintf("+%s%s", organization.PhonePrefix, dest)
if (phonePrefix == "" || phonePrefix == "undefined") && userByPhone != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle undefine in frontend. Just has "" condition in backend.

object/user.go Show resolved Hide resolved
object/user.go
@@ -177,6 +178,7 @@ type Userinfo struct {
Avatar string `json:"picture,omitempty"`
Address string `json:"address,omitempty"`
Phone string `json:"phone,omitempty"`
PhonePrefix string `json:"phonePrefix,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not need. The Userinfo struct is for oidc protocol.

hsluoyz
hsluoyz requested changes Feb 7, 2023
View reviewed changes
controllers/verification.go
phonePrefix = organization.PhonePrefix
}
checkDest = fmt.Sprintf("+%s%s", phonePrefix, dest)
checkDest = fmt.Sprintf("%s%s", phonePrefix, dest)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can phonePrefix be empty?

controllers/verification.go
@@ -177,7 +184,7 @@ func (c *ApiController) ResetEmailOrPhone() {
checkDest := dest
organization := object.GetOrganizationByUser(user)
if destType == "phone" {
if object.HasUserByField(user.Owner, "phone", user.Phone) {
if object.HasUserByField(user.Owner, "phone", dest) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change it?

controllers/auth.go
@@ -240,6 +240,7 @@ func (c *ApiController) Login() {
var user *object.User
var msg string

// Login with code
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this

controllers/account.go
@@ -137,7 +137,7 @@ func (c *ApiController) Signup() {

var checkPhone string
if application.IsSignupItemVisible("Phone") && form.Phone != "" {
checkPhone = fmt.Sprintf("+%s%s", form.PhonePrefix, form.Phone)
checkPhone = fmt.Sprintf("%s%s", form.PhonePrefix, form.Phone)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove + ?

controllers/auth.go
@@ -257,16 +258,13 @@ func (c *ApiController) Login() {
}
checkDest = form.Username
} else {
verificationCodeType = "phone"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this?

web/src/ResetModal.js
@@ -97,7 +101,7 @@ export const ResetModal = (props) => {
textBefore={i18next.t("code:Code You Received")}
onChange={setCode}
method={"reset"}
onButtonClickArgs={[dest, destType, Setting.getApplicationName(application)]}
onButtonClickArgs={[dest, destType, Setting.getApplicationName(application), null, phonePrefix]}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's null?

web/src/SelectPhonePrefixBox.js
if (isOrgnazition) {
phoneArray = Setting.CountryPrefiexphone;
} else {
for (let i = 0; i < this.props.obj.phonePrefix.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

foreach

web/src/SelectPhonePrefixBox.js

render() {
const isOrgnazition = this.props.isOrgnazition;
const wid = isOrgnazition ? "100%" : 100;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wid?

web/src/Setting.js
@@ -204,7 +207,7 @@ export const OtherProviderInfo = {
},
};

export function getCountryRegionData() {
export function getCountryRegionData(key) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't mess with getCountryRegionData

web/src/auth/LoginPage.js
@@ -189,7 +189,7 @@ class LoginPage extends React.Component {
} else {
values["type"] = this.state.type;
}
values["phonePrefix"] = this.getApplicationObj()?.organizationObj.phonePrefix;
// values["phonePrefix"] = this.getApplicationObj()?.organizationObj.phonePrefix[0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove

@hsluoyz hsluoyz mentioned this pull request Feb 7, 2023
Closed
@hsluoyz hsluoyz mentioned this pull request Feb 16, 2023
Merged
@hsluoyz
Copy link
Member

hsluoyz commented Feb 16, 2023

Replaced by: #1557

@hsluoyz hsluoyz closed this Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leo220yuyaodog leo220yuyaodog leo220yuyaodog left review comments

@hsluoyz hsluoyz hsluoyz requested changes

@ComradeProgrammer ComradeProgrammer Awaiting requested review from ComradeProgrammer

@Resulte Resulte Awaiting requested review from Resulte

@seriouszyx seriouszyx Awaiting requested review from seriouszyx

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow changing the country code of phone number on login page
4 participants
@jakiuncle @casbin-bot @hsluoyz @leo220yuyaodog