Re-enabled updating encrypted custom fields via API [sc-41465] #14602
Conversation
PR Summary
|
| public function encrypted_field() | ||
| { | ||
| return $this->state(function () { | ||
| $field = CustomField::factory()->testEncrypted()->create(); // TODO - having to create and then 'find' the thing you just created is WEIRD |
There was a problem hiding this comment.
I'm going to see if I can help make these a little clearer.
snipe
changed the title
|
@marcusmoore I think I made just about all of the choices you asked for - feel free to take another look, or if you want to 'fix' the factory thing or point me the right way to do that - that's fine too. Also, if you think we should merge this as-is and do a Shortcut writeup for the improvements, we can do that too. |
|
@uberbrady I'd rather have the factories tightened up before merging. I'll work on that. I'll also see what's up with the MySQL test failures. |
|
@uberbrady I totally forgot I've been down this path with testing custom fields before. MySQL tests are failing because the creation of a custom field runs a migration and Laravel's tests are being run in a transaction which doesn't like that... We could do some toggling where tests for custom fields are only run on the sqlite driver but I really don’t want to segment our tests like that. |
| $response = $this->actingAsForApi($normal_user) | ||
| ->patchJson(route('api.assets.update', $asset->id), [ | ||
| $field->db_column_name() => 'Some Other Value Entirely!' | ||
| ->postJson(route('api.assets.store'), [ |
There was a problem hiding this comment.
This Patch -> Post thing, is that important? Or is it pretty much the same thing? Or are we talking about, maybe, two different things - updating an existing thing versus making a new thing?
There was a problem hiding this comment.
Or are we talking about, maybe, two different things - updating an existing thing versus making a new thing?
Yeah that's pretty much it. Our docs (and http standards I think) say POST for creating a new thing and PATCH for partially updating a thing. Side-note: we also support PUT for updating a thing fully (meaning all properties are present in the request).
|
Tests seem to be failing on this @marcusmoore @uberbrady |
|
@snipe Yeah...I don't think we'll be able to get around that unfortunately (see my last comment) 😞 I hate to say it but it's probably best to remove the test cases from this PR 🙁 |
|
We talked internally about possible ways of not pulling the tests entirely, and the change that I pushed up later adds some code to mark a test as 'incomplete' if the test database driver is 'mysql' - so you can still run the tests locally against sqlite, but they won't completely explode on a GH action in a PR on the 'server side'. While not ideal (you don't want to ever have to say "if this driver is mysql..." in a test), it's at least half-there, and maybe at some point in the future we can get all-there. @marcusmoore was able to clean up my fixes into something a little more systemic, which was good! |
|
Gonna give this another review pass. There is one todo I added that I might be able to easily handle. |
|
Note: I think it'll be a good idea to review/merge #14458 and then apply this change on top of it. Looking at the changed lines in the controllers I think the merge conflicts will be easier to handle in this PR than that one. |
There was a problem hiding this comment.
I removed my todo since it wasn't holding up this PR.
This looks good but remember that it'll probably be easier to get #14458 in first and then add this PR on top of it.
Some recent tweaks to the API code made it so that updates to encrypted custom fields were not correctly being encrypted, and, thus, not decrypted either.
This fixes that, and adds new tests to cover those possibilities.
Additionally, we decided to add some new parts - such as when you try to update a custom field, but fail, because you don't have "admin" permissions. Now we still allow the request to succeed, but add a warning in the message. Tests now cover this possibility.
The one weird thing here is that there were TWO return statements in the API - one that, obviously, fired. And one that was being ignored. I decided to just simply delete the second one, since it was not actually being run, ever.
I think there are a lot of inefficiencies with how I've done the testing, and will probably tap @marcusmoore for some help on more Laravel-esque ways of setting those test factories up in a bit of a less-janky fashion going forward. But I'd still go with this for now, as it seems like it's more in the right direction.