added map struct for external ids #320
Conversation
There was a problem hiding this comment.
I'm generally fine with this change, because it would open the platform for further external integrations. However supporting legacy (keeping the legacy external_id) state is always a painful process which at some point produces problems in the near future. I would like to keep such support away as much as possible unless we have rock solid reason for the oposit.
@shurwit plrease feel free to add anything if I miss something.
| ID *string `json:"id"` // membership id | ||
| GroupIDs []string `json:"group_ids"` // list of group ids | ||
| UserID *string `json:"user_id"` // core user id | ||
| UserIDs []string `json:"user_ids"` // core user ids | ||
| ExternalID *string `json:"external_id"` // core user external id | ||
| NetID *string `json:"net_id"` // core user net id | ||
| Name *string `json:"name"` // member's name | ||
| Statuses []string `json:"statuses"` // lest of membership statuses | ||
| Offset *int64 `json:"offset"` // result offset | ||
| Limit *int64 `json:"limit"` // result limit | ||
| ExternalIDs map[string]string `json:"external_ids` //Map of external ids |
There was a problem hiding this comment.
If we gonna support a map wih key value pairs for all external ids, then I would highly appriaciate to take out the legacy field and make a migration.
| @@ -452,6 +453,7 @@ func (sa *Adapter) CreateGroup(clientID string, current *model.User, group *mode | |||
| castedMemberships = append(castedMemberships, membership) | |||
| } | |||
| } else if current != nil { | |||
| externalIDs := map[string]string{"netID": current.NetID, "externalID": current.ExternalID} | |||
There was a problem hiding this comment.
Please use under scores instead of camel names. Camel names are not wrong approach. The idea is just to keep common standard within the project.
There was a problem hiding this comment.
Also in this way of thoughts externalID is better to be renamed to uin, uiuc_uin if we gonna support more such attributes and external ids...
CC @shurwit
| @@ -1122,6 +1123,7 @@ func (h *ApisHandler) CreateMember(clientID string, current *model.User, w http. | |||
| return | |||
| } | |||
|
|
|||
| externalIDs := map[string]string{"netID": current.NetID, "externalID": current.ExternalID} | |||
Description
Please provide a summary of the pull request and the issue it resolves. Please add necessary details, context, dependencies, explanation of when review is needed (see next section), etc.
Fix external ID handling
Resolves #239
Review Time Estimate
Please give your idea of how soon this pull request needs to be reviewed by selecting one of the options below. This can be based on the criticality of the issue at hand and/or other relevant factors.
Type of changes
Please select a relevant option:
Checklist:
Please select all applicable options: