NO TICKET: Coverage and Feature support#83
Conversation
Codecov Report❌ Patch coverage is
|
| group_location_data: CreateGroupThing, | ||
| session: session_dependency, | ||
| user: admin_dependency, |
There was a problem hiding this comment.
In my update to the contact router I've named the endpoint /thing-association to clarify what is the contact is being associated with. That could be done here, too.
Also, in our standup today I spoke about how creating an association can be done by defining the group_id in the path or query parameters (in our discussion I talked about the contact router). I decided to go with path parameters (e.g. /group/{group_id}/thing-association), but the group_id can just as well be defined in the payload. Should that be the case here, or should that change in the contact router? Either way, a decision should be made and should be implemented consistently consistent across the API.
There was a problem hiding this comment.
Now that I think about it, though, most POST endpoints let the user-defined parameters be defined in the payload (which group_id and contact_id are)...
| return adder(session, Group, group_data, user=user) | ||
|
|
||
|
|
||
| @router.post( |
There was a problem hiding this comment.
Updates look good, but what are your thoughts on using /thing-association to be explicit about which table this association references? It may be a moot point here since there's only one association with a group (thing), but if other tables have multiple associations it'd be good to have clear nomenclature and then keep a consistent style throughout the API.
There was a problem hiding this comment.
I looked through the database and this is relevant for the author table (AuthorContactAssociation, AuthorPublicationAssociation), so if we decide on a nomenclature now I'll implement it throughout and write it up in the API discussion (which will turn into ADR)
(may also be relevant to others, but this showed up first in my search)
There was a problem hiding this comment.
Per our discussion today, comment out the /association endpoints. We may not want users to directly interact with these tables; that will be determined in conversations with AMP.
Should the responses have lists of the associations, at least their IDs? So GroupResponse could also have
associations: dict[str, List[int]]e.g.
associations: {"thing": [1, 2, 3]}if that group is associated with things 1, 2, and 3. This allows multiple associations to exist and be returned (like the authors)
Or keep the GET endpoints (and name their associations, like /group/thing-association
|
Another thought... proceed with commenting out all |
…n getting individual asset or thing_id is set
3 participants
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?