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

Changes settings revert all to send DELETE on individual endpoint rather than PATCHing #10376

Merged

Conversation

mabashian
Copy link
Member

@mabashian mabashian commented Jun 7, 2021

SUMMARY

Resolves #3831

#3831 (comment) is a good explanation of the state of the new UI as well as the old. The Misc System settings were the most problematic part of this since that form pulled fields from both /api/v2/settings/system and /api/v2/settings/authentication. In order to revert these fields with a DELETE request I needed to split this form up into Miscellaneous System and Miscellaneous Authentication. The Activity Stream Settings also needed to be absorbed by the Miscellaneous System Settings.

All settings forms (except noted below) will now issue a DELETE request to revert all the fields for a particular settings subsection.

There was one form that I did not change the Revert All functionality on and that was the LDAP Settings form(s). The UI splits these fields out into 5 different subforms and reverting one (issuing a DELETE on /api/v2/settings/ldap) would revert all the ldap settings which I think is undesirable.

This fix has the added benefit of cleaning up future activity stream entries for reverting an entire settings category.

I also consulted with @wenottingham and @gamuniz on some fields that were present in the api but missing in the ui and I added those.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • UI

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@AlanCoding
Copy link
Member

I checked out this branch and poked around. Starting out I mucked with the jobs settings. Changing some, then reverting all.

Screenshot from 2021-06-08 10-17-16

Screenshot from 2021-06-08 10-17-23

This looks nice. I think there are good reasons to go with this.

The one qualifier I want to make here is that the PATCH will include all settings in the category ("jobs" category here). This leads to "creation" entries like this:

Screenshot from 2021-06-08 10-22-03

The default value of the "Default Project Update Timeout" is 0. So this is a junk entry. I do feel opinionated that we should address this. However, I think the proper place for the fix is API-side. I think we can get by without it for now, but we should file and issue for that if this merges.

Confirmed that LDAP categories behave like they always have. Those should be pretty stable and I'm not too worried about it.

I looked over the activity stream after mucking with some settings categories. There are a lot of entries, due to the default value issue I mentioned. Also, the deletion issues are pretty ugly, due to an open issue, #10360

@AlanCoding AlanCoding requested a review from jakemcdermott June 8, 2021 14:30
@AlanCoding
Copy link
Member

(you should get UI reviews too, I'm just considering the requests being made)

Copy link
Contributor

@tiagodread tiagodread left a comment

Choose a reason for hiding this comment

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

We are now making DELETE requests for all settings subforms properly and these events are being displayed as deleted in the activity stream.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

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.

The UI will PATCH to all instead of DELETE when reverting settings category to defaults
4 participants