-
Notifications
You must be signed in to change notification settings - Fork 183
[policies] Add infrastructure to track user decisions to policies #9818
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
base: main
Are you sure you want to change the base?
[policies] Add infrastructure to track user decisions to policies #9818
Conversation
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 like the idea of this and did a first pass at skimming it. I think the structure should be reworked.
In addition to the changes requested, on a high level I think it would make sense to have a "Policy" class to group the behaviour together rather than everything being ad-hoc strings and Utility functions and htdocs files.
raisinbread/RB_files/RB_policies.sql
Outdated
SET FOREIGN_KEY_CHECKS=0; | ||
TRUNCATE TABLE `policies`; | ||
LOCK TABLES `policies` WRITE; | ||
INSERT INTO `policies` (`PolicyID`, `Name`, `Version`, `ModuleID`, `PolicyRenewalTime`, `PolicyRenewalTimeUnit`, `Content`, `SwalTitle`, `HeaderButton`, `HeaderButtonText`, `Active`, `AcceptButtonText`, `DeclineButtonText`, `CreatedAt`, `UpdatedAt`) VALUES (1,'dataquery_example','v1',49,7,'D','By using this Data Query Tool, you acknowledge that you know it is in beta and may not work as expected. You also agree to use it responsibly and not to misuse the data.','Terms Of Use','Y','Terms Of Use','Y','Yes, I accept','Decline','2025-05-28 10:54:21','2025-05-28 10:54:21'); |
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'm confused by the fact that the example in the database is for the data query tool but the code being changed is for request 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.
There are two example policies. The one for the login page is right under this.
The one for dataquery does not require any edits to the module as it is inherited from NDB_Page and will work the same for all pages / modules. So any module just needs to have an entry in policies for it to show up.
I had to make changes to request account to enforce that if there is a policy there it is enforced where you have to view it to request an 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.
To clarify this a bit further as well, as mentioned there are two example policies in this PR.
- A pop-up policy for the DQT that both pop-up when the module is opened, but is also re-openable whenever you press on Terms of Use in the header while the module is open. This behaviour is easily reproducible in any module just through sql.
- A policy that is tracked on the login page when a user is requesting an account. It's a different use case than the regular pop-up policies since we need a button in request account rather than having it automatically pop-up or having a button in the header. So for this one we need to manually add the PolicyButton, versus the other implementation where the policy is enforced / added through SQL.
@jeffersoncasimir let me know if you have time for this one, else re-assign it to me. |
@regisoc I don't think this is ready for review. Not passing tests, no changes since previous review and likely needs rebase |
@jeffersoncasimir sorry about that -- you're right it went off my radar when I went on vacation. I'll fix it up and then request a review! Thanks |
@jeffersoncasimir thank you for your patience -- it's ready for review now! |
…nt_Tracking_Infrastructure
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.
Works nicely. Likely ready for approval after revision.
Please make sure to add policy_tracker
to the modules
table in the patch, the -01 schema, and RB.
My interpretation of the testing instructions is that it acts as somewhat of a safeguard to a module, but it doesn't re-prompt when declined, and it can easily be bypassed. I can decline the RB dqt prompt, be sent the home page, but then go back to the dqt and use it normally and unprompted, as if nothing happened. Is it intended do a be a bit more consequential?
@jeffersoncasimir Made all the requested changes except for changing the Date Interval units (see comment). |
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.
Works as expected!
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 don't think this was meant to be added
if (onClickPolicy) { | ||
return <a | ||
className="hidden-xs hidden-sm" | ||
id="onClickPolicyButton" |
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.
id="onClickPolicyButton" | |
id="on-click-policy-button" |
@@ -73,6 +73,16 @@ | |||
toggleIcon.classList.add('glyphicon-chevron-down'); | |||
} | |||
}); | |||
|
|||
headerPolicyRoot = ReactDOM.createRoot( | |||
document.getElementById("headerPolicyButton") |
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.
document.getElementById("headerPolicyButton") | |
document.getElementById("header-policy-button") |
@@ -143,6 +153,9 @@ | |||
</a> | |||
</li> | |||
{/if} | |||
{if $header_policy} | |||
<li class="hidden-xs hidden-sm" id="headerPolicyButton"></li> |
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.
<li class="hidden-xs hidden-sm" id="headerPolicyButton"></li> | |
<li class="hidden-xs hidden-sm" id="header-policy-button"></li> |
Brief summary of changes
If there is a policy for the login module
If there is a policy for a non-anonymous module, and HeaderButton = 'Y'
Testing instructions (if applicable)
SELECT * FROM POLICIES
select * from user_policy_decision
Link(s) to related issue(s)