-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check field associated entity and add missing fields #58
base: master
Are you sure you want to change the base?
Conversation
The goal was at one point to sync, the associated entity's meta with the actual entity's meta. So if we had already cached JobOrder, and we tried to get |
aa53124
to
7dba111
Compare
expect(res[2]).toBe('jobOrders(id,title)'); | ||
expect(res[3]).toBe('businessSectors(name,id)'); | ||
expect(res[4]).toBe('category'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you are missing a test that get the fields inside a nested field
ie. meta.getSubFields('businessSectors[3](name,id){name=\'Insurance\'}')
and meta.getSubFields('placement(candidate(id,name))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We made a change to this, to only target 'INLINE_EMBEDDED' entities. The only entity that is configured that way it defaultEanCode, and the other earn code entities. The fields for these, do not ever have double nested fields, so the current logic should be able to handle that.
Confused why we would pull back scope since we thought this would solve/reduce additional requests |
@bvkimball We found that these changes were causing issues with embedded custom objects. In an effort to still get this fix in, we reduced the scope to only fix the target issue. |
&& meta.associatedEntity | ||
&& meta.associatedEntity.fields | ||
&& this.getSubFields(field).some(subField => !meta.associatedEntity.fields.find(aef => aef.name === subField))) { | ||
result.push(cleaned); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something i might have missed earlier is that we are pushing cleaned
that seems wrong because all the default fields would have to be included, Maybe that is true for INLINE_EMBEDDED.
was think something more like
private missing(fields): string[] {
if (!this.memory) {
return fields;
}
if (fields && fields[0] === '*' && this.allFieldsLoaded) {
return [];
}
const result: string[] = [];
for (const field of fields) {
const cleaned: string = this._clean(field);
const meta: any = this.memory[cleaned];
if (!meta) {
// We can't push `cleaned` field because that wouldn't request subField
result.push(field);
} else if (meta.associatedEntity) {
const aefs = meta.associatedEntity.fields.map(aef => aef.name);
// filter out any existing associatedEntity fields
// but this probably needs to be recursive or the subfields need to be cleaned
const missingSubFields = this.getSubFields(field).filter(sub => !aefs.includes(sub));
// construct field with missing sub fields
result.push(`${cleaned}(${missingSubFields.join()})`);
}
}
return result;
}
This fixes an issue where the meta service does not update associated entity fields.
If an entity with an embedded entity field is retrieved and cached, and later retrieved by a view that requires more fields from that embedded entity, the fields do not update.