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

feat: Delete old assets on test overwrite #2393

Merged
merged 43 commits into from
Jul 25, 2023

Conversation

hectoras
Copy link
Contributor

@hectoras hectoras commented Jul 6, 2023

Associated Jira issue: PISA25-411

When importing a test with the overwrite option, if there are media assets referenced by the items in the test, delete them as well the same way we do for items. If that operation would end up with the former asset classes empty, delete them too.

Related PRs:

@codecov-commenter
Copy link

codecov-commenter commented Jul 11, 2023

Codecov Report

Merging #2393 (5d38559) into develop (d15113c) will increase coverage by 0.08%.
The diff coverage is 39.02%.

@@              Coverage Diff              @@
##             develop    #2393      +/-   ##
=============================================
+ Coverage      16.71%   16.80%   +0.08%     
- Complexity      3173     3182       +9     
=============================================
  Files            206      207       +1     
  Lines          11433    11463      +30     
=============================================
+ Hits            1911     1926      +15     
- Misses          9522     9537      +15     
Impacted Files Coverage Δ
models/classes/class.QtiTestService.php 5.93% <0.00%> (-0.15%) ⬇️
models/classes/event/QtiTestsDeletedEvent.php 100.00% <100.00%> (ø)
models/classes/xmlEditor/XmlEditor.php 93.33% <100.00%> (-6.67%) ⬇️

@hectoras
Copy link
Contributor Author

taoQtiTest_models_classes_QtiTestService::importMultipleTests directly instantiates taoQtiTest_models_classes_PackageParser to extract a zip file and taoQtiTest_models_classes_ManifestParser to parse some of its contents. Moreover, it does some checks based on filesystem contents (is_dir...) before calling the protected method importTest, which then calls the private method deleteTestsFromClassByLabel modified by this PR.

Therefore, it does not seem trivial to add unit tests for the new code unless we do a major refactor of taoQtiTest_models_classes_QtiTestService first.

Copy link
Contributor

@gabrielfs7 gabrielfs7 left a comment

Choose a reason for hiding this comment

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

Please, update composer.json with the minimal version of tao-items to be used here (as event payload was updated).

Thanks!

@gabrielfs7 gabrielfs7 self-requested a review July 20, 2023 14:42
Copy link
Contributor

@gabrielfs7 gabrielfs7 left a comment

Choose a reason for hiding this comment

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

  • New code is covered by tests (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • New code is respecting code style rules
  • New code is respecting best practices
  • New code is not subject to concurrency issues (if applicable)
  • Feature is working correctly on my local machine (if applicable)
  • Acceptance criteria are respected
  • Pull request title and description are meaningful

@github-actions
Copy link

Version

Target Version 47.2.0
Last version 47.1.4

There are 0 BREAKING CHANGE, 4 features, 3 fixes

@hectoras hectoras merged commit d2d83c3 into develop Jul 25, 2023
4 checks passed
@hectoras
Copy link
Contributor Author

Released as v47.2.0.

@hectoras hectoras deleted the feat/PISA25-411-delete-old-assets-on-test-overwrite branch July 25, 2023 08:54
hectoras added a commit that referenced this pull request Jul 25, 2023
* feat: Delete old assets on test overwrite

* feat: Rename event, trigger event before the deletions

* feat: Remove logic to extract media references form this extension

* fix: dont delete classes whose label don't match the expected label

---------

Co-authored-by: Gabriel Felipe Soares <gabrielfs7@gmail.com>
@hectoras
Copy link
Contributor Author

Backported as v47.1.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants