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

fix/archive toggle no longer respects change request #6882

Merged
merged 2 commits into from
Apr 18, 2024

Conversation

sighphyre
Copy link
Member

@sighphyre sighphyre commented Apr 17, 2024

What

This changes the UI flow in archive toggle to ignore the SKIP_CHANGE_REQUEST permission. Salient change is in the second commit.

Background

Currently we have a bug where having the SKIP_CHANGE_REQUEST permission will force the UI to attempt to directly archive a toggle, however the backend does not respect this permission and raises an error. This means a user with SKIP_CHANGE_REQUEST can neither archive the toggle directly, nor add it to a change request.

This is caused by an asymmetry in the way permissions work for archiving a toggle; SKIP_CHANGE_REQUEST is an environment level permission but archiving a toggle is a project level permission since it affects every environment.

Alternatives considered

The obvious alternative here is to allow the backend to respect this permission. We decided not to pursue this path for two reasons (discussed with @sjaanus and @FredrikOseberg).

The first is danger - that this requires quite invasive changes to the way the backend handles permissions for SKIP_CHANGE_REQUEST that would potentially have knock effects on the entire change request flow, which is already quite complex.

The second is that the SKIP_CHANGE_REQUEST permission is built to serve a particular purpose - allow trusted users who are working on time sensitive tasks like on-call to get the velocity they need. Archiving a toggle should never be a thing that needs to be done in an emergency situation. Ultimately skipping change requests here doesn't make a lot of sense

Considerations for discussion

  • Does this need to be more obvious to the user? Currently I've left the dialog alone but this has the potential to be quite confusing to the user who may believe they should have the required permissions to skip the change request flow
  • Are these clean ups that need to be done here? I don't see any obvious dead code paths but I really don't know this feature all that well

Copy link

vercel bot commented Apr 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview Apr 17, 2024 9:57am
1 Ignored Deployment
Name Status Preview Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Apr 17, 2024 9:57am

@sjaanus
Copy link
Contributor

sjaanus commented Apr 17, 2024

Does this mean archiving must always go through a change request? Are there any changing coming in backend also?

@sighphyre
Copy link
Member Author

Does this mean archiving must always go through a change request?

Yes. This will always trigger a change request flow. The dialog always take you to creating a change request now. That means the API call that gets made is one to create the change request and not to delete the toggle

Are there any changing coming in backend also?

No change needed on the backend (tested manually but kinda hard to write a automated test for this)

Copy link
Contributor

@sjaanus sjaanus left a comment

Choose a reason for hiding this comment

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

LG

@sighphyre sighphyre merged commit 6b5cdc2 into main Apr 18, 2024
15 checks passed
@sighphyre sighphyre deleted the fix/archive-toggle-no-longer-respects-change-request branch April 18, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants