bihanviranga / helpdesk Goto Github PK

• It should be a string field for the assigner's username.
• It should be nullable.

(I'll fix this issue later)

It should return the newly created category.

Instead it returns this:

When we send a request to API, and the request does not contain a token, it returns some big error about a Null Pointer.

If the user is not authenticated, it should simply return a 401 Unauthorized response.

I think some fields are marked as NOT NULL when they aren't necessary. And some necessary fields are marked NULL. Should go over the tables.

Can refactor how exceptions are handled in the Controllers. We Can use a Middleware which will handle the exceptions globally. So we can get rid of try, catch statements in every method.

Please refer this if you're interested.

Problem

When creating new objects (tickets, companies, anything) and we forget some required fields, it returns a 500 Internal Server Error because validation is not done.

Expected Behaviour

This is not a server error. It is a user error and therefore should return a 400 Bad Request or something.

Steps to reproduce the problem

• Make a POST request to /ticket endpoint without a required field, such as CompanyId.
• It will return 500 Internal Server Error

Suggestion

For the fields that cannot be null in the database, add a [Required] annotation in the Create DTO. Then we can do a ModelState.IsValid check in the controller and return BadRequest if false.

Applies to

All Create DTOs.

It’s a clean code practice to write to failing part of the code first to make the code cleaner and have less indent

                if (userRole == "Client")
                    return StatusCode(401, "401 Unauthorized  Access");

                if (userRole == "Manager")
                {
                    var modules = await _repository.Module.GetModuleByCondition(userType, userCompanyId);
                    foreach (var module in modules)
                    {
                        var TempModuleDto = _mapper.Map<ModuleDto>(module);
                        if (module.CompanyId != null)
                        {
                            var company = await _repository.Company.GetCompanyById(new Guid(module.CompanyId));
                            if (company != null)
                            {
                                TempModuleDto.CompanyName = company.CompanyName;
                                ModuleList.Add(TempModuleDto);
                            }
                        }
                    }
                    return Ok(ModuleList);
                }
               
                else
                {
                    return StatusCode(500, "Something went wrong");
                }

Better use common names with overloads, so it makes developer who using the contract to use the service easily.

Example:

public async Task<IEnumerable<ProductModel>> GetProductsByCompanyId(string id)
public async Task<IEnumerable<ProductModel>> GetProductsByCondition(string userType, string userCompanyId)

After Refactoring:

public async Task<IEnumerable<ProductModel>> GetProductsAsync(string id)
public async Task<IEnumerable<ProductModel>> GetProductsAsync(string userType, string userCompanyId)

If we could use this type of common method names in all the service contracts, it is easy to use the service, rather having different method names

Will do tomorrow.

GetCompaniesByCondition -> GetCompaniesByConditionAsync

It should return the newly created ticket.

Instead it returns this:

TicketCode field is a char(20) field in the SQL script. But the TicketCode generating code in TicketController does not take this into account.
So when we use a longer company name it throws an error.

How to recreate:

• Use Helpdesk Company Ltd (or something with similar length, ~20) as company name.
• Try to create a ticket.

Output:

• SQL throws a truncation error.
  

Attachments get renamed and lose their extension. On some systems they can't be opened after downloading.

Use Proper naming when using bool type.

Use:
public bool IsNotificationRead { get; set; }

Instead of:
public bool NotifRead { get; set; }

For example, in some places we check whether a user type is "HelpDesk". Ideally, we should use these as constants. So we can say something like UserType=UserTypes.HELPDESK

Reduces the possibility of mistakes.

Pushing Application Settings file with all the secret values has a bit of a risk. Rather storing there we can secure all the keys in a secure file.
You can read the following article to get some solutions.

humankode.com/asp-net-core/asp-net-core-configuration-best-practices-for-keeping-secrets-out-of-source-control

Create the Brand controller.

When creating a new Module, it does not return the created module. It returns some other values instead.

Example: