-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat: add SurveyReportUpload and add send report method #31431
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
Merged
felipemontoya
merged 18 commits into
openedx:master
from
eduNEXT:survey_report_send_report
Feb 21, 2023
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
02259d1
feat: add SurveyReportUpload and add send report method
Alec4r 50703ad
docs: Update openedx/features/survey_report/management/commands/gener…
Alec4r a266eb1
docs: Update openedx/features/survey_report/models.py
Alec4r 2774888
refactor: Update openedx/features/survey_report/models.py
Alec4r 7ff5df2
style: Update openedx/features/survey_report/api.py
Alec4r 1bf781a
feat: add migratio file and update status field name
Alec4r 29d65d0
refactor: rename send report method
Alec4r 55f31dd
test: fix test errors
Alec4r 7fbfb02
test: add command options
Alec4r 2b5ac9a
refactor: simple conditional instead of ok method
Alec4r e83bebf
fix: remove useless imports
Alec4r 220aec5
fix: use status code instead of status
96827ec
feat: add zapier endpoint
Alec4r 8d3a27b
style: solve pylint issues
Alec4r 2287584
feat: add id field to send report data
Alec4r 5a460de
refactor: regenerate migration with correct history
mariajgrimaldi c7ecb23
feat: add anonymous site id model
Alec4r 85ce813
feat: update zapier url
Alec4r File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
24 changes: 24 additions & 0 deletions
24
openedx/features/survey_report/migrations/0004_surveyreportupload.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| # Generated by Django 3.2.16 on 2023-02-01 15:16 | ||
|
|
||
| from django.db import migrations, models | ||
| import django.db.models.deletion | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('survey_report', '0003_add_state_field_and_add_default_values_to_fields'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.CreateModel( | ||
| name='SurveyReportUpload', | ||
| fields=[ | ||
| ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
| ('sent_at', models.DateTimeField(auto_now=True, help_text='Date when the report was sent to external api.')), | ||
| ('status_code', models.IntegerField(help_text='Request status code.')), | ||
| ('request_details', models.CharField(blank=True, help_text='Information about the send request.', max_length=255, null=True)), | ||
| ('report', models.ForeignKey(help_text='The report that was sent.', on_delete=django.db.models.deletion.CASCADE, to='survey_report.surveyreport')), | ||
| ], | ||
| ), | ||
| ] |
20 changes: 20 additions & 0 deletions
20
openedx/features/survey_report/migrations/0005_surveyreportanonymoussiteid.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| # Generated by Django 3.2.16 on 2023-02-10 15:45 | ||
|
|
||
| from django.db import migrations, models | ||
| import uuid | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('survey_report', '0004_surveyreportupload'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.CreateModel( | ||
| name='SurveyReportAnonymousSiteID', | ||
| fields=[ | ||
| ('id', models.UUIDField(default=uuid.uuid4, editable=False, primary_key=True, serialize=False)), | ||
| ], | ||
| ), | ||
| ] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ormsbee do you have something different in mind for sending this report?
We have explored things like having a zappier that formats and sends to google docs, but we are open to anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a real URL enabled by default (if one exists yet), and a timeout on this request so it doesn't block for too long. But other than that, this seems fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an endpoint to send to at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ormsbee No, we don't have an endpoint for it, @felipemontoya some approach for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can produce a Zappier webhook and we leave that as the default for now.
https://help.zapier.com/hc/en-us/articles/8496326446989
Webhooks are available only in paid plans. I can put it in the edunext account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felipemontoya @jmakowski1123 @e0d I added the site name in a new field and I added a setting to make the site name anonymous.
new survey report file: https://docs.google.com/spreadsheets/d/1PWhWZ0XN6tEo8xhDh3kTSuGMrEYAE6tDBQu2kdF8tZA/edit?usp=sharing
setting: ANONYMOUS_SURVEY_REPORT is False by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the hash computed? Ideally it would be a consistent value -- it wouldn't change if the secret key was updated for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@e0d I'm using a sha256 from the hashlib so the hash will be always the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the risk of bikeshedding on this one, if we really want to have a fixed ID that each site carries for anonymization purposes, please make it so that the value is randomly generated and stored in the database. A simple hash of the base URL is too easy to figure out because there are relatively few Open edX sites out there, and we can run SHA256 against all of them. Salting doesn't help because either the salt is randomly generated per-site (in which case it might as well be the anonymous value), or it's linked against some secret that might change.
We've walked up and down this path a couple of times with anonymous user IDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ormsbee I tough 2 difference options for this:
Setting: We can define a new setting to save the ID, the problem with this is that the ID could be changed by the user whenever he wants, so maybe is not the best idea.
Model: We can create a new model just to save the ID, this will help us to persist the ID without problems and the ID will never change, we can use a get_or_create in the send method.
What do you think?