Issue 938 Backend: реализация контроллера "Вебинары" в лк #938 #950
Conversation
…inars # Conflicts: # src/main/java/io/hexlet/cv/config/JacksonConfig.java
build.gradle.kts
Outdated
| implementation(libs.jsonunitAssertj) | ||
| implementation(libs.guava) | ||
| implementation("com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.17.2") | ||
| // implementation("com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.17.2") |
There was a problem hiding this comment.
Обычно если что-то не нужно - то это удаляем, не оставляем закомментированные строчки.
Почему это оказалось не нужно?
| @@ -1,3 +1,4 @@ | |||
|
|
|||
| private final Inertia inertia; | ||
| private final FlashPropsService flashPropsService; | ||
| private final UserPageSercive userPageService; | ||
| private final UserPageService userPageService; |
There was a problem hiding this comment.
Почему светится как измененное? Строки на вид идентичны
There was a problem hiding this comment.
тут была опечатка serCive заменено на serVice.
Тут все нормально.
| @Controller | ||
| @RequiredArgsConstructor | ||
| @RequestMapping("/account") | ||
| @PreAuthorize("isAuthenticated()") |
There was a problem hiding this comment.
Это точно нужно? Вроде бы уже сказали в SecurityConfig.java, что нужна авторизация для доступа
There was a problem hiding this comment.
добавил в SecurityConfig.java запрет на /account и убрал PreAuthorize("isAuthenticated()") отсюда.
Сделано!
| var props = service.indexWebinars(userId.get(), pageable); | ||
|
|
||
| var flash = RequestContextUtils.getInputFlashMap(request); | ||
| if (flash != null && !flash.isEmpty()) { |
There was a problem hiding this comment.
Здесь можно использовать библиотеку какую-нибудь, чтобы сразу проверять на null и пустоту
There was a problem hiding this comment.
Не особо вижу смысла менять шило на мыло, но ок.
исправиль на !ObjectUtils.isEmpty(flash)
| public Object registerToWebinar(@PathVariable Long webinarId, | ||
| RedirectAttributes redirectAttributes) { | ||
|
|
||
| var userId = userUtils.currentUserId(); // id юзера который залогинился |
There was a problem hiding this comment.
готово, убрал все подобные коменты!
| public class AccountWebinarsController { | ||
|
|
||
| private final Inertia inertia; | ||
| private final AccountWebinarService service; |
There was a problem hiding this comment.
Сервис лучше назвать accountWebinarService, так более читабельно, ведь сервисов бывает много в контроллере.
| RedirectAttributes redirectAttributes) { | ||
|
|
||
| var userId = userUtils.currentUserId(); // id юзера который залогинился | ||
| service.addWebinarToCalendar(userId.get(), webinarId); |
There was a problem hiding this comment.
Тут предлагаю сразу сделать CalendarService и перенести туда логику работы с календарем
There was a problem hiding this comment.
Перенес логику календаря в CalendarService.
Стоит сделать отдельный issue по контроллеру календаря и убрать работу с календарем в контроллер работы с календарем.
|
|
||
| @Controller | ||
| @RequiredArgsConstructor | ||
| @RequestMapping("/account") |
There was a problem hiding this comment.
Контроллер, судя по названию, про покупки и подписки, почему урл про аккаунт?
There was a problem hiding this comment.
issue было "Покупки и подписки" для личного кабинета "
/account - личный кабинет в нем /purchase - покупки ... подписки и тд.
Считаю верным такой эндпоинт.
Ничего не менял.
| return inertia.render("Admin/Marketing/Index", props); | ||
| } | ||
|
|
||
| /* |
There was a problem hiding this comment.
Не храним в репозитории закомментированные куски кода, удаляем, если не нужно.
|
|
||
| @DeleteMapping("/{id}/delete") | ||
| public Object deleteWebinar(@PathVariable Long id, | ||
| RedirectAttributes redirectAttributes) { |
| import jakarta.persistence.Converter; | ||
|
|
||
| @Converter(autoApply = true) | ||
| public class PurchSubsTypeConverter implements AttributeConverter<StatePurchSubsType, String> { |
There was a problem hiding this comment.
Какой смысл в сокращениях в названии?
There was a problem hiding this comment.
Сократил так как только одно имя на полэкрана. Ну ок.
заменил на PurchaseAndSubscriptionTypeConverter.
| @Getter | ||
| @Setter | ||
| @NoArgsConstructor | ||
| public class PurchSubsDTO { |
There was a problem hiding this comment.
Если у нас в проекте еще нет соглашений, то предлагаю называть SomeDto, а не SomeDTO
There was a problem hiding this comment.
звменил на PurchaseSubscriptionDto
| public class WebinarDTO { | ||
| private Long id; | ||
|
|
||
| @NotBlank |
There was a problem hiding this comment.
Валидация тут нужна потому, что dto приходит с фронта? Такие можно называть не дто, а формами, например. Для остальных полей валидация не нужна?
There was a problem hiding this comment.
заменил на WebinarDto.
dto приходит в фронта, и валидация по названию нужна (чтобы в базе не было просто случайных пустышек), остальная валидация, я считаю, не нужна, так-как это приходит из админки, и можно написать только название будущего вебинара, а потом уже уточнять/заполнять и менять даты проведения и прочие параметры.
| @NotBlank | ||
| private String webinarName; | ||
|
|
||
| @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd HH:mm") |
There was a problem hiding this comment.
Можно настроить формат дат где-то в одном месте, чтобы не писать это в каждой дто с датой
There was a problem hiding this comment.
я предполагаю, что дату-время создания и изменения сущности лучше хранить в полном формате с секундами. А дату-время начала вебинара - дата и часы: минуты. Секунды явно ясно лишние.
Пока оставил без изменений, так-как предполагается общий рефакторинг на эту тему
вот тут ты писала про это
| } | ||
|
|
||
|
|
||
| @ExceptionHandler(WebinarAlreadyExistsException.class) |
There was a problem hiding this comment.
Не стоит делать для каждой ошибки свой класс исключения, так как на фронт мы все равно отдаем только текст. Нужно сделать общее исключение типа CvException и им пользоваться.
There was a problem hiding this comment.
Да, именно так и надо сделать. Но эти исключения уже смержили с main и они используются в разных сервисах. Потому тут надо отдельное issue на рефакторинг исключений.
Пока что оставил без изменений.
| } | ||
|
|
||
|
|
||
| // это просто ошибки все остальное |
There was a problem hiding this comment.
По принципу "оставь код чище, чем он был до тебя" такие комменты предлагаю беспощадно удалять.
There was a problem hiding this comment.
Удалил ненужные комменты.
| @Profile("dev") | ||
| @AllArgsConstructor | ||
| @DependsOn({"userDataInitializer", "webinarDataInitializer"}) | ||
| public class PurchaseAndSubscriptionDataInitializer { |
There was a problem hiding this comment.
Такой способ инициализации бд не подходит, будет отдельная ишью под это.
There was a problem hiding this comment.
убрал все такие инициалайзеры, оставил только UserDataInitializer. При рефакторинге и его уберем.
| public abstract WebinarDTO map(Webinar webinar); | ||
|
|
||
| @Mapping(target = "id", ignore = true) | ||
| public abstract void update(WebinarDTO dto, @MappingTarget Webinar model); |
There was a problem hiding this comment.
Подход, когда с фронта приходит вся сущность и мы слепо обновляем все, кроме id в перспективе не очень удобен. Логика усложняется, начинают появляться развилки типа "а вот если мы обновили вот это поле на вот это, то в этом могут быть только такие значения". Для изменения лучше получать с фронта конкретное поле, которое изменяем, но это надо думать уже совместно с фронт-разработчиками.
There was a problem hiding this comment.
Тогда пока оставил без изменений. Но обновить то можно не только одно поле, а сразу несколько полей.
ок. Надо понять как фронт собирается будет отсылать эти обновления.
|
|
||
| private String description; | ||
|
|
||
| // id вебирана или встречи и тд зависит от eventType |
There was a problem hiding this comment.
Заменил на javadoc хотя можно и удалить такой комментарий, в целом и так понятно
|
|
||
|
|
||
|
|
||
| //@JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd'T'HH:mm:ss.SSSSSS") |
src/main/java/io/hexlet/cv/model/account/PurchaseAndSubscription.java
Outdated
Show resolved
Hide resolved
| private String itemName; // название товара/подписки и тд | ||
|
|
||
| @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd") | ||
| private LocalDate purchasedAt; // наверное это дата с какого числа подписка или дат когда куплено |
There was a problem hiding this comment.
Лучше выяснить точно, что именно означает это поле
There was a problem hiding this comment.
судя по картинке в issue - это дата покупки/подписки
Убрал лишние комментарии, добавил javadoc (хотя модно было его и не писать)
| private String orderNum; // Номер заказа типа #A-1042 | ||
|
|
||
| @NotNull | ||
| private String itemName; // название товара/подписки и тд |
There was a problem hiding this comment.
Очевидные комментарии лучше убрать, не очевидные - в javadoc
There was a problem hiding this comment.
Все поля и так понятны без комментариев, убрал все лишнее
| .map(subs -> subs.getReferenceId()) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| List<Webinar> webinars = webinarRepository.findAllByIdInOrderByWebinarDateAsc(webinarIds); |
There was a problem hiding this comment.
А можем за один запрос получить вебинары?
There was a problem hiding this comment.
заменил на специфический один запрос.
Но получается для каждого типа (Вебинары, Курсы, и тд) надо в репозитории писать свой метод.
В моем старом варианте для какого-то нового типа (к примеру Курсы), в репозиториях ничего не меняется. Добавили новый тип, добавили реппозиторий для этого типа и все. Один запрос находит все id подписки данного типа, второй запрос уже по нужному типу, по id достает сущности.
Если все за один запрос, это конечно быстрее будет, но надо будет для каждой сущности нового типа добавлять в PurchaseAndSubscriptionRepository свой метод.
ок. заменил!
There was a problem hiding this comment.
Добавить метод в репозиторий - не страшно, хуже, когда все тормозит, потому что много лишних запросов в бд.
| var foundWebinar = webinarRepository.findById(webinarId).orElseThrow( | ||
| () -> new ResourceNotFoundException("error.webinar.notFound") | ||
| ); | ||
| var foundUser = userRepository.findById(userId).orElseThrow( |
There was a problem hiding this comment.
Мне кажется не очень чистой история, когда мы с одной стороны предполагаем, что в качестве id user'а получим id залогиненного пользователя, и в то же время вынуждены проверять, а точно ли пользователь с таким id есть в базе. Предлагаю не передавать в метод сервиса userId, а получать его внутри метода. И, соответственно, убрать orElseThrow.
There was a problem hiding this comment.
перенес получение id в сервис, убрал лишние orElseThrow
|
|
||
| var subscription = new PurchaseAndSubscription(); | ||
| subscription.setUser(foundUser); | ||
| subscription.setOrderNum("free"); // без номера вебинар |
There was a problem hiding this comment.
Почему без номера? "free" в константу
There was a problem hiding this comment.
Пересмотрел логику. Теперь зарегистрироваться можно только на вебинар на который есть подписка и статус available. Платный или бесплатный это будет решаться в покупках. При регистрации на вебинар статус меняется на registrated. Все статусы в enum
|
|
||
| subscription.setItemName("Вебинар: " + foundWebinar.getWebinarName()); | ||
| subscription.setPurchasedAt(LocalDate.now()); | ||
| subscription.setAmount(BigDecimal.ZERO); // типа бесплатно |
There was a problem hiding this comment.
Можно тоже вынести в константу и назвать как-то про "бесплатно", либо просто убрать коммент
There was a problem hiding this comment.
Логику переделал. Все константы-статусы в enum
| throw new WebinarAlreadyExistsException("error.webinar.alreadyRegistered"); | ||
| }); | ||
|
|
||
| var subscription = new PurchaseAndSubscription(); |
There was a problem hiding this comment.
весь этот кусок убрал, логику поменял.
| event.setUser(foundUser); | ||
| event.setTitle("Вебинар: " + foundWebinar.getWebinarName()); | ||
| event.setStartAt(foundWebinar.getWebinarDate()); | ||
| //event.setFinishAt(foundWebinar.getWebinarDate()); // задать окончания вебинара ? |
There was a problem hiding this comment.
Не нужно оставлять такие комментарии, стоит сразу разобраться
| LocalDateTime to = null; | ||
|
|
||
| // 2000 ... 2099 | ||
| if (searchString.matches("^20\\d{2}$")) { |
There was a problem hiding this comment.
Расскажи, пожалуйста, подробнее, для чего здесь такая логика поиска. Выглядит сложно.
There was a problem hiding this comment.
тут поиск или по текстовому полю, или по LocalDateTime (только год, год-месяц, год-месяц-день, + еще и время)
ввели "2025-10" будет искать по датам в которых есть 2025-10. Вели "2023" - найдет по дате за этот год. Ввели слово "классы" будет искать это слово в текстовых полях. Готовой библиотеки для поиска и по String и по LocalDateTime одновременно не нашел, написал свой вариант.
| private MockMvc mockMvc; | ||
|
|
||
| @Autowired | ||
| private PurchaseAndSubscriptionRepository subsRepo; |
There was a problem hiding this comment.
не надо сокращать, purchaseAndSubscriptionRepository
There was a problem hiding this comment.
заменил все сокращения
|
|
||
| @AfterEach | ||
| public void cleanUp() { | ||
| eventRepo.deleteAll(); |
There was a problem hiding this comment.
Тесты могут выполняться параллельно, нужно так готовить тестовые данные, чтобы для каждого теста они были изолированы и не нужно было все отовсюду удалять каждый раз.
There was a problem hiding this comment.
если в базе что-то останется - на тесты не влияет, влияет если после тестов останется что то в базе.
Потому тут после выполнения тестов все удаляется.
В начале так же удаляется чтобы избежать каких-то неучтенных накладок, если какие то тесты вдруг (вдруг так совпадет) будут одну и ту же сущность.
| @Test | ||
| void testIndexWebinars() throws Exception { | ||
|
|
||
| var webinar = new Webinar(); |
There was a problem hiding this comment.
- Лучше использовать билдеры и методы для создания, ведь большинство параметров нам не важно. Чтобы в тесте это выглядело как "Создали один вебинар, создали второй, погнали проверять, чтобы это и вот это". По сути в конце теста чекаются только названия вебинаров, значит только их и имеет смысл делать уникальными.
- Нужно разбить тест на секции given, when, then, просто строкой с комментом:
//given
....
...
//when
...
//then
...
| public class AdminWebinarsControllerTest { | ||
|
|
||
| @Autowired | ||
| private MockMvc mockMvc; |
There was a problem hiding this comment.
Можно сделать базовый тестовый класс, вынести в него повторяющиеся поля и логику
There was a problem hiding this comment.
на мой взгляд будет менее читаемо. Тут разные пути проверяются, разные коды ответа. Я не вижу тут повтряющиеся куски чтобы их можно было вынести в базовый класс и от него наследоваться. Ну или будет запутанно. На мой взгляд.
There was a problem hiding this comment.
а! или ты имеешь ввиду сделать базовый класс для всех тестов (не конкретно для этого) и вынести туда все удаления базы, все инжекции зависимостей MockMvc, jwtUtils, passwordEncoder и тд ?
Но если так - то вынести получится не так и много, у всех же репозитории свои. Только если найти похожие, к примеру все про Admin и сделать базовый класс для них. Но как-то не верен что имеет смысл.
часть тестов смержина в main и наверное тогда надо отдельный issue для переделки всего.
| .withUser(testUser) | ||
| .withWebinar(webinar2) | ||
| .build()); | ||
|
|
| var mvcResult = mockMvc.perform(get("/account/webinars") | ||
| .cookie(new Cookie("access_token", candidateToken)) | ||
| .header("X-Inertia", "true")) | ||
| //when |
2 participants
Backend: реализация контроллера "Вебинары" в лк #938
GET /account/webinars?page=X&size=Y
Возвращает в JSON вебинары на которые подписан пользователь который в настоящий момент залогинен.
Поддерживается пагинация, по умолчанию 0 страница и 10 вебинаров на странице.
Пример ответа:
{
"component": "Account/Webinars/Index",
"props": {
"totalItems": 1, <- сколько всего нашлось у пользователя подписок на вебинары
"totalPages": 1, <- сколько всего страницы с вебинарами
"currentPage": 0, <- ккая сейчас страница выводится
"webinars": [ <- массив с найденными вебинарами на которые подписался юзер
{
"id": 1,
"webinarName": "название вебинара",
"webinarDate": "2025-11-21 14:30",
"webinarRegLink": "https://.....",
"webinarRecordLink": "https://....",
"feature": true,
"publicated": false,
"createdAt": [2025, 11, 30, 18, 34, 22, 746662000],
"updatedAt": [2025, 11, 30, 18, 34, 22, 746662000]
}
]
},
"flash" : {"............"}, <- не обязательно, но если были переданы flash сообщения то они тут
"url": "/account/webinars",
"version": "1",
"encryptHistory": false,
"clearHistory": false
}
POST /account/webinars/{webinarId}/registrations
В теле ничего нет.
Регистрирует залогиненого юзера на вебинар, вносит в его "Подписки и Вебинары" вебинар с ID webinarId
Если нет такого вебинара - отдает JSON Flash с атрибутов "error" и значением "error=error.webinar.notFound", если юзер уже подписан на вебинар то "error=error.webinar.alreadyRegistered", и если, что быть не может, залогиненного юзера нет в базе юзеров, то отдает "error=error.user.notFound"
Если все прошло нормально то пишет во flash "success": "webinar.registered.success" и редирект на /account/webinars
POST /account/webinars/{webinarId}/calendar-events
В теле ничего нет.
Добавляет в календарь залогиненого юзера вебинар с ID webinarId
Если нет такого вебинара - отдает JSON Flash с атрибутов "error" и значением "error=error.webinar.notFound", если юзер уже подписан на вебинар то "error=error.webinar.alreadyRegistered", и если, что быть не может, залогиненного юзера нет в базе юзеров, то отдает "error=error.user.notFound"
Если все прошло нормально то пишет во flash "success": "webinar.add.success" и редирект на /account/webinars
ИТОГО:
Регистрация на вебенар, это добавление этого вебенара в подписки как "бесплатного", вероятно потом следует делить все на платное и бесплатное и регистрировать только бесплатные, а платные регистрируются при покупке, к примеру.
Напрашивается отдельно реализовать работу с календарем, и, вероятно, перенести потом "добавление в календарь" отсюда в работу с календарем.
сделал Инициализатор начальных данных для dev.
Все покрыто тестами.