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

xAPI Agent Profile Resource - error in order of elements #1109

Closed
davidpesce opened this issue Feb 19, 2018 · 17 comments
Closed

xAPI Agent Profile Resource - error in order of elements #1109

davidpesce opened this issue Feb 19, 2018 · 17 comments
Assignees

Comments

@davidpesce
Copy link
Contributor

Version
2.1.3 and 2.1.4 (but guessing this is a global issue)

Steps to reproduce the bug

  1. Submit an agentProfile with the agent.name first and agent.mbox second (in the JSON object)
    "agent" : {
        "objectType" : "Agent", 
        "name" : "User Full Name", 
        "mbox" : "mailto:support@example.com"
    },
  1. Attempt to retrieve the agentProfile value

Expected behaviour
Value of the agentProfile will be returned

Actual behaviour
404 response. agentProfile doesn't exist.

Additional information
OS: CentOS 7.4
Browser: Chrome (latest)

If you swap the order of agent.mbox and agent.name, it is retrievable:

    "agent" : {
        "objectType" : "Agent", 
        "mbox" : "mailto:support@example.com",
        "name" : "User Full Name"
    },
@ryasmi
Copy link
Member

ryasmi commented Feb 20, 2018

Hmmm this is also odd because it shouldn't be storing the name I don't think.

@ryasmi
Copy link
Member

ryasmi commented Feb 20, 2018

Okay we are storing the name and I can't think why we're doing that, we certainly shouldn't be using it to match an agent as far as I'm aware.

@davidpesce
Copy link
Contributor Author

If you want me to take a look at attempting a PR, can you point me to where I should be looking.? I haven't dug too deep into the architecture of llv2 yet.

@ryasmi
Copy link
Member

ryasmi commented Feb 20, 2018

It's all good @davidpesce, I'm on it already, thanks for the offer though 👍

@ryasmi ryasmi removed the xAPI-State label Feb 20, 2018
@davidpesce
Copy link
Contributor Author

The spec doesn't say it's invalid if you do include both. It does say that you can only use one of the four reverse functional identifiers (mbox, mbox_sha1sum, openid, or account), but name is listed as an optional.

@ryasmi
Copy link
Member

ryasmi commented Feb 20, 2018

Yeah that's fine. I don't think we should have been storing the name in the database, it's not actually required. Just waiting for the build to pass over in learninglocker/xapi-agents#43 then I'll get this fix released 👍

@davidpesce
Copy link
Contributor Author

You da man! Thanks, Ryan!

@ryasmi
Copy link
Member

ryasmi commented Feb 20, 2018

Haha you're welcome @davidpesce 👍 that's just gone live v2.1.9 of the xapi-service - got to love automated releases haha.

@davidpesce
Copy link
Contributor Author

davidpesce commented Feb 20, 2018

I grabbed the tagged release for 2.1.9 and restarted the xapi (and then LL) pm2 services, but I'm still having the same issue. Did I miss something in applying the patch?

@ryasmi
Copy link
Member

ryasmi commented Feb 20, 2018

Hey @davidpesce you need to yarn install && yarn build 😄 Sorry I should have said that before.

@davidpesce
Copy link
Contributor Author

Yeah, that's what I forgot. The docs still show "npm install" and "npm run build". I did that and it worked like a charm!

@ryasmi
Copy link
Member

ryasmi commented Feb 20, 2018

Ah cool yeah npm will work too just a little less safe than yarn if you're not using npm 5 (our AMIs and deploy scripts still install npm 3 because we've not tested the webapp with npm 5). The difference is that npm 3 doesn't use the package-lock.json file whereas npm 5 does. Yarn uses the yarn.lock file. Not sure if you're familiar with lock files, just ensures that you install the exact same version of dependencies as our tests.

@davidpesce
Copy link
Contributor Author

Ok, cool. I wonder if it's worth it to update the custom install docs? http://docs.learninglocker.net/guides-custom-installation/

@ryasmi
Copy link
Member

ryasmi commented Feb 20, 2018

Yeah it almost certainly is. If you're able to make a PR there that would be awesome. It should be the src/guides-custom-installation.md file that needs updating in the docs repo.

@davidpesce
Copy link
Contributor Author

Yup can do!

@davidpesce
Copy link
Contributor Author

Confirming that this definitely resolved the issue. To make doubly-sure, I also tried re-migrating the LLv1 agentProfiles into the LLv2 DB and my course was able to retrieve them without a hitch. It also didn't create new agentProfiles (the originals retrieved fine).

@ryasmi
Copy link
Member

ryasmi commented Feb 20, 2018

Brilliant cheers @davidpesce 👍

Awesome thank you for confirming! That's really good.

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

No branches or pull requests

2 participants