Conversation
Problem: `set-field` route was used both for changing visibility and setting fields. This resulted in ambiguous behavior in case of missing contents and visibility options. Solution: require contents for setting field and allow changing visibility for convenience. Add `set-field-visibility` request which doesn't require field contents.
|
Seems in issue description field visibility should be a query parametr
|
Yes but that was an initial proposal, we later decided to split our queries into I don't want the |
|
Ok, IMO it would be more logical to make it query. JSON looks like overshot. And seems nothing chain us to JSON format |
Yeah, will redo. |
|
@Kariel-Myrr I moved visibility from JSON body to the query string. Now a request looks like |
| ] | ||
| |] | ||
| -- unit_set_field_missing_field_contents :: IO () | ||
| -- TODO: add test that expects an error in case of missing contents |
There was a problem hiding this comment.
Resolved now, you can check how I did it, I'm not sure it is the proper way
| void $ setField "dir/entry" "private-field" Nothing "contents" | ||
| void $ setField "dir/entry" "public-field" Nothing "contents" | ||
| changeFieldVisibility "dir/entry" "private-field" "private" | ||
| changeFieldVisibility "dir/entry" "public-field" "public" |
There was a problem hiding this comment.
Why there is two checks in one test?
If we're expecting to fail at once = one way (only private/public)
If it could fail on one, but not on second variant seems as two separate tests
I know that they're just doing the same thing, but we aren't short on test space)
tests/server-integration/Utils.hs
Outdated
|
|
||
| setField :: Text -> Text -> Text -> IO (JsonResponse Value) | ||
| setField path name contents = | ||
| setField :: Text -> Text -> Maybe Bool -> Text -> IO (JsonResponse Value) |
There was a problem hiding this comment.
Why not straight Maybe Visibility
There was a problem hiding this comment.
I think I will change Bool to Text to unify the style with function changeFieldVisibility
sancho20021
force-pushed
the
sancho20021/#119-refactor-set-field-endpoint
branch
from
72d11b0 to
e5e088d
Compare
|
This PR is closed due to existing #130. |
2 participants
Description
Problem:
set-fieldroute was used both for changing visibilityand setting new fields. This resulted in additional handling of the
situation where both field contents and visibility option are missing.
Solution: require contents for setting field and allow changing
visibility for convenience. Add
set-field-visibilityrequestwhich modifies only visibility.
Question?
Currently in
set-field-visibilityI pass the value in the body of the request.Maybe it would be better to pass it in the request string itself:
I don't know.
Related issue(s)
Fixed part of #119. Refactor of CLI API still needed.
✅ Checklist for your Pull Request
Related changes (conditional)
Tests
silently reappearing again.
Documentation
Stylistic guide (mandatory)