batches: add SSBC support for organisations#42131
Conversation
This reverts commit 4c1d0c5.
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff cc07fd8...a91ac89.
|
| "testing" | ||
|
|
||
| "github.com/keegancsmith/sqlf" | ||
| "github.com/stretchr/testify/require" |
There was a problem hiding this comment.
I've seen a number of patterns for assertion within the codebase but this is new. Is there a difference between require.NoError and assert.NoError
There was a problem hiding this comment.
I see require and assert the same way as t.Fatal versus t.Error, respectively (and, indeed, that's what they ultimately map to under the hood). Errors from fixture setup or teardown I would tend to handle with require, since there's usually no sensible way to continue, whereas error from assertions related to the actual test I would tend to use assert for, since there may be utility in continuing to run the test case.
There was a problem hiding this comment.
Yep! I use require when there should never be an error (set setups). If an error occurs, there is no reason to continue since something is super broke. require will stop a test in its tracks, but assert will let the test continue even is the assertion fails (that way you can see all the assertion failures at once instead one at a time)
…e.tsx Co-authored-by: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com>
|
I agree, I think omitting the buttons is better than disabling them. Thanks for the fix @LawnGnome. Do you mind adding the comment summary descriptor for |
Sure thing! Added. |
This PR adds full support for creating and applying batch changes within organisations from SSBC. The vast majority of the work here was around our permission handling, rather than anything to do with SSBC or the UI specifically.
Fixes #36536.
Permissions
As a refresher (because I certainly needed one), this is how permissions are documented to work in Batch Changes.
I discovered that our organisation handling was basically broken when querying batch changes that the user has admin rights to — batch changes owned by organisations that the user belongs to would never be returned. This affected both client and server side batch change functionality, and has been fixed.
Fixing this broke a number of tests that didn't set up full user, organisation, and user/org membership fixtures. These tests have been updated to set up more realistic environments.
Fixing that made me realise how hard some of the batch change store tests were to update, since their state was essentially linked to specific
csindices doing magic things upon creation. I made that creation explicit, rather than implicit, and updated tests that needed updating from there. (I chose not to do a full conversion of those tests to useassertandrequire, but did make spot updates as I rewrote specific subtests.)One remaining issue here is that (non-admin) users belonging to an organisation may not be able to see all executions of a batch change in that organisation — right now, they only have access to their own executions. On balance, this might be the right thing to do, but input welcome there.
UI changes
This PR reverted #37187 to reinstate a usable namespace selector when creating a batch change from the UI. I took the opportunity to simplify the
NamespaceSelectorprop types along the way, since our cases are now somewhat simpler.While testing this work, I also realised that there were a few potential banners on the batch change details page that are unactionable if the user doesn't have admin access to that batch change, so I've removed those in that case.
One final change that I would like feedback on is that users who cannot administer a batch change will now not see the Edit and Close buttons on the batch change. The alternative here would be to disable them with tooltips, but showing them at all feels a bit wrong to me. Thoughts?
Test plan
I believe I've tested all the possible combinations here (site admin in an org, site admin not in an org but using their site admin powers (maybe) responsibly, user in an org, user not in an org), and have added what I think are reasonable unit tests covering this at the resolver and store levels.