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.

Sign up for GitHub

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

Jump to bottom

feat: added automatic hiding functionality for the survey report banner when clicking on the dismiss button #34914

Merged

ormsbee merged 3 commits into openedx:master from eduNEXT:ase/added-dismiss-functionality-survey-report

Jun 19, 2024

Contributor

Asespinel commented Jun 5, 2024

Description

This PR adds a basic functionality to automatically hide the survey report banner when the dismiss button is clicked for one month depending on the user that clicked on the button. This could be changed in the future if requested. We use the localStorage to check when the Dismiss button was clicked so we can hide the banner from the admin site.

Testing instructions

Create 2 admin users and check the admin site separetly.
With the first admin click on the Dismiss button and if you refresh the page, you should see how the banner dissapears after a short delay
The second admin should see the banner appear normally
Afterwards, if you click on the button with the second admin, the banner should hide automatically as well
Is important to note that this dismiss variable is located in the local storage and you can clear it in your browser if necesarry if you want to do further testing

openedx-webhooks commented Jun 5, 2024

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:

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 added the open-source-contribution label

mariajgrimaldi changed the title ~~feat: added automatic hiding functionality for the survey reprot banner when clicking on the dismiss button~~ feat: added automatic hiding functionality for the survey report banner when clicking on the dismiss button

BryanttV reviewed

View reviewed changes

openedx/features/survey_report/static/survey_report/js/admin_banner.js Outdated

Comment on lines 20 to 21

    
                var bannerDismissalTime = localStorage.getItem('bannerDismissalTime_' + userId);

                var bannerExpirationTime = localStorage.getItem('bannerExpirationTime_' + userId);

Contributor

BryanttV Jun 5, 2024

What would happen if we access from incognito mode?

Contributor Author

Asespinel Jun 6, 2024

If we access from incognito, the localstorage woulb be empty. But as long as the incognito tab remains open the local storage for that tab should not be erased. Is there a lot of users that navigate using incognito? DO you think it might be a problem?

Contributor

BryanttV Jun 6, 2024

I understand. From my side I think there is no problem with it working that way.

openedx/features/survey_report/static/survey_report/js/admin_banner.js Outdated

    
                    // Store the dismissal time in milliseconds

                    var dismissalTime = new Date().getTime();

                    // Calculate the expiration time (1 month in milliseconds)

                    var expirationTime = dismissalTime + (30 * 24 * 60 * 60 * 1000); // 30 days

Contributor

BryanttV Jun 5, 2024

Would it be useful to be able to configure a customized number of days/months?

Contributor Author

Asespinel Jun 6, 2024

Initially we thought it could be a set amount for everybody. I don't think it would be that useful but I'm open to suggestions.

Contributor

BryanttV Jun 6, 2024

If we are going to leave a default time, I believe that one month is adequate and reasonable. But maybe providing the possibility that this time is customizable at the platform level using a setting is also good. Either option would be fine for me.

openedx/features/survey_report/static/survey_report/js/admin_banner.js Outdated

    
            @@ -4,8 +4,26 @@ $(document).ready(function(){
          
                    // If you want to do something after the slide-up, do it here.

                    // For example, you can hide the entire div:

                    // $(this).hide();

                    var userId = document.getElementById('userIdSurvey').value;

Contributor

BryanttV Jun 5, 2024

Could we use let instead of var?

Contributor Author

Asespinel Jun 6, 2024

Sure I'll change it right away

BryanttV reviewed

View reviewed changes

openedx/features/survey_report/static/survey_report/js/admin_banner.js Outdated

    
                    // Calculate the expiration time (1 month in milliseconds)

                    var expirationTime = dismissalTime + (30 * 24 * 60 * 60 * 1000); // 30 days

                    // Store the dismissal time in the browser's local storage

                    localStorage.setItem('bannerDismissalTime_' + userId, dismissalTime);

Contributor

BryanttV Jun 5, 2024

Is it necessary to store this variable?

Contributor Author

Asespinel Jun 6, 2024

Yes since, it would be different from user to user and it would serve as comparation with the other bannerExpirationTime variable

Contributor Author

Asespinel Jun 7, 2024

I also used the varaible as a flag to check for the first case where a survey report hasn't been sent and the button hasn't been pressed

BryanttV reviewed

View reviewed changes

openedx/features/survey_report/static/survey_report/js/admin_banner.js Outdated

    
                var currentTime = new Date().getTime();

                if (bannerDismissalTime && bannerExpirationTime && currentTime < bannerExpirationTime) {

                  // Banner was dismissed and it's still within the expiration period, so hide it

                  $('#originalContent').hide();

Contributor

BryanttV Jun 5, 2024

I tested the functionality and although it does hide the survey when I press the Dismiss button, every time I reload the page it shows and hides it quickly (milliseconds). Is it possible that the survey is never shown?

Contributor Author

Asespinel Jun 6, 2024

To do that we would have to add the display: none property on the banner and toggle it with javascript as well when the document loads. It would have the same effect when the banner should appear.

Contributor

BryanttV Jun 6, 2024

Perfect, it would be nice if it could be done.


          feat: added hiding feature for the survey report banner

b46e8ff

Asespinel force-pushed the ase/added-dismiss-functionality-survey-report branch from f33600c to b46e8ff Compare

June 6, 2024 02:08


          refactor: changed survey report to be initially hidden

f6c20f5

Contributor Author

Asespinel commented Jun 11, 2024

@mariajgrimaldi When you have the time could you review this PR?

Contributor

Alec4r commented Jun 11, 2024

I have check the code and I think it's okay, but I think we could re factorize it and make it clean with some auxiliary functions.

Remind that if you need to set a lot of "code comments" could be a signal of your code it is not clear enough.

We could use some auxiliary functions to get the user id, the current time, set the localstorage, and validate if we should show or hide the banner.


          refactor: added some functions for better code readability

c3ee72a

Contributor Author

Asespinel commented Jun 12, 2024

Thanks for the suggested changes @Alec4r. I think this PR is ready to be reviewed. Can you check it when you have the chance @felipemontoya @mariajgrimaldi @ormsbee ?

ormsbee approved these changes

View reviewed changes

Contributor

ormsbee left a comment

Please squash your commits, and make the commit message better describe why this change was made. I'll merge and do the redwood backport.

felipemontoya reviewed

View reviewed changes

openedx/features/survey_report/templates/survey_report/admin_banner.html

                  <button id="dismissButton" type="button" style="background-color:var(--close-button-bg); color: var(--button-fg); border: none; border-radius: 4px; padding: 10px 20px; margin-right: 10px; cursor: pointer;">Dismiss</button>
                   <form id='survey_report_form' method="POST" action="/survey_report/generate_report" style="margin: 0; padding: 0;">
                       {% csrf_token %}
+                      <input type="hidden" id="userIdSurvey" value="{{ user.id }}">

Member

felipemontoya Jun 12, 2024

everything else being the same, could we change this to use the anonymous ID?
You could easily use

edx-platform/common/djangoapps/util/keyword_substitution.py

Line 32 in f256684

return anonymous_id_for_user(user, None)

Contributor

ormsbee Jun 18, 2024

@felipemontoya: Unless you have a strong objection here, I'm inclined to merge this as-is, so that we can still try to squeeze this into Redwood. Is that okay with you?

Member

felipemontoya Jun 18, 2024

Yeah, I didn't want to leave the sequential ID stored there, but this only happens for a very small subset of users. We can go ahead and merge

Member

mariajgrimaldi commented Jun 12, 2024

I was going to review this but you folks have it covered. Thanks!

felipemontoya approved these changes

View reviewed changes

Member

felipemontoya left a comment

I belive we can merge this as is so that is makes it into redwood

ormsbee merged commit beb7aee into openedx:master

46 checks passed

openedx-webhooks commented Jun 19, 2024

@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.

ormsbee pushed a commit to ormsbee/edx-platform that referenced this pull request


          feat: hide the survey report banner for a month after clicking the di…

23af936

…smiss button (openedx#34914)

This hides the survey report banner from the Django Admin for a
particular user for one month after they click on the "dismiss" button.
This is done completely on the client side using localStorage, so the
same user could see the banner again if they're logging in with a
different browser.

ormsbee mentioned this pull request

feat: hide the survey report banner for a month after clicking the dismiss button #35010

Merged

Contributor

edx-pipeline-bot commented Jun 19, 2024

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

ormsbee pushed a commit that referenced this pull request


          feat: hide the survey report banner for a month after clicking the di…

aa70fea

…smiss button (#34914)

This hides the survey report banner from the Django Admin for a
particular user for one month after they click on the "dismiss" button.
This is done completely on the client side using localStorage, so the
same user could see the banner again if they're logging in with a
different browser.

Contributor

edx-pipeline-bot commented Jun 20, 2024

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment

Contributor

edx-pipeline-bot commented Jun 20, 2024

2U Release Notice: This PR has been deployed to the edX production environment.

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

Labels

open-source-contribution