Skip to content
This repository was archived by the owner on Nov 15, 2024. It is now read-only.

Improve: add data extraction management command - #440

Merged
asamolion merged 14 commits into
developfrom
osama/LP-845
Apr 26, 2018
Merged

Improve: add data extraction management command#440
asamolion merged 14 commits into
developfrom
osama/LP-845

Conversation

@asamolion

Copy link
Copy Markdown

No description provided.

@muhammadnadeem muhammadnadeem left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is for first iteration.

Comment thread lms/envs/common.py Outdated
'lms.djangoapps.student_dashboard',

# Data extraction App
'openedx.features.database_extract',

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please update the name of app.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done.

@@ -0,0 +1,5 @@
from django.contrib import admin
from .models import TargetCourse

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

please remove relative imports

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

from django.contrib import admin
from .models import TargetCourse

admin.site.register(TargetCourse)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Model name should be improved. You should delete all migrations and recreate after updating the name of model

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

name Improved. Now is CourseDataExtraction

def get_file_url(answer):
try:
return ora_file_upload_api.get_download_url(answer.file_key)
except:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is it possible to create some custom exception for this ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

used AttributeError

'modified': course_structure.modified.__str__(),
'course_id': course_structure.course_id.to_deprecated_string(),
'structure_json': course_structure.structure_json,
'discussion_id_map_json': course_structure.discussion_id_map_json,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is this map contain the id of course related NodeBB discussion ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No. this is something that is built in. I'm not sure what this is. Probably the discussion_id of default Ruby/Sinatra app that edx uses.


user_community_data = json.loads(response._content)['payload']

demo_data = {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i think variable name should be updated

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

changed to demographic_data

'postcount': user_community_data['postcount'],
}

anon_user_id = AnonymousUserId.objects.get(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you should get all users anonymous user_ids in a single query and convert them to a map. Then inside loop you can reuse that map instead of making a query for each loop iteration.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

'done': module.done,
'created': module.created.__str__(),
'modified': module.modified.__str__(),
} for module in StudentModule.objects.filter(student_id=profile.user.id)],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

are you sure this loop will skip all irrelevant records in StudentModule table like type="sequential" ? i think this will get some extra data that we don't need. Also this code should also be spitted using functions.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think they don't mind if we deliver this data as well.

old_name='email_list',
new_name='emails',
),
]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

delete all existing migrations and create only one migration as all db related things are final now.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done.

emails = models.TextField()

def __unicode__(self):
return '{}'.format(self.course_id) No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

enter new line at end of each file.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

@asamolion
asamolion merged commit a51cefa into develop Apr 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants