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.

Sign up for GitHub

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

Jump to bottom

Fixes Opencast-Moodle/moodle-filter_opencast#54 #55

Merged
merged 7 commits into from
Jan 16, 2025
Merged

Fixes Opencast-Moodle/moodle-filter_opencast#54 #55

merged 7 commits into from
Jan 16, 2025

Conversation

elke-hsh
Copy link
Contributor

No description provided.

ferishili
ferishili requested changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@ferishili ferishili left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@elke-hsh Thanks for your work. That would be great if you could take care of these points.
Kudos for using the organization api endpoint to retrieve the engage UI URL!

classes/local/lti_helper.php Outdated Show resolved Hide resolved
classes/local/lti_helper.php Outdated Show resolved Hide resolved
lang/en/filter_opencast.php Outdated Show resolved Hide resolved
@ferishili
Copy link
Contributor

Hi @elke-hsh,
As we approach the deadline for merging new changes to the plugins in favor of the refactoring, I would like to ask you to either handle the changes or, if you are unable to, let me proceed!
Thanks in advance

@elke-hsh
Copy link
Contributor Author

elke-hsh commented Jan 13, 2025 via email

@ferishili
Copy link
Contributor

Thanks for your work @elke-hsh

@ferishili
Copy link
Contributor

@elke-hsh, is it ok if I add a few things to your PR?

@elke-hsh
Copy link
Contributor Author

elke-hsh commented Jan 16, 2025 via email

@ferishili
Copy link
Contributor

@elke-hsh, I have tested it locally and the result from getOrgEngageUIUrl for AllInOne Instance is localhost:8080, which is wrong!
Have you test this on your dual-node instance? and is it working?

@elke-hsh
Copy link
Contributor Author

elke-hsh commented Jan 16, 2025 via email

@ferishili
Copy link
Contributor

@elke-hsh
Please test the latest commit I just pushed, which ensures we get a proper engage url as the base url.
Would be nice to give a short and quick feedback, in order for us to proceed with the release today.
Thanks in advance.

@elke-hsh
Copy link
Contributor Author

Unfortunately it does not work on our system.

@ferishili
Copy link
Contributor

Would you please DM me in Element, so we can tackle this in an Video Conference?
Thanks

@ferishili
Copy link
Contributor

@elke-hsh
please rebase this PR and test it again. thanks!

@ferishili
Copy link
Contributor

As discussed in our meeting today:

  • From now on, the API User should get the role ROLE_UI_EVENTS_EMBEDDING_CODE_VIEW only when the filter plugin is going to deliver videos via LTI Authentication!

    • The reason is that we are using the endpoint /api/info/organization/properties/engageuiurl and that endpoint by default needs this role: ROLE_UI_EVENTS_EMBEDDING_CODE_VIEW

  • We don't go for services search, because it adds unwanted ADMIN role to the API Users!

  • The changes now could satisfy both AllInOne and Multi-Node instances!

@ferishili
Copy link
Contributor

ferishili commented Jan 16, 2025 •
edited
Loading

@bluetom the PR is ready!
And just please keep in mind that we need to explicitly update the readme file of this plugin and add the additional role required: ROLE_UI_EVENTS_EMBEDDING_CODE_VIEW when using LTI Auth!

@elke-hsh
Copy link
Contributor Author

elke-hsh commented Jan 16, 2025 via email

@bluetom bluetom merged commit 3754760 into Opencast-Moodle:MOODLE_405_STABLE Jan 16, 2025
8 checks passed
bluetom added a commit that referenced this pull request Jan 16, 2025
@bluetom @elke-hsh @ferishili
Fixes #54 (#55) (#58)
684a3ea 
* Fixes #54

* read paella json data properly from mod, (#57)

fixes #56

* Use Opencast PHP Library for api call

Change error message

* make sure lti launch gets a proper base url in both allinone and multi-nodes opencast instances

* change the service to search and make sure it is active

* get rid of services

---------

Co-authored-by: elke-hsh <[email protected]>
Co-authored-by: FarbodZamani <[email protected]>
Co-authored-by: ferishili <[email protected]>
bluetom added a commit that referenced this pull request Jan 17, 2025
@bluetom @elke-hsh @ferishili
Moodle 405 stable 4.5.3 (#59)
be82761 
* Fixes #54 (#55)

* Fixes #54

* read paella json data properly from mod, (#57)

fixes #56

* Use Opencast PHP Library for api call

Change error message

* make sure lti launch gets a proper base url in both allinone and multi-nodes opencast instances

* change the service to search and make sure it is active

* get rid of services

---------

Co-authored-by: FarbodZamani <[email protected]>
Co-authored-by: ferishili <[email protected]>

* version.php changelog readme

* moodle release change branch to main

---------

Co-authored-by: elke-hsh <[email protected]>
Co-authored-by: FarbodZamani <[email protected]>
Co-authored-by: ferishili <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ferishili ferishili ferishili approved these changes

Assignees
No one assigned
Labels
4.5.3 bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elke-hsh @ferishili @bluetom