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

Moodle PROXY setting not used #84

Closed
nadavkav opened this issue Aug 21, 2016 · 9 comments
Closed

Moodle PROXY setting not used #84

nadavkav opened this issue Aug 21, 2016 · 9 comments
Assignees
Labels
Milestone

Comments

@nadavkav
Copy link
Contributor

It seems that the RusticiSoftware/TinCanPHP library that is used in this project did not implement proxy support when initiating the remotelrs class (and obviously, when sending the xAPI staements to the remote LRS)

An issue report and a workaround can be found here:
RusticiSoftware/TinCanPHP#63

I have temporarily worked around it, as our campus is behind a reverse proxy (firewall).
But a fix need to be applied first in RusticiSoftware/TinCanPHP before we can implement a permanent solution on our end. (this project)

Please help raise priority of this issue on the RusticiSoftware/TinCanPHP project (above link) as we are depending on them.

@ryasmi
Copy link
Member

ryasmi commented Aug 21, 2016

Thanks @nadavkav 👍

@ryasmi ryasmi self-assigned this Aug 21, 2016
@ryasmi ryasmi added this to the v1.1.1 milestone Aug 21, 2016
@ryasmi ryasmi modified the milestones: v1.1.1, v1.2.0 Aug 21, 2016
@deedey
Copy link
Contributor

deedey commented Aug 22, 2016

You are right @nadavkav.
Thank you for this suggest.
I think it would be much more appropriate when calling from "RemoteLRS.php" in "/ mod / tincanlaunch / TinCanPHP / src /", the API directly retrieves the proxy data if exists from the table "mdl_config" variables "proxyhost" and "proxyport" to implement them directly.
If this is managed at "RemoteLRS.php", the problem does not arise, nor for Tincanlaunch nor logstore_xapi.

@nadavkav
Copy link
Contributor Author

nadavkav commented Aug 22, 2016

@brianjmiller (of RusticiSofware) was quick enough to come up with a fix on their side.
RusticiSoftware/TinCanPHP#68
I added a quick test code snippet inside the above issue, but suggest a better one on our side:
https://github.com/LearningLocker/xAPI-Recipe-Emitter/blob/master/src/Repository.php#L15

public function __construct(TinCanRemoteLrs $store) {
        global $CFG;
        $this->store = $store;
        if (!empty($CFG->proxyhost)) {
            $this->store->setProxy($CFG->proxyhost.':'.$CFG->proxyport);    
        }
    }

@deedey
Copy link
Contributor

deedey commented Aug 23, 2016

@garemoko @brianjmiller @nadavkav
Can you suggest a same solution to integrate proxy parameters automaticly in [https://github.com/garemoko/moodle-mod_tincanlaunch]
I can open an issue if necessary.

@ryasmi
Copy link
Member

ryasmi commented Aug 23, 2016

Hi @nadavkav, thanks for that snippet, but we cannot allow Moodle specific code inside the xAPI recipe emitter, because it is used by other plugins outside of Moodle. Therefore, I would recommend the following adjustment to the "classes/log/store.php" file.

    /**
     * Creates a connection the xAPI store.
     * @return xapi_repository
     */
    private function connect_xapi_repository() {
        global $CFG;
        $remote_lrs = new tincan_remote_lrs(
            $this->get_config('endpoint', ''),
            '1.0.1',
            $this->get_config('username', ''),
            $this->get_config('password', '')
        );
        if (!empty($CFG->proxyhost)) {
          $remote_lrs->setProxy($CFG->proxyhost.':'.$CFG->proxyport);
        }
        return new xapi_repository($remote_lrs);
    }

I'd recommend trying out that change, then submitting a pull request to this repo.

@deedey
Copy link
Contributor

deedey commented Aug 23, 2016

@ryansmith94 @nadavkav ,
I've tested this last part of store.php script.
It works fine. Thanks a lot.

@nadavkav
Copy link
Contributor Author

@ryansmith94 you are right. I thought It was a Moodle specific plugin.
Anyways, your suggest code looks great.
And I already see @deedey is going to use it too.
So thank you all for quickly fixing this!

@ryasmi
Copy link
Member

ryasmi commented Aug 23, 2016

@deedey feel free to make a PR so that other people have it too 👍

@davidpesce
Copy link
Collaborator

This is fixed in v1.2.0

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

No branches or pull requests

4 participants