Added endpoint to patch attendee registration #407
Conversation
kgukevin
changed the title
kgukevin
changed the title
kgukevin
changed the title
kgukevin
changed the title
There was a problem hiding this comment.
Good start! I think we might need to change our strategy here a bit, so gonna nack it but let me know if you disagree/have a different approach. Sorry for the back and forth.
Some general tips regarding PRs:
- I noticed your second commit
0cc6c76has the same commit message as the first commit. I would either squash all the commits into one commit or make sure that your second commit has a different, useful message - No need to change the PR titles to make it perfect. As you can see every edit leaves an edit history in the PR and it makes the PR a little messy, and as long as the title makes general sense it's fine
- If the PR addresses/fixes an issue then you can reference that issue number from anywhere in this PR, whether it's the original PR message or any comments. For example, this PR is related to Add endpoint to patch user registration #234
common/datastore/validation.go
Outdated
| if len(params) == 1{ | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Could you explain what this change does?
Edit: ah I see you made this flag to skip the validation. I think we can avoid having to do this since we might need to change our strategy for this PR. See my other comment below
common/datastore/conversions.go
Outdated
| } | ||
| // else { | ||
| // data[field.Name] = getDefaultValue(field.Type) //removed for registration/attendee patch function | ||
| // } |
There was a problem hiding this comment.
Hm. Now that I think about it, I think we actually don't want to do this. Users currently expect the API to always return all fields, even if their value is empty/default. 1. We wouldn't want to introduce a change to this behavior without thinking about its implications on the user and 2. I think it is better to have an API that returns a consistent format.
So as for how to get around this issue: we can try to manually strip off fields from the user_registration when we pass it into the database patch function
| original_registration, err := service.GetUserRegistration(id) | ||
|
|
||
| if err != nil { | ||
| errors.WriteError(w, r, errors.DatabaseError(err.Error(), "Could not get user's original registration.")) | ||
| return | ||
| } | ||
|
|
||
| user_registration.Data["github"] = user_info.Username | ||
|
|
||
| user_registration.Data["createdAt"] = original_registration.Data["createdAt"] |
There was a problem hiding this comment.
We shouldn't need to re-insert the "github" or "createdAt" fields. We only need to change the "updatedAt" field + the fields the user provided in the request.
…ault value population
…elds with registration definition
|
Per offline discussion with @heesooy, the current strategy for the patch endpoint implementation was to use the raw request body as a map and compare it to the registration definition to prune unwanted excess fields before patching it to the database. This prevents default value population in the patch datastore to combat "erasing" unpopulated fields after patching while preserving the functionality for better consistency. ValidateNonEmpty was also created to accommodate for validating a patch (which is a subset of fields in the registration definition). Other strategies were discussed to combat default values (such as getting original registration from the database and updating it with the new data before patching back to the database) but the implemented strategy was preferred for database operation atomicity. |
Implements patch endpoint for attendee registration. Adds parameter to validate to allow override of form field requirements. Removes default value population from model conversions.