Dzhus/organization(provider) update #53
Conversation
VyacheslavDzhus
requested review from
dmitrykiev,
DmyMi,
Bogdan-Hasanov and
mmmpolishchuk
| [TestFixture] | ||
| public class Tests | ||
| { | ||
| private readonly ILogger<ProviderController> _logger; |
…Provider'" This reverts commit ae05cbb.
| { | ||
| bool IsUnique(Organization entity); | ||
| bool IsNotUnique(Provider entity); |
There was a problem hiding this comment.
the old name 'IsUnique' was better
the rule is to not create negating names
imagine how it will look like '!IsNotUnique' how hard it would be to figure out what is the the result of the operation
| @@ -0,0 +1,26 @@ | |||
| using Microsoft.Extensions.Logging; | |||
There was a problem hiding this comment.
looks like we have 2 files ProviderControllerTest.cs
could you check which one is not needed?
| @@ -42,7 +64,8 @@ public void ConfigureServices(IServiceCollection services) | |||
| }); | |||
|
|
|||
| services.AddCors(confg => | |||
| confg.AddPolicy("AllowAll", | |||
| confg.AddPolicy( | |||
There was a problem hiding this comment.
because of the StyleCope
|
|
||
| // Register the Swagger generator, defining 1 or more Swagger documents | ||
| services.AddSwaggerGen(); | ||
|
|
||
| services.AddAutoMapper(typeof(Startup)); | ||
| } | ||
|
|
||
| // This method gets called by the runtime. Use this method to configure the HTTP request pipeline. | ||
| public void Configure(IApplicationBuilder app, IWebHostEnvironment env) |
There was a problem hiding this comment.
It is not removed, i put this method over ConfigureServices, because of the StyleCope
| } | ||
| catch (ArgumentNullException ex) | ||
| { | ||
| throw new ArgumentNullException(nameof(id), ex.Message); |
There was a problem hiding this comment.
we throw the same exception and we loose stacktrace here
|
|
||
| return provider.ToModel(); | ||
| } | ||
| catch (Exception ex) |
| public async Task<ProviderDto> GetById(long id) | ||
| { | ||
| var provider = await repository.GetById(id).ConfigureAwait(false); | ||
| if (provider == null) |
|
|
||
| if (repository.Exists(provider)) | ||
| { | ||
| throw new ArgumentException("There is already an providerDto with such data"); |
There was a problem hiding this comment.
I believe we should not write 'providerDto' here
it is more clear for the user to see just 'provider'
| { | ||
| if (dto == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(dto), "Provider was null."); |
There was a problem hiding this comment.
Provider was null. -> Provider is null.
| @@ -19,7 +16,8 @@ public class WorkshopDTO | |||
|
|
|||
| [DataType(DataType.PhoneNumber)] | |||
| [Required(ErrorMessage = "Phone number is required")] | |||
| [RegularExpression(@"([\d]{9})", | |||
| [RegularExpression( | |||
There was a problem hiding this comment.
because of the StyleCope
| [MinLength(1)] | ||
| public string Title { get; set; } | ||
|
|
||
| [Required(ErrorMessage = "Title is required")] |
| public string Website { get; set; } = string.Empty; | ||
|
|
||
| [DataType(DataType.Url)] | ||
| [MaxLength(30)] |
There was a problem hiding this comment.
#54 Max 30 characters
| return BadRequest(ModelState); | ||
| } | ||
|
|
||
| return Ok(await providerService.Update(providerDTO).ConfigureAwait(false)); |
There was a problem hiding this comment.
please return CreatedAtAction as Mykhailo suggested
There was a problem hiding this comment.
He said to use CreatedAtAction with Post method, here we have the Put method
|
|
||
| if (providerDTO == null) | ||
| { | ||
| return BadRequest(providerDTO); |
There was a problem hiding this comment.
does any error message needed here?
There was a problem hiding this comment.
will user understand what went wrong in this case?
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="ProviderController"/> class. | ||
| /// </summary> | ||
| /// <param name="logger">Logging mechanism.</param> |
There was a problem hiding this comment.
maybe "Logger instance" sounds better
|
|
||
| [Required(ErrorMessage = "Title is required")] | ||
| [DataType(DataType.Text)] | ||
| public string Title { get; set; } |
There was a problem hiding this comment.
Shouldn't it be like this: public string Title { get; set; } = string.Empty ?
There was a problem hiding this comment.
We decided not to use this for properties with attribute Required
| @@ -59,7 +59,7 @@ public async Task<ActionResult> GetWorkshopById(long id) | |||
| /// </summary> | |||
| /// <param name="workshopDto">Entity to add.</param> | |||
| /// <returns>A <see cref="Task{TResult}"/> representing the result of the asynchronous operation.</returns> | |||
| [Authorize(Roles = "organization,admin")] | |||
| [Authorize(Roles = "provider,admin")] | |||
| [HttpPost] | |||
| public async Task<ActionResult> CreateWorkshop(WorkshopDTO workshopDto) | |||
There was a problem hiding this comment.
Let's stick to the rule: actions from our controllers should return IActionResult
|
SonarCloud Quality Gate failed. |
No description provided.