feat: added banner message for survey report - #33633
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @Asespinel! 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:
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. |
mphilbrick211
added
the
needs test run
|
Thanks for all the suggestions @dcoa I'll update the templates accordingly |
mphilbrick211
removed
the
needs test run
|
HI @Asespinel! Just flagging that there are some failing tests. |
ff01956 to
21e4e49
Compare
|
Thanks for the heads up @mphilbrick211 I'll check on them |
21e4e49 to
14b41d5
Compare
14b41d5 to
9778c83
Compare
|
We manually increase the number of queries to make the test pass; is there a better way? @ormsbee |
|
@MaferMazu: Incrementing the number like this is fine. The CCX test shouldn't be written to test query counts on an entire view load. Query counts should only be done on lower level things (like |
|
Could you help me with a review of this PR? @mariajgrimaldi @felipemontoya @ormsbee . I think everything is working as expected but any feedback is welcomed. Let me know your thoughts on the matter. |
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
Mostly questions about whether we can simplify the Admin template.
|
Hi @ormsbee I deleted the base.html file and made the changes acordingly to maje the code easier to read and understand. Please let me know if you have anymore suggestions |
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
@Asespinel: Thank you for the admin template simplifications–this looks much cleaner!
This looks good to me once commit messages are squashed. I have one suggestion, but it's optional.
| from .models import SurveyReport | ||
|
|
||
|
|
||
| def admin_extra_context(request): |
There was a problem hiding this comment.
Random thought (completely optional): Is it easy to check if this request is for an admin view and return {'show_survey_report_banner': False} if it's not an admin view (without having to check the database)? In that case, you might be able to remove the changes that require bumping the number of queries in the other tests.
There was a problem hiding this comment.
I implemented the change requested with this change we should revert back the query counts by 1 in all the tests? Or just on particular ones?
There was a problem hiding this comment.
We will know for sure once the tests run completely but my guess is that for all of them.
felipemontoya
left a comment
felipemontoya
left a comment
There was a problem hiding this comment.
Good work @Asespinel. I only have a minor request about the threshold
| The current treshhold to show the banner is one month but this can be redefined in the future | ||
|
|
||
| """ | ||
| months = 1 |
There was a problem hiding this comment.
We should move this to a setting so that it is configurable and also put a better default. Since we would like to get reports about twice a year, we could set the default to 6.
There was a problem hiding this comment.
Thanks for the feedback ioll change it to 6
| The current treshhold to show the banner is one month but this can be redefined in the future | ||
|
|
||
| """ | ||
| months = 6 |
There was a problem hiding this comment.
Thanks for the change, but this is still a constant. I meant configuring this so that it reads from a variable. Similar to what happens here.
| }); | ||
| </script> | ||
|
|
||
| {% endblock %} No newline at end of file |
felipemontoya
left a comment
felipemontoya
left a comment
There was a problem hiding this comment.
Thanks for the updates @Asespinel. I think this is good to be merged now.
|
Hello @ormsbee, @felipemontoya if there's no more changes needed. Could you help me with the squash so we can close this PR? |
|
@Asespinel 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
* feat: added banner message for survey report * refactor: addressed PR feedback and changes * fix: fixed styles on admin templates * refactor: changed script location to survey report block * chore: removed whitespaces and renamed the context processor files * feat: added banner message for survey report * refactor: separated survey report template from admin and deleted base template * refactor: changed months variable into a configurable setting --------- Co-authored-by: Maria Fernanda Magallanes Zubillaga <maria.magallanes@edunext.co> Co-authored-by: María Fernanda Magallanes <35668326+MaferMazu@users.noreply.github.com>
9 participants
This PR adds a custom banner message to the admin site. Right now the message only appears if a survey report hasn't been generated in 6 months but this threshold is configurable . Also this PR will send the report when it's generated as well.
How to test