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

Feature/aut 1647 add xinclude loading on sleep #154

Merged

Conversation

Karol-Stelmaczonek
Copy link
Contributor

@Karol-Stelmaczonek Karol-Stelmaczonek commented Mar 18, 2024

Ticket: https://oat-sa.atlassian.net/browse/AUT-1647

What's Changed

  • Add loading of xi:include elements nested in pages

Dependencies PRs

How to test

  • Create new item
  • Add Text Reader Interaction
  • Add shared stimulus to pages
  • Shared stimulus is rendered in authoring
Screenshare.-.2024-02-29.10_08_29.AM.mp4

Copy link

github-actions bot commented Mar 18, 2024

Front-end summary Node 18

💯 Total ✅ Passed ⏭️ Skipped ❌ Failed
0 0 0 0

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (4157149) to head (8a16077).

Additional details and impacted files
@@            Coverage Diff            @@
##             develop    #154   +/-   ##
=========================================
  Coverage       0.00%   0.00%           
  Complexity        65      65           
=========================================
  Files              6       6           
  Lines            205     205           
=========================================
  Misses           205     205           

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

Copy link
Contributor

@jsconan jsconan left a comment

Choose a reason for hiding this comment

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

The code looks ok. In the end, we should consider move the xincludeloader to a shared place so that we can reuse it.

Please add the PCI version bump and a migration script. Also, please compile the PCI and add the bundles.

Copy link
Contributor

@jsconan jsconan left a comment

Choose a reason for hiding this comment

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

Wait, in the authoring, there is an error in the console if the PCI does not come with a shared stimulus...
image

@viktar-dzmitryieu-tao
Copy link

It works ok, but:

  • user can still edit the stimulus content in item-authoring, but the changes will not be applied which is correct. Probably we should lock ability to edit stimulus here as it can confuse the user.
  • if we export the item with stimulus - qti.xml contains xi:include href=\"taomedia://mediamanager.. in pages which means it couldn't be loaded as is on the other environment, I think we should consider the need of BE work on export functionality.

Copy link
Contributor

@jsconan jsconan left a comment

Choose a reason for hiding this comment

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

Good job, concise and neat!

Review Checklist

  • New code is covered by tests (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • New code respects code style rules
  • New code respects 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
  • Pull request's target is not master
  • Commits are following conventional commits
  • Commits messages are meaningful
  • Commits are atomic

Copy link
Contributor

@jsconan jsconan left a comment

Choose a reason for hiding this comment

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

Wait, there is an issue as spotted by @viktar-dzmitryieu-tao.
Actually, the feature does:

  • the shared stimulus is inlined inside the PCI…
  • user can modify the content and save it
  • the PCI can be exported and imported in another platform: the shared stimulus being inlined, the content is present but the shared stimulus is not defined as such
  • any change made later to the shared stimulus is not reflected… because it is inlined

Finally, there is a problem because the connection with the shared stimulus is lost at some point

@Karol-Stelmaczonek
Copy link
Contributor Author

It works ok, but:

  • user can still edit the stimulus content in item-authoring, but the changes will not be applied which is correct. Probably we should lock ability to edit stimulus here as it can confuse the user.
  • if we export the item with stimulus - qti.xml contains xi:include href=\"taomedia://mediamanager.. in pages which means it couldn't be loaded as is on the other environment, I think we should consider the need of BE work on export functionality.

Thanks for noticing this two problems. They are not directly related to changes in the PR so I will put a comment under my task for QA team to check those and create new bug tickets for them.

Copy link

@viktar-dzmitryieu-tao viktar-dzmitryieu-tao left a comment

Choose a reason for hiding this comment

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

Approved as this code works well and considering that the spotted issues will be addressed in some way

Copy link
Contributor

@jsconan jsconan left a comment

Choose a reason for hiding this comment

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

Fine for me too, as long as the reported defects are addressed with a later task 😉

Copy link

Version

Target Version 3.9.0
Last version 3.8.7

There are 0 BREAKING CHANGE, 2 features, 4 fixes

@Karol-Stelmaczonek Karol-Stelmaczonek merged commit 675e3fd into develop Mar 29, 2024
5 of 6 checks passed
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