Reconnecting Token Generator for Annotation Tool #3466
Conversation
|
@lduarte1991 Thanks, making smaller pull requests should make this easier to review. :) Also, we're going to need a signed individual contributor agreement from you -- can you send that in, please? |
|
Absolutely! Is that something I should email? Fax over to Edx? Give to someone at HarvardX? |
|
As it says on the form, you can either email it to jennifer@edx.org, or print it out and mail it to the address on the form. |
|
Will do 👍 |
|
@cpennington Since there are no CMS-related items Christina said I should tag you to check the LMS and approve/merge this PR. Thanks in advance! |
There was a problem hiding this comment.
The diacritics field seems out of place in this PR.
There was a problem hiding this comment.
It is, I was trying to unravel a few PRs that weren't related and must have missed this. I'm literally about to do another PR that includes/uses the diacritics. Should I remove it here anyway and add it back in the next PR? It doesn't cause any errors/issues as far as I can tell.
There was a problem hiding this comment.
Yes, lets keep the PRs focused. It'll help them be easier to review.
|
@cpennington Is there any way I can be whitelisted so that the Jenkins tests will run? Thanks |
|
@jzoldak Can you do that? |
|
@lduarte1991 actually, it looks like the tests did run. They sometimes take a while to start. |
|
Yes I saw that it says "Failed" because they were aborted by @jzoldak. Any way they can actually be run? Thanks in advance! Is there anything else you're waiting on me for? |
|
@lduarte1991 I have added you to the whitelist so PRs from your fork will trigger builds automatically. This morning we had a problem with a namespace collision on some jenkins workers so I had to abort a bunch of builds in order to recycle the workers. That issue has been solved and I have just manually re-triggered a build for this PR. |
|
@lduarte1991 Looks like there are unit tests failures. We're on the verge of cutting a release, is there any urgency around this PR? |
|
@e0d There is some urgency and the unit tests can be fixed really quickly. Give me a few minutes if you can? |
|
@e0d Just fixed it but I guess the test isn't running automatically? I ran it locally and the error seems to be gone. EDIT: Never mind it just started running. |
There was a problem hiding this comment.
Nitpick: # Your comment, rather than #your comment
|
👍 |
|
Hey Calen, I'm assuming that thumbs up means the PR is good to push. When will this be merged? |
|
@cahrens @singingwolfboy: Does one of you guys want to be the other +1? |
|
@singingwolfboy Do you have time to take a look? This PR doesn't have Studio changes. |
There was a problem hiding this comment.
Why did you change from self._render_content() to self.content? Is there a difference?
There was a problem hiding this comment.
It was a change the Daniel made in #2515. Basically render_content was an old function from annotatable that was not needed. the function has since been removed.
|
@lduarte1991 Do you want to make any of those last-minute changes? The only comments I had left were nitpicks, so I'm happy to merge this pull request without those fixes -- but I don't want to merge it before you're ready for it to be merged. |
|
@singingwolfboy Give me an hour to make some fixes. Would you like me to rebase and squash as well? |
|
@singingwolfboy Let me know if I need to and I'll do it, otherwise feel free to merge! |
|
@singingwolfboy Can you tell me what went wrong here? I'm still a bit iffy on what the bok-choy test actually checks for and what went wrong even looking at the console output for it. |
|
@lduarte1991 I think it's just a flaky test. I'm trying to run the tests on your branch locally on my computer to be sure. |
|
@singingwolfboy Did the tests turn out ok? or should I change something? Please let me know asap so I can try to have it fixed by monday. Thanks! |
|
@lduarte1991 I never actually got the tests to run on my machine -- but that's a different problem for a different day. I've kicked off another Jenkins test run -- lets see if it passes this time. |
|
Alright, the bok choy tests all passed, but now one of the Jasmine tests is failing. I'm certain that's a flaky test, so I'm going to merge this. |
Reconnecting Token Generator for Annotation Tool
This reverts commit 59e3cae.
7 participants
Background: As per Christina's request I'll be turning the Pull Request #2515 into a few smaller PRs.
Goal of this PR: The goal of this PR is to fix two main issues. The first is to use the Firebase Token Generator from the PyPI version of Firebase and the second is to pass the token as a string rather than create a view (in djangoapp/students) that pops up with the token as a string in the body of the html.
CMS Updates: This one still follows the original first push in that Annotator is not yet working in the CMS (this will change in one of the following chunks). In terms of creating a test environment, the Firebase Token Generator uses a different token standard and the current testing backend does not meet those so please email me to ask for a new secret token and url link to the annotation_storage_url. There are no UI changes.
LMS Updates: Nothing should change in the UI of the LMS, only the way that the token is handled should change. The secret token and url links have been changed so email me for updates.
Testing notes: Once the proper secret token and URL are placed you should be able to create annotations in the LMS (not the CMS). There is a problem if you get a black bar at the top saying "annotation could not be stored" or if you get a 401 error in the console debugger.