Skip to content

Issue 48490: Add auditing behavior param via new options param #66

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
merged 10 commits into from
Dec 28, 2023

Conversation

RosalineP
Copy link
Contributor

Rationale

See more details here. It is desired we add support for a couple of options passed to insert_rows, update_rows, and delete_rows. There are a notable amount of parameters that are supported on the server but not passed to these functions (around 8). As such, it seemed more maintable and elegant to add an 'options' paramater to these functions rather than passing all ~8 explicitly.

Related Pull Requests

Changes

  • Update insert_rows, update_rows, and delete_rows
  • Update change log

@RosalineP
Copy link
Contributor Author

RosalineP commented Dec 18, 2023

I considered updating select_rows() to take in an 'options' param too rather than the list of ~14 params and editing up the fn so that the payload is still well-formed, but it seemed likely unnecessary and backwards incompatible. Waffling though so happy to do so if we think it's a good idea.
Edit: Outdated comment, we are not using the 'options' paradigm

Copy link
Contributor

@labkey-alan labkey-alan left a comment

Choose a reason for hiding this comment

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

I am not 100% sure about the options dict approach, I have the same issues with it as you. I pinged Nick to get his thoughts, but I am probably leaning towards just adding them all as kwargs in order to better match our JS API.

Additionally, we should make some small changes to keep the supported Python versions to 3.7+.

RosalineP and others added 3 commits December 20, 2023 13:55
Co-authored-by: Alan Vezina <alanv@labkey.com>
@RosalineP
Copy link
Contributor Author

RosalineP commented Dec 21, 2023

ToDo:

  • Manual test plan
  • Double check 'transacted' parameter and whether it's respected for these operations

Copy link
Contributor

@labkey-alan labkey-alan left a comment

Choose a reason for hiding this comment

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

Looks good. You'll need to merge Nick's changes in before merging. He bumped the major version number, but I haven't released that yet, so all you'll need to do is put your release notes under the same version as Nick.

@RosalineP
Copy link
Contributor Author

Manual Test Plan
The following assumes that list1 is an autoInt list.

resp1 = api.query.insert_rows("lists", "list1", [{'Field1': 'one'}], skip_reselect_rows=False, transacted=False, audit_behavior=AuditBehavior.DETAILED, audit_user_comment="hi0")
print(resp1)
resp2 = api.query.insert_rows("lists", "list1", [{'Field1': 'one'}], skip_reselect_rows=True, transacted=False, audit_behavior=AuditBehavior.SUMMARY, audit_user_comment="hi1")
print(resp2)

Validate the difference in response lengths of resp1 and resp2.
In the audit log, under 'Query Update Events', validate the user comments and the (minor) difference in comments.
transacted can be confirmed by going to QueryController.java, line 4545 which checks json.optBoolean("transacted", true), and validating that transacted is false.

api.query.update_rows("lists", "list1", [{'Field1': 'two', 'Key': 1}], transacted=False, audit_behavior=AuditBehavior.DETAILED, audit_user_comment="hello0")
api.query.update_rows("lists", "list1", [{'Field1': 'three', 'Key': 1}], transacted=False, audit_behavior=AuditBehavior.SUMMARY, audit_user_comment="hello1")
api.query.update_rows("lists", "list1", [{'Field1': 'four', 'Key': 1}], transacted=False, audit_behavior=AuditBehavior.NONE, audit_user_comment="hello2")

Validate the audit log comments and user comments.
No audit log will appear for the final command.
Validate transacted as mentioned above.

api.query.delete_rows("lists", "list1", [{'Field1': 'four', 'Key': 1}], transacted=False, audit_behavior=AuditBehavior.DETAILED, audit_user_comment="comment1")
api.query.delete_rows("lists", "list1", [{'Field1': 'one', 'Key': 2}], transacted=False, audit_behavior=AuditBehavior.SUMMARY, audit_user_comment="comment2")

Validate audit log comment and user comments.
Validate transacted as mentioned above.

@RosalineP RosalineP merged commit 78289b4 into develop Dec 28, 2023
@RosalineP RosalineP deleted the fb_Issue48490 branch December 28, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants