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

fix: incorrect field validation on the grading page #919

Closed

Conversation

DmytroAlipov
Copy link
Contributor

@DmytroAlipov DmytroAlipov commented Mar 25, 2024

Description

  1. Possible save just space forAssignment type name
  2. Possible save Number of droppable field with values more than the Total number value which leads to displaying errors all the time
  3. Possible save Weight of total grade field with empty values:
    44

Implemented so that when an error occurs, the modal window containing the Save changes button is not displayed:

12 11

Fixed display of errors for the Number of droppable field:

14 15

@DmytroAlipov DmytroAlipov requested a review from a team as a code owner March 25, 2024 19:55
@openedx-webhooks
Copy link

Thanks for the pull request, @DmytroAlipov! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 25, 2024
@DmytroAlipov DmytroAlipov force-pushed the fix-valdate-assignment-types branch from adbc8ec to db5839a Compare March 25, 2024 20:31
@DmytroAlipov DmytroAlipov added the bug Report of or fix for something that isn't working as intended label Mar 25, 2024
@DmytroAlipov DmytroAlipov force-pushed the fix-valdate-assignment-types branch from db5839a to cccc106 Compare March 26, 2024 18:30
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.01%. Comparing base (1dde30a) to head (cccc106).
Report is 22 commits behind head on master.

❗ Current head cccc106 differs from pull request most recent head de73dd1. Consider uploading reports for the commit de73dd1 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #919      +/-   ##
==========================================
+ Coverage   91.93%   92.01%   +0.08%     
==========================================
  Files         572      572              
  Lines       10166    10175       +9     
  Branches     2199     2204       +5     
==========================================
+ Hits         9346     9363      +17     
+ Misses        793      787       -6     
+ Partials       27       25       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jesperhodge jesperhodge left a comment

Choose a reason for hiding this comment

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

Thanks for these fixes! Everything works super well.
Would you mind just adding a tiny test that checks that droppable number will trigger an error if it's larger than total number? also, if I'm just blind and the test is there somewhere, please just correct me.
I just want to make sure there's no regression bug later.
Thanks for this and once that test is there I'll approve

@DmytroAlipov
Copy link
Contributor Author

Hi, @jesperhodge
I struggled with this test for a long time, but nothing worked because fireEvent does not work. I'm not very good at Java Script. I will be very grateful if anyone can help me with this test:

  it('checking correct error msg if dropCount more than minCount', async () => {
    const { getByText, getByTestId } = render(<RootWrapper />);
    await waitFor(() => {
      const assignmentMinCountInput = getByTestId('assignment-minCount-input');
      expect(assignmentMinCountInput.value).toBe('2');
      const assignmentNumberOfDroppableInput = getByTestId('assignment-dropCount-input');
      expect(assignmentNumberOfDroppableInput.value).toBe('1');
      fireEvent.change(assignmentNumberOfDroppableInput, { target: { value: '50' } });
      expect(getByText(messages.numberOfDroppableSecondErrorMessage.defaultMessage)).toBeInTheDocument();
    });
  });

@jesperhodge
Copy link
Member

Hi @DmytroAlipov , sorry for your troubles there! This was a tricky one.

Apparently the problem is that the test mocks out the setGradingData function in the <RootWrapper>. Since only the second error message depends on gradeField?.dropCount, I think gradeField never gets changed by the onChange because of this function mockout in the test. The first error message does not depend on gradeField so that's why other tests work.

Now instead of changing the mocking, I'd do a simpler fix to get the test to work. Just do the following, that works:

  it('checking correct error msg if dropCount more than minCount', async () => {
    const { getByTestId } = render(<RootWrapper />);
    const assignmentMinCountInput = getByTestId('assignment-minCount-input');
    expect(assignmentMinCountInput.value).toBe('2');
    const assignmentNumberOfDroppableInput = getByTestId('assignment-dropCount-input');
    expect(assignmentNumberOfDroppableInput.value).toBe('1');
    expect(assignmentNumberOfDroppableInput.classList).not.toContain('is-invalid');
    await waitFor(() => {
      fireEvent.change(assignmentNumberOfDroppableInput, { target: { value: '50' } });
      const dI = getByTestId('assignment-dropCount-input');
      expect(dI.classList).toContain('is-invalid');
    });
  });

This may not check which error message is displayed, but that's not very important, the test demonstrates that there is an error, so that's good enough.

@DmytroAlipov DmytroAlipov force-pushed the fix-valdate-assignment-types branch 2 times, most recently from 6a28e02 to 594e0e3 Compare April 9, 2024 17:27
@DmytroAlipov DmytroAlipov force-pushed the fix-valdate-assignment-types branch from 594e0e3 to de73dd1 Compare April 9, 2024 17:45
@DmytroAlipov
Copy link
Contributor Author

@jesperhodge Thank you very much for your help!
But now for some reason, the cover is falling...

@DmytroAlipov
Copy link
Contributor Author

Hi @PKulkoRaccoonGang. Could you help me correct this test?

@openedx-webhooks
Copy link

@DmytroAlipov Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@DmytroAlipov
Copy link
Contributor Author

I closed this PR because a new one was opened to fix this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Report of or fix for something that isn't working as intended open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants