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

Assessment: Fix the loss of submission text due to overlapping manual text blocks #6916

Merged
merged 13 commits into from
Jul 21, 2023

Conversation

terlan98
Copy link
Contributor

@terlan98 terlan98 commented Jul 16, 2023

Checklist

General

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data.
  • I followed the coding and design guidelines.

Motivation and Context

Closes #6903

Description

Removed the console error for the case when manual text blocks were overlapping. Instead, added some code to handle the case as follows: determine the block that has a feedback assigned to it and add a new text block, if needed, to ensure that the block of the new feedback does not hijack some of the existing text.

Steps for Testing

Prerequisites:

  • 1 Text Exercise with a submission ready for assessment
  • 1 Tutor
  1. Log in to Artemis as Tutor
  2. Go to Course Management -> Assessment Dashboard -> Exercise Dashboard of the text exercise
  3. Start an assessment
  4. Select a specific portion of the text, by holding the alt/option key. Let's call the selected range [r1, r2]
  5. Add some feedback and save
  6. Refresh the page
  7. Verify that you see your feedback
  8. Delete the feedback
  9. Again, hold alt/option key to select a different range of text. BUT, make sure that the new range overlaps with [r1, r2]
  10. Add some feedback and save
  11. Refresh the page
  12. Verify that you can see your new feedback

Review Progress

Performance Review

  • I confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Client

Class/File Line Coverage Confirmation (assert/expect)
text-assessment-base.component.ts 84%

@github-actions github-actions bot added the client Pull requests that update TypeScript code. (Added Automatically!) label Jul 16, 2023
@terlan98 terlan98 temporarily deployed to artemis-test5.artemis.cit.tum.de July 16, 2023 19:37 — with GitHub Actions Inactive
@terlan98 terlan98 marked this pull request as ready for review July 17, 2023 06:45
@terlan98 terlan98 requested a review from a team as a code owner July 17, 2023 06:45
Copy link
Contributor

@JohannesStoehr JohannesStoehr left a comment

Choose a reason for hiding this comment

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

Can you also add a test for this bug?

@terlan98 terlan98 marked this pull request as draft July 18, 2023 13:32
@github-actions github-actions bot added the tests label Jul 18, 2023
…https://github.com/ls1intum/Artemis into bugfix/text-exercises/overlapping-manual-text-blocks

# Conflicts:
#	src/test/javascript/spec/component/text-submission-assessment/text-submission-assessment.component.spec.ts
@jakubriegel jakubriegel temporarily deployed to artemis-test4.artemis.cit.tum.de July 18, 2023 19:48 — with GitHub Actions Inactive
Copy link
Contributor

@jakubriegel jakubriegel left a comment

Choose a reason for hiding this comment

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

Code with test LGTM 👍🏻

Works on ts4

Copy link
Contributor

@JohannesStoehr JohannesStoehr 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 adding the test. Code looks good to me 👍

@tobias-lippert tobias-lippert temporarily deployed to artemis-test4.artemis.cit.tum.de July 19, 2023 14:55 — with GitHub Actions Inactive
Copy link
Contributor

@tobias-lippert tobias-lippert left a comment

Choose a reason for hiding this comment

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

tested on ts4. Works as described.

@pal03377 pal03377 temporarily deployed to artemis-test4.artemis.cit.tum.de July 19, 2023 17:25 — with GitHub Actions Inactive
Copy link
Contributor

@pal03377 pal03377 left a comment

Choose a reason for hiding this comment

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

Code looks good to me 🎉 I also tested it again on TS4 to get a better feel of what the code is doing.

@maximiliansoelch maximiliansoelch added the maintainer-approved The feature maintainer has approved the PR label Jul 20, 2023
@bassner bassner merged commit 47ca907 into develop Jul 21, 2023
19 checks passed
@bassner bassner deleted the bugfix/text-exercises/overlapping-manual-text-blocks branch July 21, 2023 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix client Pull requests that update TypeScript code. (Added Automatically!) maintainer-approved The feature maintainer has approved the PR ready to merge tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text Exercises, Assessment: Overlapping manual text blocks lead to the loss of some student text
7 participants