Skip to content

Conversation

@danielcebrian
Copy link
Contributor

Here is the first round of annotation tools for the platform. Included are Text and Video annotation modules along with the My Notes page module that aggregates all annotations.

Copy link
Contributor

Choose a reason for hiding this comment

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

All of these scripts need to be loaded via require.js, instead of via <script> tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First of all, happy new year 2014
Thank you very much for reviewing the code
I going to answer all your reviews.
I do not know how to use Require.js. As soon as possible I will try to use this library, and make a new pull

@singingwolfboy
Copy link
Contributor

This pull request is going to need a lot of cleanup before it's ready to go in: probably some major refactoring to account for things like internationalization, modular, testable Javascript, RESTful URL routing, and so on. I also don't see any tests whatsoever.

Daniel Cebrián and others added 2 commits January 3, 2014 13:23
@jtauber
Copy link
Contributor

jtauber commented Jan 15, 2014

@danielcebrian Can this PR be rebased? At the moment it can't be merged (and a clean rebase will make reviewing easier)

@sarina
Copy link
Contributor

sarina commented Jan 16, 2014

@danielcebrian FYI if you're unfamiliar about rebasing, see https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request and feel free to ask us.

@jzoldak
Copy link
Contributor

jzoldak commented Jan 16, 2014

@jtauber and @danielcebrian:

Reviewing from a testeng perspective: this PR currently has 13% unit test coverage on lines of python code changed.

We cannot merge in changes to the student and notes djangoapps or new xmodules (textannotation_module and videoannotation_module) that have no unit tests.

Also the changes to the StudentNotes class in notes.coffee should be covered by a Jasmine test.

@jtauber
Copy link
Contributor

jtauber commented Jan 16, 2014

@jzoldak yep, they are working on it. Luis just sent an email to the edx-code google group (subject "Testing Xmodule Question") asking for some help on a particular point.

@jzoldak
Copy link
Contributor

jzoldak commented Jan 16, 2014

Awesome. I can help out Luis, no problem.
@danielcebrian Will you guys be closing out this PR in favor of #2188?

@danielcebrian
Copy link
Contributor Author

Thank you to all for your help and evaluation.
Please, let me continue in the other pull
https://github.com/edx/edx-platform/pull/2188
We are working to test all the code

jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Jul 25, 2017
…nosis_generate_csv openedx#2023 openedx#2024 (openedx#2049)

* Add command ga_diagnosis_send_data openedx#2023

* Add report email feature to ga_diagnosis_generate_csv command openedx#2024
shimulch pushed a commit to open-craft/openedx-platform that referenced this pull request Jan 26, 2021
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.

7 participants