Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed the ability to save answer of question when type is Open Answer #3148

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

amoutens
Copy link
Contributor

@amoutens amoutens commented Jan 28, 2025

  • Fixed ability to save Open Answer answers
  • Provided opportunity to add more than one answer for Open Answer
  • Added styles for edit open answer question
  • Added error handling on blur when we input empty new value
  • Provided functional and styles for checking right answer when Open Answer

image
image
image
image
image

@amoutens amoutens marked this pull request as draft January 28, 2025 18:10
@amoutens amoutens self-assigned this Jan 30, 2025
@amoutens amoutens added bug Something isn't working Frontend part Functional Something isn't working labels Jan 30, 2025
@amoutens amoutens marked this pull request as ready for review January 30, 2025 13:23
src/components/question-editor/QuestionEditor.tsx Outdated Show resolved Hide resolved
src/components/question-editor/QuestionEditor.tsx Outdated Show resolved Hide resolved
title: 'Question title',
text: 'Question text',
answers: [
{id: 0, text: 'Answer 1', isCorrect: true },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format this line, please

src/components/question-editor/QuestionEditor.tsx Outdated Show resolved Hide resolved
@@ -150,12 +161,14 @@ const CreateOrEditQuizQuestion: FC<CreateOrEditQuizQuestionProps> = ({
useEffect(() => {
!question && onOpenCreateQuestionModal()
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try to not use eslint disablers in our project

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it isn't mine

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but If you're working on the pr you should fix all of the issues coming with it. In my opinion at least...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to do my best

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just add useCallback to the onOpenCreateQuestionModal function declaration. This will stabilize the reference for that function, and useEffect will run only if question or onOpenCreateQuestionModal are changed.

@@ -37,6 +37,7 @@ export interface UpdateQuestionParams {

export interface QuestionFormAnswer extends Answer {
id: number
isEditing?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isEditing optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because isEditing only exists on openAnswer question type

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay then perhaps should we make 2 different types? up 2 u tho

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I'll take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to leave as it was because creating 2 different types brings a lot of types mismatches

@markgol777
Copy link
Contributor

markgol777 commented Feb 3, 2025

image

Regarding this. This is so called varenychky or gastronomy just rerun the tests and they should work.

Perhaps your coverage will go up too

@amoutens
Copy link
Contributor Author

amoutens commented Feb 3, 2025

image

Regarding this. This is so called varenychky or gastronomy just rerun the tests and they should work.

Perhaps your coverage will go up too

Thanks, I'll try. I'm going to fix some comments and then push again

@markgol777
Copy link
Contributor

image
Regarding this. This is so called varenychky or gastronomy just rerun the tests and they should work.
Perhaps your coverage will go up too

Thanks, I'll try. I'm going to fix some comments and then push again

image

It's just enough to push this button but alright

Copy link

sonarqubecloud bot commented Feb 3, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
75.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

]
handleNonInputValueChange('answers', addAnswer)
}
if (isEmptyAnswer) return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add braces here, please

Comment on lines +164 to +168
const onOpenCreateQuestionModalRef = useRef(onOpenCreateQuestionModal)

useEffect(() => {
!question && onOpenCreateQuestionModal()
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [])
!question && onOpenCreateQuestionModalRef.current()
}, [question])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very strange usage of useRef
Check, please, how it workds without useRef, but with onOpenCreateQuestionModal inside dependency array

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format this file, please

fireEvent.click(addNewOneButton)

expect(handleNonInputValueChange).toHaveBeenCalledWith("answers", [
{ id: 0, text: "", isCorrect: true, isEditing: true },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{ id: 0, text: "", isCorrect: true, isEditing: true },
{ id: 0, text: '', isCorrect: true, isEditing: true },

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to format this file with prettier

@@ -116,6 +129,37 @@ describe('CreateOrEditQuizQuestion component without question', () => {
expect(snackbar).toBeInTheDocument()
expect(createQuestionSpy).toHaveBeenCalled()
})
it('should call onCreateQuestion with openAnswer data', async () => {
mockAxiosClient
.onPost(`${URLs.resources.questions.post}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.onPost(`${URLs.resources.questions.post}`)
.onPost(URLs.resources.questions.post)

@@ -38,4 +38,24 @@ describe('Quiz Question tests', () => {
const element = screen.getByText('0/1')
expect(element).toBeInTheDocument()
})
it('Should render correctness icon if shouldShowAnswersCorrectness is true', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('Should render correctness icon if shouldShowAnswersCorrectness is true', () => {
it('should render correctness icon if shouldShowAnswersCorrectness is true', () => {

const icon = screen.getAllByTestId('CheckIcon')[0]
expect(icon).toBeInTheDocument()
})
it('Should render open answer input field', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('Should render open answer input field', () => {
it('should render open answer input field', () => {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Frontend part Functional Something isn't working
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

(SP: ) Fix the ability to save answer of question when type is Open Answer
3 participants