3PillarGlobal-Czechia / interview-app-api

API for Interview App.
MIT License
3 stars 1 forks source link

API: Update difficulty attribute #92

Closed Ali-Kabbadj closed 2 years ago

Ali-Kabbadj commented 2 years ago

Added the necessary validation attributes to the Difficulty property

Summary

Added [Range] and [Required] attributes to the Difficulty properly in : *CreateQuestionRequest

Added [Range] attributes to the Difficulty properly in : *UpdateQuuestionRequest

Added a custume Validation Attribute to the Difficulties properly in : *GetQuestionRequest

Note: For Difficulties properly in GetQuesttionRequest , i added an extension to the ValidationAtribute Class to be able to validate the range of an IEnumerable< int > Difficulties

Types of changes

Notes

I was wondering if the custume validation for IEnumerable Difficulties should maybe return all the questions for the difficulties that were correct, and just ignore the out of range ones, instead of what my implementation does ,which is return an error if one of the values in the IEnumerable is out oof range.

Also , for some reason when updating a question ,if one of the values is not provided in the put request the value is set to null , i was wondering is that was by choice .

If there is anything to change or modify just let me know , thanks in advance!

TomasNiemczyk98 commented 2 years ago

Looking at the reason the pipeline is failing and it's due to a test that is not passing, the test is in End2EndTests/QuestionTests/UpdateQuestionTest, the test basically tries to change the difficulty of a question to some invalid value (previously this was 6), now this test will probably have to be changed a little bit since the validation is now performed on the controller.

[Fact]
public async Task Update_InvalidRequest_ReturnsBadRequest()
{
    var request = new UpdateQuestionRequest
    {
        Difficulty = 6
    };

    var result = await EndpointCall(1, request);

    Assert.Equal(HttpStatusCode.BadRequest, result.StatusCode);
}
Ali-Kabbadj commented 2 years ago

@TomasNiemczyk98 @petrspelos , why did we have to change the range attribute instead of the way the test works , or is that just temporary ?

petrspelos commented 2 years ago

@Ali-Kabbadj you're right, we should do that. I was just a little confused since you didn't link the issue with this PR.

Yeah, let's change the value in the test. ☺️


btw, in the next PR, you can add #75 to the description in order for it to link. πŸ˜‰ or you can do it through the GitHub UI on the right side.

TomasNiemczyk98 commented 2 years ago

@Ali-Kabbadj you were correct to change the range from 1-5 to 0-100 since that is the description of the task. The issue is that this will break our test, so that will need to be fixed (as mentioned above) before we can merge πŸ™‚

Ali-Kabbadj commented 2 years ago

@petrspelos My bad , will do next time ,thanks

Ali-Kabbadj commented 2 years ago

@TomasNiemczyk98 i haven't really worked with tests extensively enough ,but , if i understand correctly the test should try and pass out of range values to difficulty , should the values be random out of range or just one should suffice like 6 in the example of (1-5 ) range

petrspelos commented 2 years ago

@Ali-Kabbadj just one should be enough. You can (if you're willing) add one that is too low and one that is too high. πŸ™‚

Ali-Kabbadj commented 2 years ago

@petrspelos sorry for the caused trouble , really!

i don't know how to read the log of the tests to understand where the problem has accured .

Maybe in UpdateQuestionUseCaseTest in UnitTests? the instance of the QuestionModel has a Difficulty=1 , and it is used in the method Execute_PassValidInput_CallsInvalid() , im not sure it's just a guess, i might have done something wrong again, probably πŸ˜…πŸ˜….

TomasNiemczyk98 commented 2 years ago

@Ali-Kabbadj Hey, the fault is on our side, the workflow logs are pretty useless right now since they do not give any relevant information about what went wrong. I am guessing that the error is related to sonarcloud and it's secret key, but will have to verify tomorrow, I will let you know ASAP.

Ali-Kabbadj commented 2 years ago

Great . Thanks @TomasNiemczyk98 😊

TomasNiemczyk98 commented 2 years ago

Hi @Ali-Kabbadj we have released a fix for the pipeline failing, could you please try merging our main branch into your Update_Difficulty_Attribute? I believe that should fix that.

petrspelos commented 2 years ago

@TomasNiemczyk98 actually @PlesnikJakub will merge this anyways this evening if everything turns out to be okay. It's just that we updated the pipeline while the PR is in progress. The best way forward is to wrap this up ASAP. ☺️


And while I do agree merging the workflow might even resolve this, we only require it for an arbitrary checkbox. πŸ˜…

Ali-Kabbadj commented 2 years ago

@TomasNiemczyk98 Hey , i have done so , hopefully that fixes it

PlesnikJakub commented 2 years ago

Hey @Ali-Kabbadj, thank you for your contribution and I am sorry for the issues with the pipeline. Merging this now πŸš€

For context, it's really as other guys mentioned, since PR is based on fork pipeline doesn't have access to secrets required to execute sonar cloud analysis. We will sort this out ASAP.

Ali-Kabbadj commented 2 years ago

Hey @PlesnikJakub . Thank you , and no problem! sorry as well ! i can't apologize enough , i made some mistakes during the process , hopefully i didn't cause the guys any headaches they've been really insightful and patient with me , thanks again!