-
Notifications
You must be signed in to change notification settings - Fork 15
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
AYS-661 | Refactor email validation limits #431
base: main
Are you sure you want to change the base?
Conversation
…min length to 6 characters - AYS-661 : Set email validation max length to 254 characters - Add test cases for email validation limits
@m1erla selamlar, |
Ben test ederken, yaptığım düzenlemeler sorunsuz geçti testlerden ama build aldığımda diğer test dosyalarından hata almıştım ama benim düzenlememde olmayan test dosyalarıydı. Onlara ayrı bir issue açmak gerek sanırım. |
@m1erla Belli ki yaptığımız geliştirmeler bir şeyleri bozmuş. Bu sebeple oradaki testlerin de düzeltilmesi gerekiyor. Ayrı bir issue açmamız gerekmiyor şu an için. Bu issue kapsamında yapılan değişikliklerin test etkileri olmuş gibi görünüyor. |
src/main/java/org/ays/auth/model/request/AdminRegistrationApplicationCompleteRequest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/ays/auth/model/request/AysPasswordForgotRequest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/ays/auth/model/request/AysUserCreateRequest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/ays/auth/model/request/AysUserCreateRequest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/ays/auth/model/request/AysUserCreateRequest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/ays/emergency_application/model/EmergencyEvacuationApplicationBuilder.java
Outdated
Show resolved
Hide resolved
...test/java/org/ays/emergency_application/model/request/EmergencyEvacuationRequestBuilder.java
Outdated
Show resolved
Hide resolved
...test/java/org/ays/emergency_application/model/request/EmergencyEvacuationRequestBuilder.java
Outdated
Show resolved
Hide resolved
...test/java/org/ays/emergency_application/model/request/EmergencyEvacuationRequestBuilder.java
Outdated
Show resolved
Hide resolved
...test/java/org/ays/emergency_application/model/request/EmergencyEvacuationRequestBuilder.java
Outdated
Show resolved
Hide resolved
…@SiZe annotations on emailAddress. Added some invalid email value.
* Extends {@link BaseDomainModel} and includes details such as personal and | ||
* contact information, |
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.
Burayi eski haline cevirebiliriz
@@ -72,12 +71,14 @@ public void pending() { | |||
this.referenceNumber = AysRandomUtil.generateNumber(10).toString(); | |||
this.status = EmergencyEvacuationApplicationStatus.PENDING; | |||
this.isInPerson = this.applicantPhoneNumber == null; | |||
this.hasObstaclePersonExist = null; |
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.
Bu degisiklige nicin ihtiyacimiz oldu 🤔
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.
yaptığım değişikliklerden sonra test'lerden geçemedi. Null set edilmediği için hata almıştım. Bundan kaynaklı null değeri atadım oraya ve testlerden sorunsuz geçti.
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.
Bu alanla alakali degisiklikleri bu issue ozelinde yapmamiza gerek olmadigi icin, degisiklikleri ve testlerdeki eklemeleri geri alabiliriz
* @param institutionId the ID of the institution to associate with the | ||
* application. |
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.
Burayi eski haline cevirebiliriz
* The emergency evacuation applications are defined in the | ||
* AYS_EMERGENCY_EVACUATION_APPLICATION table in the database. |
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.
Burayi eski haline cevirebiliriz
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.
@agitrubard son requested changes'larına göre düzenledim. Tekrar bu haline mi çevireyim?
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.
Bu javadoc'larin degistirilmesine gerek yok, maindeki haline cevirebiliriz eger @agitrubard icin de uygunsa
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.
Bunları kullandığım formatter dan kaynaklı bozulmuştu. @agitrubard saten bana geri çevirmem için uyardı ama ben bunları gözden kaçırmışım hemen hallediyorum.
@@ -98,7 +99,6 @@ public class EmergencyEvacuationApplicationEntity extends BaseEntity { | |||
@Column(name = "NOTES") | |||
private String notes; | |||
|
|||
|
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.
Burayi eski haline cevirebiliriz
@@ -76,22 +76,24 @@ public class EmergencyEvacuationApplicationRequest { | |||
@Valid | |||
private AysPhoneNumberRequest applicantPhoneNumber; | |||
|
|||
private Boolean hasObstaclePersonExist; |
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.
Bu degisiklige nicin ihtiyacimiz oldu 🤔
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.
testleri build ederken hasObstaclePersonExist olmadığına dair ve bundan kaynaklı hata almıştım. Attribute olarak eklediğimde testlerden geçti.
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.
Testlerin tamami mainde geciyor. Yaptigin baska degisiklikler sonucu bu hatalari almis olmalisin. Bu alan 661 issue'sunun scope'unda olmadigi icin bunlari kaldirip testleri de ona gore duzeltebilir misin?
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.
Tamam dır. Şeyi sormak isterim ben 609 issue'sunu yaparken bu değişiklikleri yapmıştım. Bu değişiklikleri 609 branch'ine alıp tekrar commit atayım mı? Yoksa 609 issue'suna dokunmayayım mı?
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.
Bu degisiklikler baska bir issue'ya aitse onun branch'inde olmasi gerekir, ancak 609 su an icin backlog'da. Ready'de olan issue'lari kendimize assign edip calismaya oyle basliyoruz. Eger ek sorularin olursa jira'da issue uzerinden konusalim beni etiketleyebilirsin
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.
Teşekkürler.
@@ -42,7 +43,6 @@ public class EmergencyEvacuationApplication extends BaseDomainModel { | |||
|
|||
private Institution institution; | |||
|
|||
|
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.
Burayi eski haline cevirebiliriz
@@ -62,7 +62,6 @@ public boolean isInstitutionOwner(final String institutionId) { | |||
return this.institution.getId().equals(institutionId); | |||
} | |||
|
|||
|
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.
Burayi eski haline cevirebiliriz
@@ -13,10 +13,11 @@ | |||
import org.ays.common.util.validation.Name; | |||
import org.hibernate.validator.constraints.Range; | |||
|
|||
|
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.
Burayi eski haline cevirebiliriz
@@ -64,7 +65,6 @@ public class EmergencyEvacuationApplicationRequest { | |||
@Size(min = 2, max = 100) | |||
private String targetDistrict; | |||
|
|||
|
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.
Burayi eski haline cevirebiliriz
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.
Request changes yerine approve'a tiklamisim son eklememi yaparken, duzeltiyorum.
@@ -12,7 +12,6 @@ public class AysPasswordForgotRequest { | |||
|
|||
@EmailAddress | |||
@NotBlank | |||
@Size(min = 2, max = 255) |
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.
Size kontrolu buradan kaldirilmis ancak @EmailAddress
anotasyonu icerisine eklenmemis, o eklemeyi yapabilir miyiz?
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.
Size kontrolünü'de @agitrubard isteği üzerine kaldırdım. Kontrol saten EmailAddress içinde yapılıyormuş. Ekstra bir şey eklememe gerek var mı yine de?
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.
Onceki commitinde hem burada @Size
anotasyonu eklenmis hem de @EmailAddress
anotasyonunun icerisine size kontrolu eklenmisti. Dolayisiyla @Size
anotasyonunu kaldirmak yeterli olacakti ancak sonraki committe ikisi birden kaldirilmis. @EmailAddress
anotasyonunun icerisine tekrar ekleme yapabilir misin?
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.
Tamamdır gözümden kaçmış affola.
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.
Hic onemli degil, yardimci olabildiysem ne mutlu bana 🙌
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.
@emailaddress interface sınıfına hiç dokunmamıştım. Eğer commit mesajımla alakalı bahsetmişseniz ben orada emailAddress attribute'unun üstündeki anatasyon olarak belirtmek istedim. Eğer yanlışım varsa düzeltin çünkü commitlerimin hepsine baktım ama hiç ellememişim interface sınıfını.
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.
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.
Kusuruma bakmayın gözümden kaçmış şimdi düzelttim commit ettim.
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.
Rica ederim eline saglik 🙌🏼
…me classes, revert withSeatingCount method on withValidValues in EmergencyEvacuationRequestBuilder.
if (email.length() < 6 || email.length() > 254) { | ||
return this.buildViolation(constraintValidatorContext, "length must be between 6 and 254 characters"); | ||
} | ||
|
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.
Size kontrolü ilk başta olabilir, çünkü size'dan geçemezse en başta bunu tespit edebiliyor olmamız gerekiyor
@@ -68,6 +69,10 @@ public boolean isValid(String email, ConstraintValidatorContext constraintValida | |||
return this.buildViolation(constraintValidatorContext, "email is not in a valid format"); | |||
} | |||
|
|||
if (email.length() < 6 || email.length() > 254) { |
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.
if (email.length() < 6 || email.length() > 254) { | |
if (email.length() <= 6 || email.length() >= 254) { |
@@ -39,10 +39,8 @@ public class EmergencyEvacuationApplication extends BaseDomainModel { | |||
private Boolean isInPerson; | |||
private Boolean hasObstaclePersonExist; | |||
private String notes; | |||
|
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.
@egehanasal'ın yazdıklarına ek olarak burayı da eski hâline alabiliriz
@@ -76,22 +74,22 @@ public class EmergencyEvacuationApplicationRequest { | |||
@Valid | |||
private AysPhoneNumberRequest applicantPhoneNumber; | |||
|
|||
|
|||
@JsonIgnore |
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.
@egehanasal 'ın söylediklerine ek olarak burayı da eski hâline alabiliriz. Üstünde bir boşluk vardı bu silinmiş
@JsonIgnore | |
@JsonIgnore |
if (StringUtils.isEmpty(this.applicantFirstName) && | ||
StringUtils.isEmpty(this.applicantLastName) && this.applicantPhoneNumber == null) { |
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.
Burayı da eski hâline alabilir miyiz?
if (StringUtils.isEmpty(this.applicantFirstName) && | |
StringUtils.isEmpty(this.applicantLastName) && this.applicantPhoneNumber == null) { | |
if (StringUtils.isEmpty(this.applicantFirstName) && StringUtils.isEmpty(this.applicantLastName) && this.applicantPhoneNumber == null) { |
!(StringUtils.isBlank(this.applicantPhoneNumber.getCountryCode()) && | ||
StringUtils.isBlank(this.applicantPhoneNumber.getLineNumber())); |
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.
Burayı da eski hâline alabilir miyiz?
!(StringUtils.isBlank(this.applicantPhoneNumber.getCountryCode()) && | |
StringUtils.isBlank(this.applicantPhoneNumber.getLineNumber())); | |
!(StringUtils.isBlank(this.applicantPhoneNumber.getCountryCode()) && StringUtils.isBlank(this.applicantPhoneNumber.getLineNumber())); |
if (StringUtils.isEmpty(this.applicantPhoneNumber.getLineNumber()) || | ||
StringUtils.isEmpty(this.phoneNumber.getLineNumber())) { |
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.
Burayı da eski hâline alabilir miyiz?
if (StringUtils.isEmpty(this.applicantPhoneNumber.getLineNumber()) || | |
StringUtils.isEmpty(this.phoneNumber.getLineNumber())) { | |
if (StringUtils.isEmpty(this.applicantPhoneNumber.getLineNumber()) || StringUtils.isEmpty(this.phoneNumber.getLineNumber())) { |
if (this.sourceCity == null || this.sourceDistrict == null || this.targetCity == null | ||
|| this.targetDistrict == null) { |
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.
Bu format düzenlemesi henüz yapılmamış
Assertions.assertNotNull(applicationFromDatabase.get().getUpdatedUser()); | ||
Assertions.assertNotNull(applicationFromDatabase.get().getUpdatedAt()); | ||
Assertions.assertTrue(UUIDTestUtil.isValid(applicationFromDatabase.get().getUpdatedUser())); | ||
} | ||
|
||
} |
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.
Özel bir formatter kullanıyosun sanırım, herkes default IntellIJ formatter'ını kullanıyor bu sebeple burayı eski hâline alabiliriz
.withAddress("Lorem Ipsum is simply dummy text of the printing and typesetting industry."); | ||
.withAddress("Lorem Ipsum is simply dummy text of the printing and typesetting industry.") | ||
.withSeatingCount(1) | ||
.withApplicantPhoneNumber(new AysPhoneNumberRequestBuilder().withValidValues().build()); |
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.
Burayı eski hâline alabilir miyiz?
Checklist
Before submitting your pull request, ensure the following:
Title and Branch Naming Conventions:
standard: Pull Request Naming Conventions.
the Branch Naming Conventions.
Local Testing:
Code Quality:
Documentation:
Testing:
Reviewers and Assignees:
Labels and Associations: