#127 Хранение технологических сообщений #4

Merged
on.nemtina merged 14 commits from TechMessages into master 2024-12-04 16:51:51 +05:00
Collaborator
No description provided.
rs.efremov added 6 commits 2024-11-26 14:21:25 +05:00
rs.efremov added 2 commits 2024-11-27 15:22:46 +05:00
rs.efremov added 1 commit 2024-11-28 08:55:57 +05:00
rs.efremov added 1 commit 2024-11-28 13:13:16 +05:00
on.nemtina requested changes 2024-11-28 17:47:06 +05:00
@ -6,1 +6,4 @@
namespace Persistence.API.Controllers;
/// <summary>
/// <20>אבמעא ס גנולוםם<D79D>לט האםם<D79D>לט
Owner

Кодировка

Кодировка
@ -45,4 +79,0 @@
public async Task<ActionResult<int>> Save(Guid setpointKey, object newValue, CancellationToken token)
{
// ToDo: вычитка idUser
await setpointRepository.Save(setpointKey, newValue, 0, token);
Owner

Здесь userId вместо 0 можно уже использовать

Здесь userId вместо 0 можно уже использовать
Author
Collaborator

iduser вынесен в параметры запроса

iduser вынесен в параметры запроса
@ -35,0 +56,4 @@
/// <param name="token"></param>
/// <returns></returns>
[HttpGet("log")]
public async Task<ActionResult<Dictionary<Guid, IEnumerable<SetpointLogDto>>>> GetLog([FromQuery] IEnumerable<Guid> setpointKeys, CancellationToken token)
Owner

Лучше писать так. Так читабельнее будет выглядеть. Я у себя тоже этот момент поправлю.

Лучше писать так. Так читабельнее будет выглядеть. Я у себя тоже этот момент поправлю.
@ -43,0 +73,4 @@
[HttpPost]
public async Task<ActionResult<int>> Save(Guid setpointKey, object newValue, CancellationToken token)
{
// ToDo: вычитка idUser
Owner

var userId = User.GetUserId();

var userId = User.GetUserId<Guid>();
Author
Collaborator

Вынесено в параметр

Вынесено в параметр
@ -0,0 +42,4 @@
/// <param name="token"></param>
/// <returns></returns>
[HttpGet("statistics/{autoDrillingSystem}")]
public async Task<ActionResult<int>> GetStatistics([FromRoute] string? autoDrillingSystem, int? importantId, CancellationToken token)
Owner

Возвращаемый тип Dictionary<string, int>

Возвращаемый тип Dictionary<string, int>
@ -0,0 +44,4 @@
[HttpGet("statistics/{autoDrillingSystem}")]
public async Task<ActionResult<int>> GetStatistics([FromRoute] string? autoDrillingSystem, int? importantId, CancellationToken token)
{
var result = await techMessagesRepository.GetStatistics(importantId, autoDrillingSystem, token);
Owner

Лучше все переменные и свойства importantId переименовать на categoryId или на categoryImportantId. Нужно, чтобы слово "категория" была, а то не очень понятно.

Лучше все переменные и свойства importantId переименовать на categoryId или на categoryImportantId. Нужно, чтобы слово "категория" была, а то не очень понятно.
@ -0,0 +71,4 @@
[HttpPost]
public async Task<ActionResult<int>> InsertRange([FromBody] IEnumerable<TechMessageDto> dtos, CancellationToken token)
{
var result = await techMessagesRepository.InsertRange(dtos, token);
Owner

Здесь нужно вычислить пользователя, чтобы передать ключ пользователя далее внутрь метода InsertRange репозитория (для вставки в базу, так как у TechMessage есть UserId)

Здесь нужно вычислить пользователя, чтобы передать ключ пользователя далее внутрь метода InsertRange репозитория (для вставки в базу, так как у TechMessage есть UserId)
@ -0,0 +83,4 @@
[HttpGet("categories")]
public ActionResult<Dictionary<int, string>> GetImportantCategories()
{
var result = new Dictionary<int, string>()
Owner

Желательно вынести в константу из метода контроллера.
Желательно эту константу хранить где-то на уровне репозитория (спросить у Никиты)

Желательно вынести в константу из метода контроллера. Желательно эту константу хранить где-то на уровне репозитория (спросить у Никиты)
@ -19,2 +20,3 @@
[HttpGet]
/// <summary>
/// Получить список объектов, удовлетворяющий диапазону дат
Owner

Тут тоже кодировка полетела

Тут тоже кодировка полетела
@ -29,3 +40,4 @@
[HttpGet("datesRange")]
public async Task<IActionResult> GetDatesRange(CancellationToken token)
{
var result = await this.timeSeriesDataRepository.GetDatesRange(token);
Owner

можно без this

можно без this
@ -0,0 +21,4 @@
Task<IApiResponse<IEnumerable<string>>> GetSystems(CancellationToken token);
[Get($"{BaseRoute}/statistics/" + "{autoDrillingSystem}")]
Task<IApiResponse<int>> GetStatistics(string? autoDrillingSystem, int? importantId, CancellationToken token);
Owner

Здесь тоже нужно поправить тип возвращаемых данных
$"{BaseRoute}/statistics/" + "{autoDrillingSystem}" -> можно написать слитно, а то так хуже читается

Здесь тоже нужно поправить тип возвращаемых данных $"{BaseRoute}/statistics/" + "{autoDrillingSystem}" -> можно написать слитно, а то так хуже читается
Author
Collaborator

не актуально, autoDrillingSystem ныне приходит не с роута

не актуально, autoDrillingSystem ныне приходит не с роута
@ -0,0 +159,4 @@
var imortantId = 0;
var autoDrillingSystem = nameof(TechMessageDto.System);
var dtos = await InsertRange();
var filteredDtos = dtos.Where(e => e.CategoryId == imortantId && e.System == e.System);
Owner

Не очень понятно, что такое e.System == e.System, оно же всегда будет true давать?

Не очень понятно, что такое e.System == e.System, оно же всегда будет true давать?
@ -0,0 +30,4 @@
.SortBy(request.SortSettings)
.Skip(request.Skip)
.Take(request.Take)
.ToListAsync();
Owner

По код-стандарту договорились, что материализация будет происходить в массив. Поэтому лучше ToArrayAsync(token). Должен быть еще токен отмены.

Плюс желательно сортировку по умолчанию иметь, если вдруг request.SortSettings будет пустым

По код-стандарту договорились, что материализация будет происходить в массив. Поэтому лучше ToArrayAsync(token). Должен быть еще токен отмены. Плюс желательно сортировку по умолчанию иметь, если вдруг request.SortSettings будет пустым
@ -0,0 +35,4 @@
{
Skip = request.Skip,
Take = request.Take,
Count = entities.Count,
Owner

Count должен рассчитываться до Skip и Take, - то есть Count показывает сколько всего элементов существует

Count должен рассчитываться до Skip и Take, - то есть Count показывает сколько всего элементов существует
@ -0,0 +45,4 @@
public async Task<Dictionary<string, int>> GetStatistics(int? importantId, string? autoDrillingSystem, CancellationToken token)
{
var query = GetQueryReadOnly();
var count = await query
Owner

Название переменной желательно поправить. Можно назвать result.

Название переменной желательно поправить. Можно назвать result.
@ -0,0 +47,4 @@
var query = GetQueryReadOnly();
var count = await query
.Where(e => importantId == null || e.CategoryId == importantId)
.Where(e => autoDrillingSystem == null || e.System.Name == autoDrillingSystem)
Owner

Вообще, думаю, что во всех местах, где есть поиск или фильтрация по имени, необходимо обе сравниваемые переменные приводить к одному регистру и убирать пробелы.

Вообще, думаю, что во всех местах, где есть поиск или фильтрация по имени, необходимо обе сравниваемые переменные приводить к одному регистру и убирать пробелы.
@ -0,0 +56,4 @@
public async Task<IEnumerable<string>> GetSystems(CancellationToken token)
{
var entities = await GetSystems();
Owner

Нужно токен передать внутрь приватного метода

Нужно токен передать внутрь приватного метода
@ -0,0 +59,4 @@
var entities = await GetSystems();
var systems = entities.Select(e => e.Name);
return systems ?? [];
Owner

Здесь можно не писать ?? [], по идее в systems не может быть null, там может быть только пустая коллекция

Здесь можно не писать ```?? []```, по идее в systems не может быть null, там может быть только пустая коллекция
@ -0,0 +69,4 @@
foreach (var dto in dtos)
{
var entity = dto.Adapt<TechMessage>();
var systems = await GetSystems();
Owner

Здесь тоже нужно токен передать

Здесь тоже нужно токен передать
@ -0,0 +70,4 @@
{
var entity = dto.Adapt<TechMessage>();
var systems = await GetSystems();
var systemId = systems.FirstOrDefault(e => e.Name == dto.System)?.SystemId
Owner

Здесь точно e.Name == dto.System необходимо приводить к одному регистру и убирать пробелы.
Пользователь может передать dto.System, например, с пробелом. Тогда поиск по системам не найдет необходимого элемента и создаст новый с таким же названием и с пробелом.

Также при создании системы CreateSystem лучше предварительно убирать пробелы.

Здесь точно e.Name == dto.System необходимо приводить к одному регистру и убирать пробелы. Пользователь может передать dto.System, например, с пробелом. Тогда поиск по системам не найдет необходимого элемента и создаст новый с таким же названием и с пробелом. Также при создании системы CreateSystem лучше предварительно убирать пробелы.
@ -0,0 +88,4 @@
{
var systems = await memoryCache.GetOrCreateAsync(SystemCacheKey, async f =>
{
f.AbsoluteExpirationRelativeToNow = TimeSpan.FromMinutes(60);
Owner

60 минут - лучше в отдельное поле вынести

60 минут - лучше в отдельное поле вынести
@ -0,0 +91,4 @@
f.AbsoluteExpirationRelativeToNow = TimeSpan.FromMinutes(60);
var query = db.Set<ADSystem>();
var entities = await query.ToListAsync();
Owner

ToArrayAsync и туда внутрь еще токен передать....

ToArrayAsync и туда внутрь еще токен передать....
@ -0,0 +97,4 @@
return dtos;
});
return systems ?? [];
Owner

Здесь тоже можно убрать ?? [], так как внутри systems не может быть null.
Чтобы гарантированно это обозначить, можно написать так: return systems!

Здесь тоже можно убрать ``` ?? []```, так как внутри systems не может быть null. Чтобы гарантированно это обозначить, можно написать так: return systems!
@ -0,0 +103,4 @@
{
memoryCache.Remove(SystemCacheKey);
var systemId = Guid.NewGuid();
Owner

Здесь можно дополнительную переменную не создавать

Здесь можно дополнительную переменную не создавать
ng.frolov requested changes 2024-12-02 09:49:28 +05:00
@ -0,0 +3,4 @@
/// <summary>
/// Модель системы автобурения
/// </summary>
public class ADSystemDto
Collaborator

Сокращения не желательны.
Предлагаю название DrillingSystem (без auto, так как системы необязательно должны быть автоматизированые)

Сокращения не желательны. Предлагаю название DrillingSystem (без auto, так как системы необязательно должны быть автоматизированые)
@ -0,0 +38,4 @@
/// <param name="autoDrillingSystem">Система автобурения</param>
/// <param name="token"></param>
/// <returns></returns>
Task<Dictionary<string, int>> GetStatistics(int? importantId, string? autoDrillingSystem, CancellationToken token);
Collaborator

Аргументы лучше местами поменять, чтобы их порядок быть от более крупной сущности к более мелкой (Плюс тогда сигнатура будет больше похожа на сигнатуру веб-апи).

В запросе от пользователя также лучше сразу предусмотреть возможность множественного выбора систем и категорий. Прим.: IEnumerable<int>? CategoriesIds

Возвращаемый Dictionary<string, int> не особо понятно, что именно возвращает.
Предлагаю возвращать:

[
	{ 
		"system": "System_1",
		"categories": [
			{"1": 1}, // Аварии
			{"2": 5}, // Предупреждения
			{"3": 15},
			...
		]
	},
	...
]
Аргументы лучше местами поменять, чтобы их порядок быть от более крупной сущности к более мелкой (Плюс тогда сигнатура будет больше похожа на сигнатуру веб-апи). В запросе от пользователя также лучше сразу предусмотреть возможность множественного выбора систем и категорий. Прим.: `IEnumerable<int>? CategoriesIds` Возвращаемый Dictionary<string, int> не особо понятно, что именно возвращает. Предлагаю возвращать: ``` [ { "system": "System_1", "categories": [ {"1": 1}, // Аварии {"2": 5}, // Предупреждения {"3": 15}, ... ] }, ... ] ```
rs.efremov added 1 commit 2024-12-02 15:14:04 +05:00
on.nemtina requested changes 2024-12-03 15:41:05 +05:00
@ -52,0 +106,4 @@
{
await setpointRepository.Save(setpointKey, newValue, idUser, token);
return Ok();
Owner

Вроде бы договорились CreatedAtAction возвращать?
А еще метод репозитория setpointRepository.Save ничего не возвращает, хотя в других подлобных методах возвращается int.

Вот, например:

var result = await techMessagesRepository.InsertRange(dtos, token);
return CreatedAtAction(nameof(InsertRange), result);

Еще название самого метода Save путает. По коду видно, что это именно вставка, а не просто сохранение...Но, может, так надо...

Вроде бы договорились CreatedAtAction возвращать? А еще метод репозитория setpointRepository.Save ничего не возвращает, хотя в других подлобных методах возвращается int. Вот, например: var result = await techMessagesRepository.InsertRange(dtos, token); return CreatedAtAction(nameof(InsertRange), result); Еще название самого метода Save путает. По коду видно, что это именно вставка, а не просто сохранение...Но, может, так надо...
Author
Collaborator
  1. setpointRepository.Save ничего не возвращает т.к. количество сохраненных записей всегда будет == 1

  2. Для единообразия можно сделать "Add" "AddRange"

1) setpointRepository.Save ничего не возвращает т.к. количество сохраненных записей всегда будет == 1 2) Для единообразия можно сделать "Add" "AddRange"
@ -134,0 +144,4 @@
//assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
Assert.NotNull(response.Content);
Assert.Equal(DateTimeOffset.MinValue, response.Content?.From);
Owner

Тут по идее на null уже можно не проверять response.Content?.From, потому что выше есть проверка Assert.NotNull(response.Content). Если студия ругается, то можно так написать: response.Content!.From
А еще, думаю, нужно в качестве теста заинсертить туда несколько записей, и ,зная, какие у них максимальные и минимальные даты - сверять их с тем, что вернет GetDatesRangeAsyncю.

Ниже приложила скрин подобного тестового метода, только в ChangeLogTestController

Тут по идее на null уже можно не проверять response.Content?.From, потому что выше есть проверка Assert.NotNull(response.Content). Если студия ругается, то можно так написать: response.Content!.From А еще, думаю, нужно в качестве теста заинсертить туда несколько записей, и ,зная, какие у них максимальные и минимальные даты - сверять их с тем, что вернет GetDatesRangeAsyncю. Ниже приложила скрин подобного тестового метода, только в ChangeLogTestController
Author
Collaborator

Сверять максимальные и минимальные даты необъодимо будет в методе GetDatesRange_AfterSave_returns_success

Сверять максимальные и минимальные даты необъодимо будет в методе GetDatesRange_AfterSave_returns_success
@ -134,0 +161,4 @@
//assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
Assert.NotNull(response.Content);
Assert.NotNull(response.Content?.From);
Owner

Assert.NotNull(response.Content?.From);
Тут также выше уже была проверка на null: Assert.NotNull(response.Content);
Поэтому ниже можно не писать знак вопроса: response.Content?.From.
А если не писать знак вопроса, то response.Content.From тоже не нужно проверять на null,потому что From не nullable.
Тут тоже желательно сравнить From и To с каким-то конкретными значениями, которые ты ожиадешь

Assert.NotNull(response.Content?.From); Тут также выше уже была проверка на null: Assert.NotNull(response.Content); Поэтому ниже можно не писать знак вопроса: response.Content?.From. А если не писать знак вопроса, то response.Content.From тоже не нужно проверять на null,потому что From не nullable. Тут тоже желательно сравнить From и To с каким-то конкретными значениями, которые ты ожиадешь
@ -0,0 +144,4 @@
public async Task GetStatistics_returns_success()
{
//arrange
memoryCache.Remove(SystemCacheKey);
Owner

Для тестов, которые что-то возвращают, необходимо предварительно записывать данные. И сравнивать уже получаемые данные с теми, что туда записывались.
Например, в данном методе нужно было предварительно что-то записать в базу, на основании чего бы рассчиталась статистика, а потом этот расчет уже сравнить с тем, что ты ожидаешь получить.

Сейчас здесь просто проверка на то, что метод вернул 200 ответ и что пришла пустая коллекция. Это верно, потому что в базе ничего нет. Но не факт, что тест бы прошел, если бы в базе что-то было.

Для тестов, которые что-то возвращают, необходимо предварительно записывать данные. И сравнивать уже получаемые данные с теми, что туда записывались. Например, в данном методе нужно было предварительно что-то записать в базу, на основании чего бы рассчиталась статистика, а потом этот расчет уже сравнить с тем, что ты ожидаешь получить. Сейчас здесь просто проверка на то, что метод вернул 200 ответ и что пришла пустая коллекция. Это верно, потому что в базе ничего нет. Но не факт, что тест бы прошел, если бы в базе что-то было.
Author
Collaborator

У каждого returns_success теста есть аналог с AfterSave - в котором, как раз таки, проверяются сохраненные данные. Так сделано для тестирования работоспособности отдельных методов вне зависимости от метода сохранения.

У каждого returns_success теста есть аналог с AfterSave - в котором, как раз таки, проверяются сохраненные данные. Так сделано для тестирования работоспособности отдельных методов вне зависимости от метода сохранения.
@ -0,0 +185,4 @@
public async Task GetDatesRange_returns_success()
{
//act
var response = await techMessagesClient.GetDatesRangeAsync(new CancellationToken());
Owner

Перед проверкой необходимо в базу что-то записывать и уже потом проверять сам метод контроллера.

Сейчас же здесь просто проверка на то, что метод вернул 200 ответ и что пришла пустая коллекция. Это верно, потому что в базе ничего нет. Но не факт, что тест бы прошел, если бы в базе что-то было.

Перед проверкой необходимо в базу что-то записывать и уже потом проверять сам метод контроллера. Сейчас же здесь просто проверка на то, что метод вернул 200 ответ и что пришла пустая коллекция. Это верно, потому что в базе ничего нет. Но не факт, что тест бы прошел, если бы в базе что-то было.
Author
Collaborator

У каждого returns_success теста есть аналог с AfterSave - в котором, как раз таки, проверяются сохраненные данные. Так сделано для тестирования работоспособности отдельных методов вне зависимости от метода сохранения.

У каждого returns_success теста есть аналог с AfterSave - в котором, как раз таки, проверяются сохраненные данные. Так сделано для тестирования работоспособности отдельных методов вне зависимости от метода сохранения.
@ -46,0 +47,4 @@
{
var query = GetQueryReadOnly();
var entities = await query
.Where(e => e.Created > dateBegin)
Owner

Тут нужно у Никиты спросить, строго или не строгое неравенство нужно. Я почему-то думала, что нужно >= писать....Но лучше у Никиты спросить)

Тут нужно у Никиты спросить, строго или не строгое неравенство нужно. Я почему-то думала, что нужно >= писать....Но лучше у Никиты спросить)
Collaborator

Чтобы такие вопросы не возникали мы называем переменные для фильтрации:

  • Gt* (greater-than ">")
  • Lt* (less-than "<")
  • Ge* (greater-than or equals "≥")
  • Lt* (less-than or equals "≤")

Конкретно в этом случае нужен ≥

Чтобы такие вопросы не возникали мы называем переменные для фильтрации: - Gt* (greater-than ">") - Lt* (less-than "<") - Ge* (greater-than or equals "≥") - Lt* (less-than or equals "≤") Конкретно в этом случае нужен ≥
rs.efremov added 2 commits 2024-12-04 14:44:40 +05:00
rs.efremov added 1 commit 2024-12-04 16:29:31 +05:00
on.nemtina merged commit 6f93c331a2 into master 2024-12-04 16:51:51 +05:00
on.nemtina deleted branch TechMessages 2024-12-04 16:51:51 +05:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: on.nemtina/persistence#4
No description provided.