Skip to content
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

Basic encrypted custom fields support for checkboxes - needs testing, tests, and love #14721

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

uberbrady
Copy link
Collaborator

Sometimes some customers might choose to encrypt custom fields that are just a collection of checkboxes. We should be able to support that.

Copy link

what-the-diff bot commented May 15, 2024

PR Summary

  • Introduction of a New Variable in Api/AssetsController
    A new variable $problems_updating_encrypted_custom_fields has been introduced in a key file. This will help accurately track if there are any issues when updating encrypted fields.

  • Added Conditional Check in Api/AssetsController
    Based on the value of $problems_updating_encrypted_custom_fields, the system will now give different responses. This allows for better error handling and data accuracy.

  • Introduction of a New Variable in Assets/AssetsController
    This is similar to the change made in Api/AssetsController. We're maintaining consistency across different parts of the application for better tracking of encrypted field update issues.

  • Added Conditional Check in Assets/AssetsController
    Like with Api/AssetsController, we want to have specific responses based on encrypted field update problems. This helps with maintaining data integrity and detailed error reporting.

  • Logging Enabled in Key Components
    We've added logging statements in ValidationServiceProvider.php and Assets/AssetsController.php. This helps us track behavior, catch any bugs or errors and maintain code quality over time.

  • Decrypt Before Validation
    A conditional check was added in ValidationServiceProvider.php to decrypt the field value before validation. This should ensure that our validations are done accurately.

  • Checkbox input modified
    In the custom fields form, we've changed the checkbox input to use the Helper::gracefulDecrypt function. This will aid in presenting the accurate state of a checkbox by decrypting its value beforehand.

@uberbrady uberbrady changed the title Basic encrypted custom fields support - needs testing, tests, and love Basic encrypted custom fields support for checkboxes - needs testing, tests, and love May 15, 2024
@spencerrlongg
Copy link
Collaborator

@uberbrady I got some initial tests for non-encrypted checkbox testing, and found a problem on the first test I started writing for encrypted checkbox. There's a note in the validator about it. From memory, this is the problem I had when I was writing this validator initially and why I started questioning the encrypted checkbox in the first place.

From what I remember, I was having problems consistently decrypting $options to get things to a place where I could do the array comparison for the validation.

I stopped when I remembered I couldn't really test it without going and re-enabling the feature on the front end to get an encrypted checkbox set up, so I'll get back to that in the morning. Or maybe later tonight if I can't sleep again.

@uberbrady
Copy link
Collaborator Author

Thank you for the tests! Let me see if there's anything I can help with.

@spencerrlongg
Copy link
Collaborator

DONT MERGE

Found a pretty significant issue with this that needs attention first. We can talk on Monday.

@spencerrlongg
Copy link
Collaborator

Alright, I think that was a false alarm, sorry - I was trying to check an asset out and was failing encrypted checkbox validation, but that's actually happening on dev, not this branch.

Seems like a backwards-compatibility issue if I've set encrypted checkbox data and switch back to dev - which makes sense.

@spencerrlongg spencerrlongg marked this pull request as ready for review May 20, 2024 18:08
@spencerrlongg spencerrlongg requested a review from snipe as a code owner May 20, 2024 18:08
@snipe
Copy link
Owner

snipe commented May 22, 2024

Probably slightly related to this: #14720

Tho based on internal conversations, the encryption part isn't really the issue here 😬 )

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

Successfully merging this pull request may close these issues.

None yet

3 participants