-
Notifications
You must be signed in to change notification settings - Fork 151
Backend: Маркетинг в админ панели #881 #964
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
base: main
Are you sure you want to change the base?
Backend: Маркетинг в админ панели #881 #964
Conversation
|
Я представил такой вариант реализации. Контроллер оставлен как единая точка входа для всех сущностей маркетинга(статьи, истории, отзывы, команда, цены) в соответствии с требованием задачи. Дублирование кода в сервисах преднамеренно - для упрощения текущей реализации. Возможен рефакторинг с созданием базового класса при необходимости. |
| "team", team, | ||
| "pageTitle", "Компоненты главной" | ||
| )); | ||
| Map<String, Object> props = Map.of( |
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.
Предлагаю писать val props в подобных местах, мне кажется, это смотрится лаконичнее. Как думаешь?
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.
Такой вроде легких вопрос, но пришлось задуматься, применять неявную типизацию или нет. Тип данной переменной абсолютно понятен и рука легко убрала громоздкий
Page<ArticleDto> articlesPage = articleService.getAllArticles(pageable)
А вот при объявлении данной переменной
Page<StoryDto> storiesPage = storyService.getAllStories(pageable); ......?!
Для примера (это рассуждение и не несет конструктивной информации)! Qvi scribit bis legit)
Не сразу хотел изменить, так как необходимо выстраивать цепочку для определения типа переменной: экземпляр класса (articleService()) -> в нем вызывается метод getArticles() -> вся логика вертится вокруг статей и ниже по коду мы видим, что используется arteclesPage.getContent() - что подтверждает тип.
А вот во вложенных конструкциях
Map<String, Object> teamProps = new HashMap<>(props);
teamProps.put("positions", TeamPosition.values());
данный фокус не проходит так как происходит копирование коллекции в контексте switch и при создании одно коллекции на основе другой (теряется информация о типе и var в этом случае не лучший выбор и ошибки в компиляторе)))
Итог: чтобы код выглядел чище без потери читаемости использовал var, где это было возможно и не нарушало надежности
| Page<PricingDto> pricingPage = pricingPlanService.getAllPricing(pageable); | ||
| Map<String, Object> props = Map.of( | ||
| "pricing", pricingPage.getContent(), | ||
| "pagination", Map.of( |
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.
Здесь и выше повторяется кусок про pagination, может сделать отдельный класс, который будет получать параметры пагинации и мап с остальными параметрами, например, и из этого класса получать мапу с пропсами. Возможно, у тебя будут другие предложения, как это можно сделать. Суть в том, чтобы убрать дублировать про pagination.
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.
Переделал следующим образом: перенес в отдельную директорию маркетинг контроллер, создал отдельные контроллер для параметров пагинации (не знаю правильно это или нет)
| @@ -35,8 +46,9 @@ | |||
| import org.springframework.web.bind.annotation.ResponseStatus; | |||
|
|
|||
| @Controller | |||
| @Slf4j | |||
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.
Это хорошая идея! Надо внести в правила разработки использование slf4j
|
|
||
| @Getter | ||
| @Setter | ||
| public class ArticleCreateDto { |
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.
Это дто только для создания сущности? Или для редактирования тоже? (на эту мысль наводит поле readingTime)
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.
для создания сущности; поле readingTime можно указать при создании статьи. Автор может установить ориентировочное время при создании статьи, почему бы и нет, а если не указывает в БД будет NULLABLE.
@PostMapping("/articles")
public ResponseEntity<String> createArticle(@Valid @RequestBody ArticleCreateDto createDTO) {
log.debug("[MARKETING] Creating article");
articleService.createArticle(createDTO);
return inertia.redirect("/admin/marketing/articles");
}
т.е. только создание без id
при обновлении
private JsonNullable<Integer> readingTime;
@PutMapping("/articles/{id}")
public ResponseEntity<String> updateArticle(@PathVariable Long id,
@Valid @RequestBody ArticleUpdateDto updateDTO) {
log.debug("[MARKETING] Updating article id: {}", id);
articleService.updateArticle(id, updateDTO);
return inertia.redirect("/admin/marketing/articles");
}
id при обновлении передается отдельно
и ArtileDTO полный объект c id и временем. (отображение и чтение)
почему бы и нет? Смутило поле readingTime? Все поля которые есть в createDto должны быть в updateDto вроде так. В ArticleUpdateDto используется JsonNullable для readingTime - позволяет оставить поле как есть, указать null или указать конкретное число. Ничего не менял. Оставил как есть
| @Getter | ||
| @Setter | ||
| public class ArticleCreateDto { | ||
| private String title; |
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.
Можно для других полей добавить еще валидации типа @NotBlank
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.
Добавил валиацию для полей во все createDto.
|
|
||
| @JsonProperty("description") | ||
| private String description; | ||
| private Double discountAmount; |
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.
Вроде как избыточно. Данные уже проверены. Вариация на выходе не заменит вариацию на входе)) правильно же: create (ввод данных о пользователя) -> update (обновление данных от пользователя) -> pricingDto вывод данных пользователю (данные уже проверены при создании и обновлении)
POST (pricingCreateDto -> валидация -> обновление -> возрат pricingDto) - также PUT (только обновление а не сохранение).
GET Page <- данные уже валидны. А то что это вычисляемые поля -из корректных входных данных получаются корректные выходные. Может и не прав. Оставляю как есть
| String errorMessage = "Invalid JSON format"; | ||
|
|
||
| if (ex.getCause() instanceof InvalidFormatException) { | ||
| InvalidFormatException ife = (InvalidFormatException) ex.getCause(); |
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.
Предлагаю не использовать сокращения. У меня первая ассоциация с именем этой переменной - ifelse))
Тут можно назвать переменную cause, например, норм будет читаться
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.
Исправил!
|
|
||
| @Getter | ||
| public enum TeamMemberType { | ||
| FOUNDER("Основатель"), |
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.
Получается, локализация на бэке? Лучше отдавать name, а на фронте пусть локализуют.
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.
Теперь на бэкенде храним только технические значения("DEVELOPER"), фронт отвечает за локализацию и отображение, в enum убраны все конструкторы с названиями, остались только константы.
|
|
||
| teamMapper.update(updateDTO, team); | ||
|
|
||
| if (updateDTO.getPosition() != null && updateDTO.getPosition().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.
Может вынести как-то в метод проверку на null и present? Использовать лямбду в качестве параметра метода, например, и сам метод куда-то в utils класс
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 (updateDTO.getIsPublished() != null && updateDTO.getIsPublished().isPresent()) {
вынес эту проверку в класс JsonNullableUtils так как эта проверка повторяется в 4 -х местах. Только проверку "если значение есть", она не зависит от бизнес логики и везде одинакова.
Не стал выносить
if (Boolean.TRUE.equals(newStatus) && team.getPublishedAt() == null) {
team.setPublishedAt(LocalDateTime.now(clock));
} else if (Boolean.FALSE.equals(newStatus)) {
team.setPublishedAt(null);
}
так как каждая сущность имеет свои сеттеры, сейчас везде одинаково, но в будущем может измениться(понадобиться дополнительная проверка - в одном из или отправка уведовлений при публикации - в другом из). JsonNullableUtils отвечает только за работу JsonNullable Сервисы за бизнес логику(принцип единства ответственности).
В отдельный метод класса JsonNulableUtils вынес
if (updateDTO.getPosition() != null && updateDTO.getPosition().isPresent()) {
| testPricingPlan.setOriginalPrice(100.0); | ||
| testPricingPlan.setDiscountPercent(10.0); | ||
| pricingPlanRepository.save(testPricingPlan); | ||
| testPricingPlan = PricingPlan.builder() |
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.
то, что выше - создание user'ов, тоже можно причесать - создавать через билдер, вынести в отдельный метод создания user'а, который принимает email и роль, например
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.
Исправил во всех тестовых классах.
| .discountPercent(10.0) | ||
| .build(); | ||
| testPricingPlan.calculateFinalPrice(); | ||
| testPricingPlan = pricingPlanRepository.save(testPricingPlan); |
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.
Правильно ли я понимаю, что testPricingPlan используется не в каждом тесте? Не стоит хранить созданные сущности в полях класса с тестами, каждый тест внутри себя создает все, что ему нужно. Тест делим на секции комментами
//given
//when
///then
Когда создаем перед каждым тестом все сразу, нет наглядности, что именно нужно каждому тесту. Каждый тест должен быть автономен, не зависим от других. А сейчас получается, что теоретически возможно, что я в каком-то тесте изменю какие-то поля testPricingPlan, а другие могут попадать, если проверяют значения полей.
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.
Исправил
Обзор раздела "Маркетинг"
Раздел "Маркетинг" предоставляет администраторам комплексные инструменты для управления контентом веб-сайта, включая статьи, истории, отзывы, информацию о команде и тарифные планы. Доступен только пользователям с ролью администратора (ADMIN).
Сущности базы данных:
Управление контентом
Управление отображением на главной странице
Тарифные планы и ценообразование
Inertia Endpoints
Основные разделы:
Операции CRUD:
Специальные операции:
Структура данных
Статья (ArticleDTO)
Член команды (TeamDTO)
Тарифный план (PricingDTO)
Frontend интеграция
Типизированные enum для консистентности данных:
Автоматизация: