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.

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

#111 | Phone Number Validations and Other Validations Have Been Fixed #114

Merged
merged 12 commits into from
Jul 12, 2023

Conversation

ilkerCelimli
Copy link
Contributor

@ilkerCelimli ilkerCelimli commented Jul 4, 2023

PULL REQUEST TEMPLATE

Description

Phone Number Validations and Other Validations Have Been Fixed

Related Issue

As we defined lineNumber and countryCode as Long, we have some problems to handle with so that we can convert all these variables to String

Proposed Changes

Please provide a brief and descriptive title for your pull request, summarizing the changes or features introduced. It
should be in English and clearly indicate the purpose of the pull request. Optionally, you can provide a more detailed
description of what you have done.

  • refactor/111/phone-number-length-limited

Additional Information

If there's any additional information or context that the reviewers should know, please provide it here.

Checklist

Please check the following before submitting your pull request:

  • I have tested my changes locally and they are working as expected.
  • The code is formatted according to the project's coding guidelines and style.
  • Necessary documentation has been added or existing documentation has been updated.
  • Relevant unit tests have been written and included.
  • Relevant integration tests have been written and included.
  • The code has been reviewed to ensure its quality.
  • The code does not contain any issues flagged by SonarLint.
  • Default reviewers have been assigned to this pull request.
  • Assignees have been added if necessary.
  • Labels have been applied if necessary.
  • If an issue is opened and this pull request is related to that issue, it should not be associated with any project
    board or milestone. However, if there is no issue, the pull request should be associated with the appropriate project
    board (BE Board, In Review column) and milestone to track the progress.

Reviewers

The default project maintainers and team members will be automatically assigned as reviewers for this pull request.

@ilkerCelimli
Copy link
Contributor Author

ilkerCelimli commented Jul 4, 2023

@agitrubard , @Rapter1990 @skayikci

Mecburen Veritabanında değişiklik yapmak zorunda kaldım koaly bir şekilde işin halledilebilmesi için,

Bir sıkıntı var ise haber edersiniz.

@ilkerCelimli ilkerCelimli marked this pull request as draft July 4, 2023 13:45
@ilkerCelimli ilkerCelimli marked this pull request as ready for review July 4, 2023 14:01
@ilkerCelimli ilkerCelimli linked an issue Jul 4, 2023 that may be closed by this pull request
@ilkerCelimli ilkerCelimli changed the title Limited PhoneNumber Country code and Phonenumber Length, bugs/Limited PhoneNumber Country code and Phonenumber Length, Jul 4, 2023
@ilkerCelimli ilkerCelimli marked this pull request as draft July 4, 2023 14:41
@ilkerCelimli
Copy link
Contributor Author

LineNumber kısmı 11 hanelide hata vermeye başladığı için, Onu String, ve veri tabanı kısmında Varchar(10) yaptım,
Country code kısmı zaten düzgün çalışıyor onu ellemeye gerek yok,
@skayikci
@agitrubard
@Rapter1990 bi kontrol eder misiniz eksik var mı ?

@ilkerCelimli ilkerCelimli marked this pull request as ready for review July 4, 2023 15:44
@agitrubard
Copy link
Collaborator

@ilkerCelimli branch standartlarına uyması açısından bundan sonraki geliştirmelerde branch açtığın zaman aşağıdaki branch standartlarına uygun olacak şekilde açabilir misin?


Naming Conventions of Branchs

  • bugfix/{issue-number}/{optional-description}
    • Ex: bugfix/0/authentication-flow or bugfix/0
  • feature/{issue-number}_{optional-description}
    • Ex: feature/0/admin-register-flow or feature/0
  • refactor/{issue-number}_{optional-description}
    • Ex: refactor/0/tests-classes or refactor/0
  • hotfix/{issue-number}_{optional-description}
    • Ex: hotfix/0/authorization-flow or hotfix/0

Naming Conventions of Pull Requests

  • #{issue-number} | {header-for-summary-of-development}
    • Ex: #0 | Authentication/Authorization Flows Has Been Created

Bu kısımlar README.md dosyasında yazmıyordu sanırım, ekliyor olacağım.

Copy link
Collaborator

@agitrubard agitrubard left a comment

Choose a reason for hiding this comment

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

Benzer geliştirmeleri AdminUserEntity içerisinde de yapmalıyız. Ek olarak Admin Kayıt ve Kullanıcı Kayıt akışındaki requestlerde bulunan AysPhoneNumber objesi içerisinde de bu kısıtlamaları yapmalıyız. Fakat burada neden String kullanma ihtiyacımız olduğunu tam olarak anlayamadım. @Range anotasyonu ile kısıtlayabiliyoruz, veritabanında da INTEGER(10) olarak uzunluğunu belirtebiliyoruz.

changed LINE_NUMER big int -> varchar(5)
@ilkerCelimli
Copy link
Contributor Author

ilkerCelimli commented Jul 4, 2023

Veritabanı tarafını bilemem, MYSQL pek kullanmadım,

String kullanılma sebebi, Her bir character'in 1 byte olması ve varchar(10) durumunda bellekte sadece 10 byte alan açılması,bu da performans artışı sağlar,

BIGINT 2^63 tü sanırsam kadar default olarak alan açtığı için, buda veri boyutunu büyük oranda arttırır.

Bir başka String kullanma sebebi ise, telefon no'ları için konuşmak gerekirse String üzerinde işlem yapması daha basit olacağı için,

@range anotasyonu için konuşmak gerekirse,Açıkcası hiç kullanmamıştım onu , o yüzden yorum yapamıcam,

@ilkerCelimli ilkerCelimli changed the title bugs/Limited PhoneNumber Country code and Phonenumber Length, bugFix/111 Jul 4, 2023
@ilkerCelimli ilkerCelimli requested a review from agitrubard July 5, 2023 15:20
@agitrubard
Copy link
Collaborator

@ilkerCelimli uygun olduğunda birlikte inceleyelim mi?

@agitrubard
Copy link
Collaborator

@ilkerCelimli ek olarak aşağıdaki yorum içerisinde yazdığım standartları gözden geçirebilir misin?

#114 (comment)

@ilkerCelimli ilkerCelimli changed the title bugFix/111 bugFix/111/limited-phone-number-length Jul 5, 2023
@ilkerCelimli ilkerCelimli changed the title bugFix/111/limited-phone-number-length bugfix/111/limited-phone-number-length Jul 5, 2023
@ilkerCelimli
Copy link
Contributor Author

@ilkerCelimli ek olarak aşağıdaki yorum içerisinde yazdığım standartları gözden geçirebilir misin?

#114 (comment)

Standartlara uygun olarak olarak değiştirdim PR ismini, Tabi müsait ikimizde müsait olduğumuzda bakalım,Ama şuan bir sıkıntı olmaması gerek.

@ilkerCelimli ilkerCelimli changed the title bugfix/111/limited-phone-number-length #111/limited-phone-number-length Jul 5, 2023
changed LINE_NUMER varchar(10) -> INTEGER(10)
@ilkerCelimli
Copy link
Contributor Author

@agitrubard Gerekli düzenlemeyi yaptım, Bi kontrol edebilirseniz sevinirim.

@moaydogdu
Copy link
Contributor

CountryCode ve LineNumber alanları için bazı uyumsuzluklar mevcut.

  • LineNumber kısmı 13 haneye kadar gidebiliyor(China).
  • CountryCode kısmı 7 haneye kadar gidebiliyor (Christmas Island, Cocos (Keeling) Islands).

Kaynaklar:

@ilkerCelimli
Copy link
Contributor Author

@agitrubard değiliklikleri yaptım, Ama 13 'e 7 yapsak daha iyi olucak sanırsam

@Rapter1990 Rapter1990 added in review This issue in review refactor Improvements to project labels Jul 9, 2023
@Rapter1990 Rapter1990 changed the title #111/limited-phone-number-length #111 | limited-phone-number-length Jul 10, 2023
@agitrubard
Copy link
Collaborator

@ilkerCelimli Selamlar,
Pull request başlığını aşağıdaki standarda uyacak şekilde güncelleyebilir miyiz?

Naming Conventions of Pull Requests

  • #{issue-number} | {header-for-summary-of-development}
    • Ex: #0 | Authentication/Authorization Flows Has Been Created

@agitrubard
Copy link
Collaborator

agitrubard commented Jul 10, 2023

@agitrubard değiliklikleri yaptım, Ama 13 'e 7 yapsak daha iyi olucak sanırsam

@ilkerCelimli bu konuda standart bu şekildeyse böyle güncelleyebiliriz, fakat çalışmama gibi bir problem ile karşılaşmamak için detaylı testlerini yapmamız sağlıklı olur. Unit test'ten bahsetmiyorum, bunun dışındaki manuel http request ile yapılan testlerden bahsediyorum. Ek olarak uzunluk ile ilgili de bir unit test case yazarak bu geliştirmeyi en doğru şekilde cover etmiş oluruz.

@agitrubard agitrubard changed the title #111 | limited-phone-number-length #111 | Phone Number Validations and Other Validations Have Been Fixed Jul 11, 2023
@agitrubard
Copy link
Collaborator

@ilkerCelimli eline sağlık, Long veri tipi ile ilgili test yaptığımda çok problemler çıkıyordu, testlerle beraber düzelttim. Diğer arkadaşlar da kontrol ederse mergeleyebiliriz.

@agitrubard agitrubard removed the in review This issue in review label Jul 12, 2023
Copy link
Contributor

@Rapter1990 Rapter1990 left a comment

Choose a reason for hiding this comment

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

LGTM!

@agitrubard agitrubard merged commit 1d1c54f into main Jul 12, 2023
@agitrubard agitrubard deleted the phone-number-length-limited branch July 12, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Improvements to project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Telefon Numarası Uzunluklarının Sınırlandırılması
4 participants