-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Added EmergencyContact entity and CRUD for it #41
Conversation
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/code4ro/next-door/pbg2b3zwm |
return ResponseEntity.ok(savedEmergencyContact); | ||
} | ||
|
||
@DeleteMapping("/id/{id}") |
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.
I think an restful api is without id in path: ex DELETE: /api/emergency-contacts/{id}
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.
will change both the DELETE and GET.
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.
Also, could you add Swagger annotations?
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.
now that it's merged the PR for Swagger, I will ^^
import lombok.Setter; | ||
|
||
@Getter | ||
@Setter |
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.
De ce nu folosesti direct @ Data?
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.
Am urmarit exemplul care era in proiect, ar arata mai bine cu @DaTa intr-adevar
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.
@Data
aduce mai multe. Oare avem nevoie de tot ce aduce? eu cred ca nu
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.
Nu iti aduce decat hashCode, equals si toString
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.
Ceea ce te poate ajuta sa afisez in loguri exact obiectul, poate avem nevoie la un moment dat sa facem equals, mai ales daca vrem sa facem un diff si trebuie sa stim daca sunt egale sau nu. Deci nu vad o problema sa folosim @DaTa
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.
Da, 3 lucruri pe care nu le folosesti. Adica lombok e foarte bun, dar trebuie folosit doar pentru ce ai nevoie, ca sa nu poluezi codul
|
||
private final EmergencyContactRepository emergencyContactRepository; | ||
|
||
public EmergencyContactServiceImpl(EmergencyContactRepository emergencyContactRepository) { |
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.
Sa adaugi anotarea @ Inject
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.
Citind commentul ma gandeam ca oare cum am uitat si totusi mergea..am descoperit asta: https://docs.spring.io/spring-boot/docs/2.0.3.RELEASE/reference/html/using-boot-spring-beans-and-dependency-injection.html
O sa adaug ca in cazul in care se mai pun parametri sa nu se mai modifice.
.collect(Collectors.toList()); | ||
} | ||
|
||
private EmergencyContact mapDtoToEntity(EmergencyContactDto emergencyContactDto) { |
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.
Aici as vedea mai bine niste servicii care sa stie sa face aceste conversii. Pe viitor nu se stie cand mai avem nevoie.
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.
Am lucrat pe un proiect cu mapstruct, stie sa genereze asta singur si ma gandeam sa il adaug :D
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.
Eu lucrez acum la un PR in care am adaugat modelmapper
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.
De mapstruct stiu si eu, e minimal si face treaba rapida si usoara.
.build(); | ||
} | ||
|
||
private EmergencyContactDto mapDtoToDTO(EmergencyContact emergencyContact) { |
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.
La fel ca mai sus
Optional<EmergencyContact> dbEntity = emergencyContactRepository.findById( | ||
UUID.fromString(emergencyContactDto.getId())); | ||
|
||
if (dbEntity.isPresent()) { |
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.
Aici ai putea sa faci direct
dbEntity.map(save).map(convertToDto).orElseGet(() => save(emergencyContactDto))
Optional<EmergencyContact> dbEntity = emergencyContactRepository.findById( | ||
UUID.fromString(id)); | ||
|
||
if (dbEntity.isPresent()) { |
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.
Nu mai ai nevoie de if. Poti face direct
emergencyContactRepository.findById(UUID.fromString(id)).ifPresent(entity -> emergencyContactRepository.deleteById(UUID.fromString(id)))
UUID.fromString(id)); | ||
|
||
if (dbEntity.isPresent()) { | ||
emergencyContactRepository.deleteById(UUID.fromString(id)); |
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.
Aici ai putea sa apelezi direct delete, chiar daca nu exista informatia in baza de date ( in cazul in care nu exista nu se intampla nimic ). Asa faci 2 call-uri in baza de date.
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.
Da, nu se intampla nimic cat timp e un valid UUID. Poate ar fi buna o validare a lui in controller in acest caz.
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.
Dar chiar si daca e invalid nu ar trebui sa se intample nimic. Pentru ca e ‘DELETE FROM table WHERE uuid=“uuidPrimit”’ si daca nu exista nu executa delete
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.
dap, I was overengineering this :))
public EmergencyContactDto findByUUID(String id) { | ||
return emergencyContactRepository.findById(UUID.fromString(id)) | ||
.map(this::mapDtoToDTO) | ||
.orElse(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.
De ce nu returnam direct Optional decat sa introducem NPE-uri?
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.
Good point.
import org.mockito.Mock; | ||
import org.mockito.junit.jupiter.MockitoExtension; | ||
|
||
@ExtendWith(MockitoExtension.class) |
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.
Poti anota atat clasa cat si metodele cu @DisplayName.
Si ar fi ok sa facem @DisplayName("Emergency contact service should) pe clasa si la fiecare metoda @DisplayName cu ce ar trebui sa se intample
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.
sure :D
|
||
List<EmergencyContactDto> result = underTest.findAll(); | ||
|
||
assertEquals(1, result.size()); |
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.
Pentru partea de assert eu as merge cu https://joel-costigliola.github.io/assertj/ mi se pare mai usor de folosit si are multe chestii faine care fac partea de verificare mai usoara
Daca folosit assertj toate asserturile ar fi
asserThat(result).hasSize(1).containsOnly(Collections.singletonList(emergencyContact));
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.
In PR-ul pentru #21, tot pe AssertJ am mers, mi se pare cel mai elegant dintre junit/hamcrest/assertj
Add compose with the db
Add support for groups
private final ModelMapper modelMapper; | ||
|
||
public MapperServiceImpl() { | ||
this.modelMapper = new ModelMapper(); |
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.
} | ||
|
||
private void addCustomMappings() { | ||
modelMapper.createTypeMap(BaseEntity.class, BaseEntityDto.class) |
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.
Toate astea pot fi mutate acolo unde se creeaza ModelMapper.
} | ||
|
||
private void addCustomTypeMaps() { | ||
modelMapper.createTypeMap(Group.class, GroupDto.class) |
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.
si astea la fel
Will open a new PR, I think I rebased wrongly on this one and shows a lot more commits. |
What does it fix?
#31
#30
How has it been tested?
Tested locally all endpoints.
Still have to add unit tests for everything.