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

Product update product entity #50

Merged

Conversation

UladzislauM
Copy link
Contributor

Updated DB to new table through Liquibase.
Corrected Entity and DTO for new DB table.

@NotBlank(message = "Category is mandatory")
@Size(max = 55, message = "Category length must be less than 55 characters")
private String category;
@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove toString from dto. Its better to apply JSON library for serialization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Этого я просто не знал - сделаю

@Size(max = 55, message = "Name length must be less than 55 characters")
private String name;
@Override
public boolean equals(Object o) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please review. Should we place all this fields for equals and hashcode usage?

return Objects.hash(id, name, description, price, currency, quantity, active);
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

}

@Override
public boolean equals(Object o) {
Copy link
Contributor

Choose a reason for hiding this comment

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

probably doesnt make a sense to check for all fields

Copy link
Contributor

@Reyzis2021 Reyzis2021 left a comment

Choose a reason for hiding this comment

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

Done

import java.util.Objects;
import java.util.UUID;

@Builder
@Document
Copy link
Contributor

Choose a reason for hiding this comment

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

тут по ощущениям нужна аннотация Entity, если мы используем в качестве БД для хранения Postgres, а не Mongo

Copy link
Contributor

Choose a reason for hiding this comment

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

Можем использовать Getter. Setter аннотации что бы сократить количество кода в сущности, а equals , hashcode и toString , я бы предложил оставить как есть во избежании дальнейших конфликтов Lombok с Hibernate

Copy link
Owner

Choose a reason for hiding this comment

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

Аннотация @document тут точно лишняя так как мы используем Postgres

Должна быть аннотация @entity

import java.math.BigDecimal;
import java.util.Objects;
import java.util.UUID;

Copy link
Contributor

Choose a reason for hiding this comment

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

Как предлагал Женя, можем использовать функционал новой java, а именно record , что бы не писать множество ненужного кода

Copy link
Contributor

Choose a reason for hiding this comment

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

Также на рекорды можно будет перенести валидацию

Copy link
Contributor

Choose a reason for hiding this comment

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

Но если все таки будет принято рещение не использовать record, то тут нужно убрать лишний код, геттеры, сеттеры, хеш код и т.д . Поскольку стоит аннотация Data и благодаря ей Lombok создаст все это дело сам

Copy link
Owner

Choose a reason for hiding this comment

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

Как предлагал Женя, можем использовать функционал новой java, а именно record , что бы не писать множество ненужного кода

Согласен, давайте с record

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Здесь 100% record, но я так не делал для того чтобы это добавить в отдельной таске - для атомарности

return "ProductInfo(id=" + this.getId() + ", name=" + this.getName() + ", price=" + this.getPrice() + ", category=" + this.getCategory() + ")";
}
@Id
private UUID id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Тут вот ребята подскажите, есть вариант в самой сущности поле сделать как String. Hibernate умеет конвертировать сразу все это дело, как вариант не нужно будет UUID приводить к String, не знаю насколько это уместно будет

@@ -6,5 +6,6 @@
http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.8.xsd">

Copy link
Contributor

Choose a reason for hiding this comment

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

думаю что мастер change log проще сделать в формате yml, это займет пару строчек места и будет намного понятней в отличии от хмл формата.

Copy link
Owner

Choose a reason for hiding this comment

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

Согласен

@@ -0,0 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

все остальные(не главные) changelogs, предлагаю делать в sql формате, как уже обсуждалось. Так будет проще понимать что за скрипт накатывается, и проверять правильность самих скриптов.

@Size(max = 55, message = "Name length must be less than 55 characters")
private String name;

@NotBlank(message = "Category is mandatory")
Copy link
Owner

Choose a reason for hiding this comment

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

Написанно Category, а должно быть description

private String name;

@NotBlank(message = "Category is mandatory")
@Size(max = 55, message = "Category length must be less than 55 characters")
Copy link
Owner

Choose a reason for hiding this comment

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

Написанно Category, а должно быть description

import java.math.BigDecimal;
import java.util.Objects;
import java.util.UUID;

Copy link
Owner

Choose a reason for hiding this comment

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

Как предлагал Женя, можем использовать функционал новой java, а именно record , что бы не писать множество ненужного кода

Согласен, давайте с record

@@ -6,5 +6,6 @@
http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.8.xsd">

<include file="classpath:db/changelog/db.changelog-1.0.xml" />
<include file="classpath:db/changelog/db.changelog-product-1.0.xml" />
Copy link
Owner

Choose a reason for hiding this comment

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

давайте сделаем формат имени для changelog
например db.changelog-16.07.23.xml

Кажется лучше не добавлять product в названия changelog

@@ -6,5 +6,6 @@
http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.8.xsd">

Copy link
Owner

Choose a reason for hiding this comment

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

Согласен

return "ProductInfo(id=" + this.getId() + ", name=" + this.getName() + ", price=" + this.getPrice() + ", category=" + this.getCategory() + ")";
}
@Id
private UUID id;
Copy link
Owner

Choose a reason for hiding this comment

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

Кажется должно быть

@id
@column(nullable = false)
@GeneratedValue(strategy = GenerationType.SEQUENCE)

@Size(max = 55, message = "Name length must be less than 55 characters")
private String name;
@Override
public boolean equals(Object o) {
Copy link
Owner

Choose a reason for hiding this comment

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

Так как мы будем использовать record
Нам не нужен equals

@NotNull(message = "currency is mandatory")
private String currency;

public UUID getId() {
Copy link
Owner

Choose a reason for hiding this comment

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

Так как мы будем использовать record
Нам не нужен getter и setter

@Size(max = 55, message = "Category length must be less than 55 characters")
private String description;

@Valid
Copy link
Owner

Choose a reason for hiding this comment

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

Эта аннотация здесь не нужна
Удали

@NotNull(message = "Price is mandatory")
private BigDecimal price;

@Valid
Copy link
Owner

Choose a reason for hiding this comment

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

Эта аннотация здесь не нужна
Удали

PRIMARY KEY (id)
);

CREATE TABLE IF NOT EXISTS address
Copy link
Owner

Choose a reason for hiding this comment

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

в файле product.sql должна быть только таблица product
для остальных таблиц создай другие таблицы

databaseChangeLog:
- logicalFilePath: db/changelog/db.changelog-master.yaml
- include:
file: db.changelog-16072023.yaml
Copy link
Owner

Choose a reason for hiding this comment

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

название файла плохо читается

Может лучше:
db.changelog-product-table-17.07.2023.yaml ?

@@ -28,24 +27,18 @@ spring:
datasource:
url: jdbc:postgresql://localhost:5432/testdb
username: postgres
password: postgres
password: root
Copy link
Owner

Choose a reason for hiding this comment

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

зачем ты поменял пароль?

change-log: classpath:db/changelog/db.changelog-master.yaml
url: jdbc:postgresql://localhost:5432/testdb
user: postgres
password: root
Copy link
Owner

Choose a reason for hiding this comment

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

зачем ты поменял пароль?

@@ -111,6 +104,8 @@ jwt:
# Logging Properties
#
logging:
level:
root: DEBUG
Copy link
Owner

Choose a reason for hiding this comment

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

почему DEBUG?

Copy link
Owner

Choose a reason for hiding this comment

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

Зачем ты удалил класс для цены?

Copy link
Owner

Choose a reason for hiding this comment

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

Удали лишние эндпоинты

  • saveProduct
  • deleteById
  • updateProductInfo

В текущей версии MVP-1 данный функционал не поддерживается. В данном API будут другие методы

@@ -11,17 +10,19 @@ public class ProductInfoDtoConverter {
public ProductInfoDto convertToDto(final ProductInfo entity) {
return ProductInfoDto.builder()
.id(entity.getId())
.category(entity.getCategory())
.description(entity.getDescription())
Copy link
Owner

Choose a reason for hiding this comment

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

ты либо используй гетеры entity.getDescription() либо используй возможности record - entity.description()
и так везде единообразно должно быть

.category(dto.getCategory())
.name(dto.getName())
.price(dto.getPrice())
.description(dto.description())
Copy link
Owner

Choose a reason for hiding this comment

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

ты либо используй гетеры entity.getDescription() либо используй возможности record - entity.description()
и так везде единообразно должно быть

pom.xml Outdated
<dependency>
<groupId>org.liquibase</groupId>
<artifactId>liquibase-core</artifactId>
<version>4.23.0</version>
Copy link
Owner

Choose a reason for hiding this comment

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

версии всегда прячь в

UladzislauM and others added 8 commits July 18, 2023 07:00
…_ProductEntity

# Conflicts:
#	pom.xml
#	src/main/java/com/zufar/onlinestore/customer/converter/CustomerDtoConverter.java
#	src/main/java/com/zufar/onlinestore/customer/endpoint/CustomerEndpoint.java
#	src/main/java/com/zufar/onlinestore/customer/entity/Address.java
#	src/main/java/com/zufar/onlinestore/customer/entity/Customer.java
#	src/main/java/com/zufar/onlinestore/notification/endpoint/NotificationEndpoint.java
#	src/main/java/com/zufar/onlinestore/notification/entity/Notification.java
#	src/main/java/com/zufar/onlinestore/notification/repository/NotificationRepository.java
#	src/main/java/com/zufar/onlinestore/product/ProductsSumCalculator.java
#	src/main/java/com/zufar/onlinestore/product/converter/ProductInfoDtoConverter.java
#	src/main/java/com/zufar/onlinestore/product/dto/ProductInfoDto.java
#	src/main/java/com/zufar/onlinestore/product/endpoint/ProductsEndpoint.java
#	src/main/java/com/zufar/onlinestore/product/entity/ProductInfo.java
#	src/main/java/com/zufar/onlinestore/product/repository/ProductInfoRepository.java
#	src/main/java/com/zufar/onlinestore/review/dto/ReviewDto.java
#	src/main/java/com/zufar/onlinestore/review/entity/Review.java
#	src/main/resources/application.yaml
Copy link
Owner

Choose a reason for hiding this comment

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

У тебя опечатка. Не торопись, пожалуйста. Будь внимательней.
Было:

db.changelog.address-and-customer-and-notification-and-product-and-review-tabels-ware-created-18.07.2023

Должно быть:

db.changelog.address-and-customer-and-notification-and-product-and-review-tables-were-created-18.07.2023

Copy link
Owner

Choose a reason for hiding this comment

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

Сделай название файла в том формате что мы в чате договорились

Было:

address-and-customer-and-notification-and-product-and-review.sql

Должно быть:

address-and-customer-and-notification-and-product-and-review-tables-were-created-18.07.2023.sql

- includeAll:
path: /migrations
relativeToChangelogFile: true
# - include:
Copy link
Owner

Choose a reason for hiding this comment

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

никаких комментариев такого рода плиз

pom.xml Outdated
@@ -231,6 +233,18 @@
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-data-jpa</artifactId>
</dependency>

<!-- DataBase dependencies-->
Copy link
Owner

Choose a reason for hiding this comment

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

Пожалуйста, не забывай про пробелы

<!-- Database dependencies -->

@Sunagatov Sunagatov merged commit 2f36c72 into Sunagatov:development Jul 19, 2023
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants