-
Notifications
You must be signed in to change notification settings - Fork 21
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
[WOR-1821] Add requester pays to settings modal #5111
Conversation
} as RequesterPaysSetting, | ||
], | ||
otherSettings | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarifying the overall behavior (partly for my own benefit):
- Eventing only happens if the new setting is different from the old one
- However, the array of new settings given to Rawls will include the new setting regardless of whether it is the same as the old one...
- ...unless the old one doesn't exist (i.e. hasn't been set yet) and the new one is equivalent to the "default" setting of this type, in which case it will not be included in the new settings
d0368f8
to
e4a3854
Compare
@@ -103,6 +109,10 @@ const SettingsModal = (props: SettingsModalProps): ReactNode => { | |||
return undefined; | |||
}; | |||
|
|||
const getRequesterPaysSetting = (settings: WorkspaceSetting[]): RequesterPaysSetting | undefined => { | |||
return settings.find((setting: WorkspaceSetting) => isRequesterPaysSetting(setting)) as RequesterPaysSetting; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced filter
+ index with find
for requester pays getter.
@@ -196,13 +211,26 @@ const SettingsModal = (props: SettingsModalProps): ReactNode => { | |||
) { | |||
// If the bucket had no soft delete setting before, and the current one is the default retention, don't event. | |||
} else if (!_.isEqual(originalSoftDeleteSetting, newSoftDeleteSetting)) { | |||
// Event if an explicit setting existed before and it changed. | |||
// Event if the setting changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(changed the wording here--it also events if no explicit setting existed before and the new setting is non-default)
I am not a fan of the description text here. It doesn't explain the impact to the user. Google's text is much clearer:
|
Quality Gate passedIssues Measures |
Jira Ticket: https://broadworkbench.atlassian.net/browse/1821
Extends the workspace settings modal to include a new section for Requester Pays. The setting is represented by a simple toggle switch. This allows users to set requester pays on their workspaces without contacting Support.
Other setting types are implemented to be tolerant of receiving multiple settings of that type when getting the workspace settings. Requester pays is a more unambiguous on/off feature and it would not make sense for there to be multiple to preserve. This implementation still wouldn't error out if there were multiple requester pays settings, but it would silently discard them.
WOR-1456.mov