Feature/ego charts #255
Feature/ego charts #255
Conversation
|
I'm seeing the ego variables showing up in the settings panel, but getting nothing on the overview screen? |
Actually, I can't reproduce this. Closing the app and restarting seems to have fixed it, and now it works as expected. I wonder if it was just an issue with rehydrating the app state? |
There was a problem hiding this comment.
It works great 👍
I have a few questions about the code which may be nonsensical, hopefully useful.
One thing I wondered looking at these changes is if we should try to model the ego in the network as 'ego.ego' to make a lot of these transformations unified (would apply to other apps too) e.g.:
const network = {
node: {
person: { /* person config */ },
},
ego: {
ego: { /* ego config */ },
},
// ...
}| api.get('/protocols/:id/reports/option_buckets', (req, res, next) => { | ||
| const { variableNames = '', entityName = 'node' } = req.query; | ||
| this.reportDb.optionValueBuckets(req.params.id, variableNames.split(','), entityName) | ||
| api.post('/protocols/:id/reports/option_buckets', (req, res, next) => { |
There was a problem hiding this comment.
Why switch to post? Isn't the API currently RESTful?
There was a problem hiding this comment.
Before we were sending an array of variable names as part of the "get" request (e.g. [var1, var2, var3]) and assuming they were node variables and that types had no overlapping variables. I needed to separate those variables into ego variable, edge variables with types, and node variables with types (e.g. nodes: { type1: [var1, var2, var3], type2: [var1] }), and I couldn't send that complex of data with "get" as far as I knew.
There was a problem hiding this comment.
Okay seems like a good pragmatic decision. I think we should add a comment for posterity?
| @@ -9,8 +9,12 @@ import { mockProtocol } from '../../../../../config/jest/setupTestEnv'; | |||
|
|
|||
| jest.mock('../../../utils/adminApiClient', () => { | |||
| function MockApiClient() {} | |||
| MockApiClient.prototype.get = jest.fn().mockResolvedValue({ | |||
| buckets: { person: { distributionVariable: { 1: 4, 2: 5 } } }, | |||
| MockApiClient.prototype.post = jest.fn().mockResolvedValue({ | |||
There was a problem hiding this comment.
What are your thoughts on switching over to using jest snapshots? I think it makes these kinds of tests simpler to manage, at the expense of requiring more scrutiny for snapshot updates.
There was a problem hiding this comment.
I don't really mind one way or another. I don't think it is updated that often?
| if (!isDistributionVariable(def) || excludedSectionVariables.includes(def.name)) { | ||
| return; | ||
| } | ||
| const data = buckets[entityType] && buckets[entityType][variableName]; | ||
| const data = entityKey === 'ego' ? buckets && buckets[variableName] : buckets[entityType] && buckets[entityType][variableName]; |
There was a problem hiding this comment.
What about breaking this out into a couple of steps, maybe like:
const dataPath = entityKey === 'ego' ? [variableName] : [entityType, variableName];
const data = get(buckets, dataPath);There was a problem hiding this comment.
Good suggestion. I'll make this update.
There was a problem hiding this comment.
Made this update, and added a comment. I think that voids your review approval... :)
| @@ -57,6 +60,15 @@ const shapeBucketData = (transposedNodeCodebook, buckets, excludedChartVariables | |||
| return acc; | |||
| }, []); | |||
|
|
|||
| const shapeBucketData = (codebook, buckets, excludedChartVariables) => | |||
There was a problem hiding this comment.
Does this make 'ego' appear as a type? Could that conflict unexpectedly with a node with the same 'type' name?
Should we add a function to describe this function?
There was a problem hiding this comment.
It does make ego appear as a type. It wouldn't conflict because it is embedded within the entity as well (basically ego: { ego: [var1, var2] })
Yeah, I've wondered the same thing in other apps as well as here. Though ego doesn't really have a type, it could simplify things to structure it like it did. Is it just that is makes no logical sense to have a type for ego, or are there some things that are better because we have no ego type? I mean, it really does make no logical sense to have an ego type. But this is not the first time the code work arounds were kind of ridiculous because there is no ego type. |
There was a problem hiding this comment.
Yeah, I've wondered the same thing in other apps as well as here. Though ego doesn't really have a type, it could simplify things to structure it like it did. Is it just that is makes no logical sense to have a type for ego, or are there some things that are better because we have no ego type? I mean, it really does make no logical sense to have an ego type. But this is not the first time the code work arounds were kind of ridiculous because there is no ego type.
I think it's a semantic choice, egos just don't have a type. At this point the protocol API is probably not going to change, but we could convert it when loading into state, or at least bare this pattern in mind when we need to reuse functions.
|
I'm afraid I much prefer the duplication of code caused by ego not having a type, to changing the model to |
Resolves #252. This adds both ego and edge ordinal/categorical variables to the server overview screen, and to the settings screen to toggle which ones display.
It also fixes a bug where the counts were incorrect if you had two variables of the same name within the same entity (e.g. node: person with variable: catVar, and node: venue with variable: catVar).