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: added Multi AUs support for iframe #5

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

Talha-Rizwan
Copy link
Contributor

@Talha-Rizwan Talha-Rizwan commented Dec 27, 2023

Added support for cmi5 content having multiple AUs.
AUs with different launch methods ('OwnWindow', 'AnyWindow') are displayed differently.

Merge checklist:
Check off if complete or not applicable:

  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Fixup commits are squashed away
  • Unit tests added/updated
  • Manual testing instructions provided
  • Noted any: Concerns, dependencies, migration issues, deadlines, tickets

Copy link
Contributor

@ziafazal ziafazal left a comment

Choose a reason for hiding this comment

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

@Talha-Rizwan I have few suggestions.

@@ -11,6 +11,8 @@ <h3 class="xblock-title mb-2">{{title}}</h3>
height="{% if cmi5_xblock.height %}{{ cmi5_xblock.height }}{% else %}450{% endif %}"
>
</iframe>
<button id="nextButton">Next Assessment</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of buttons we should show list of hyperlinks each representing an AU. Clicking on the relevant link should open AU in currentWindow or ownWindow. Let me try to explain possible scenarios

  1. Single AU and launchMethod is "AnyWindow"
    We can use iframe as it is being used currently inside openedx_cmi5_xblock.html and src should be url of AU

  2. Single AU and launchMethod is "OwnWindow"
    openedx_cmi5_xblock.html file should have a single link with title Launch AU and click on that should open new window having url of AU with launch params

  3. Multiple AU and launchMethod is "AnyWindow"
    openedx_cmi5_xblock.html file should a navigation menu on left side and iframe on right side. Navigation should have list of all AUs. Clicking on any AU should open its in iframe on the right side

  4. Multiple AU and launchMethod is "OwnWindow"
    openedx_cmi5_xblock.html file should just have a navigation menu having list of all AUs. Click on any AU link should open it up in a new browser window with launch params

@@ -260,6 +284,22 @@ def studio_submit(self, request, _suffix):
response['errors'].append(e.args[0])
return json_response(response)

def launch_au_url(self, url):
Copy link
Contributor

Choose a reason for hiding this comment

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

most of the code in this function is copy of code used in index_page_url property we should make a new method e.g make_launch_url and move common code inside that method to reuse it at both places.

if au_url is not None:
self.index_page_path = au_url.text
au_urls = root.findall('.//{prefix}au/{prefix}url'.format(prefix=prefix))
if au_urls is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit : if au_urls should be sufficient here

@Talha-Rizwan Talha-Rizwan removed the request for review from hinakhadim January 1, 2024 06:32
Copy link
Contributor

@ziafazal ziafazal left a comment

Choose a reason for hiding this comment

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

👍

{% if index_page_url %}
<h5 class="xblock-title mb-2">Available Assignables:</h5>
Copy link
Contributor

Choose a reason for hiding this comment

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

mark string for translations using trans

<ol>
{% for au_url in cmi5_xblock.au_urls %}
<li>
<a href="{{ au_url.url }}" target="_blank">AU No. {{ forloop.counter }}</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

mark string for translations using trans

@Talha-Rizwan
Copy link
Contributor Author

Talha-Rizwan commented Jan 3, 2024

@ziafazal the issue of xapi statements tracking for multiple AUs is resolved.

Copy link
Contributor

@ziafazal ziafazal left a comment

Choose a reason for hiding this comment

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

LGTM


function updateIframeSrc(href, liText) {
if(liText.includes('AnyWindow')){
$('.cmi5-embedded').attr('src', href);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indentation needed here

@Talha-Rizwan Talha-Rizwan force-pushed the talharizwan/feat-multiple-AU-support branch from 7f0e5cd to 37a7db5 Compare January 15, 2024 10:00
@Talha-Rizwan Talha-Rizwan merged commit c052aa7 into master Jan 15, 2024
4 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.

3 participants