Skip to content

feat: add SurveyReportUpload and add send report method - #31431

Merged
felipemontoya merged 18 commits into
openedx:masterfrom
eduNEXT:survey_report_send_report
Feb 21, 2023
Merged

feat: add SurveyReportUpload and add send report method#31431
felipemontoya merged 18 commits into
openedx:masterfrom
eduNEXT:survey_report_send_report

Conversation

@Alec4r

@Alec4r Alec4r commented Dec 11, 2022

Copy link
Copy Markdown
Contributor

Description:

This PR adds a new model SurveyReportUpload to save the status and details of the send request.

SurveyReport model (#31183).

ISSUE:
openedx/axim-engineering#349

APPROVED DESIGN:
https://docs.google.com/document/d/1tm1O5BQsAwekphI5IN9pBn8behQDZigXePLGqCl5he8

DECISIONS:

  • We decided add a new model to save all the information about each send request.
  • We decided send the survey report data from json to a API endpoint, I hope that tCrill supply this endpoint.
  • We decided that each report generated by cli will be send automatically, unless the user set the option --no-send in the command.

How to test

First, You should define the settings: SURVEY_REPORT_ENDPOINT with a valid url, you can use some Mock API Service like https://beeceptor.com/.

Then, with a dev environment running, you should run the command python manage.py lms generate_report or python manage.py lms generate_report --no-send.

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @Alec4r! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 11, 2022
@Alec4r

Alec4r commented Dec 12, 2022

Copy link
Copy Markdown
Contributor Author

@jalondonot @felipemontoya @ormsbee @jmakowski1123 @mariajgrimaldi We would appreciated your comments in this PR.

Comment thread openedx/features/survey_report/api.py Outdated

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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?

Comment thread openedx/features/survey_report/api.py Outdated
Comment thread openedx/features/survey_report/api.py Outdated
Comment thread openedx/features/survey_report/management/commands/generate_report.py Outdated
Comment thread openedx/features/survey_report/models.py Outdated
Comment thread openedx/features/survey_report/models.py Outdated
Comment thread openedx/features/survey_report/models.py Outdated
Comment thread openedx/features/survey_report/models.py Outdated
@Alec4r
Alec4r requested review from felipemontoya, mariajgrimaldi and ormsbee and removed request for felipemontoya, mariajgrimaldi and ormsbee January 2, 2023 13:44
Comment thread openedx/features/survey_report/management/commands/tests/test_generate_report.py Outdated
@mphilbrick211

Copy link
Copy Markdown

Per Jorge, flagging this for @e0d and @jmakowski1123 to weigh in on Dave's feedback before proceeding. Thanks!

Alec4r and others added 5 commits February 1, 2023 10:35
…ate_report.py

Co-authored-by: Maria Grimaldi <maria.grimaldi@edunext.co>
Co-authored-by: Maria Grimaldi <maria.grimaldi@edunext.co>
Co-authored-by: Maria Grimaldi <maria.grimaldi@edunext.co>
Co-authored-by: Maria Grimaldi <maria.grimaldi@edunext.co>
@mariajgrimaldi
mariajgrimaldi force-pushed the survey_report_send_report branch from a697c9d to 5a460de Compare February 1, 2023 15:59
@mphilbrick211 mphilbrick211 added the product review PR requires product review before merging label Feb 1, 2023
@mphilbrick211

Copy link
Copy Markdown

@e0d @mariajgrimaldi @jmakowski1123 @felipemontoya @ormsbee - per Jorge, this is ready for another round of reviews. Please take a look! Thanks :)

@felipemontoya

Copy link
Copy Markdown
Member

@Alec4r going back to Dave's comments actually he already mentioned

please make it so that the value is randomly generated and stored in the database

This would be the option 2 where we store the value in a model. I think we should go that route and not the settings one. Writing the value in settings would require that we randomize the creation of that ID at the first deploy, which is out of our control from this work and also it would make it very easy to loose track of the generated ID and change it for cases where an install is being updated in it's technology. We have seen such cases when we are migrating old installs to something docker based.

@Alec4r
Alec4r force-pushed the survey_report_send_report branch from 46749c1 to c7ecb23 Compare February 10, 2023 16:02

@felipemontoya felipemontoya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks a lot @Alec4r. This would solve the last remaining issue with the format.

From my perspective we are ready with the code portion of this. +1. I'm leaving an approval here.

However the current zap in the production configuration belongs to edunext. I suppose we want to have a tCRIL zappier account for this right from the start.

@e0d, @jmakowski1123 is there something we can do to help with the creation of this zap? The instructions where provided by @Alec4r some time ago.

https://docs.google.com/document/d/1rLxVu5bC-XhEmKAGwF2FSyG6t8JVuVK26SS81kCgEmQ/edit

@mariajgrimaldi mariajgrimaldi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All my suggestions/questions were addressed so I'm good with this! Thank you!

@ormsbee

ormsbee commented Feb 17, 2023

Copy link
Copy Markdown
Contributor

@Alec4r: Is this the set of commits you want merged? We'd normally squash it to fewer commits, but the individual messages here seem pretty intentional.

@mariajgrimaldi, @felipemontoya: Anyhow, please merge (or squash+merge) at your discretion.

@e0d

e0d commented Feb 17, 2023

Copy link
Copy Markdown
Contributor

I've created the Zap. @felipemontoya has the URL for the endpoint. PM me in slack if you need it as well. The Zap should be published and live now.

@felipemontoya
felipemontoya merged commit 7f90b5d into openedx:master Feb 21, 2023
@openedx-webhooks

Copy link
Copy Markdown

@Alec4r 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

1 similar comment
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

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

Labels

open-source-contribution PR author is not from Axim or 2U product review PR requires product review before merging

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

9 participants