Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(smart views): smart views can access nested relationship #203

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(smart views): smart views can access nested relationship #203

wants to merge 2 commits into from

Conversation

VincentMolinie
Copy link
Member

@VincentMolinie VincentMolinie commented Oct 11, 2018 •
edited
Loading

Description

The purpose of this PR is to return correctly all relationships from the liana. This will allow to use simple getters on Smart View.

Requirements

https://github.com/ForestAdmin/forestadmin/pull/1022

Pull Request checklist:

  • Write an explicit title for the Pull Request
  • Write changes made in the CHANGELOG.md
  • Test manually the implemented changes
  • Review my own code (indentation, syntax, style, simplicity, readability)
  • Wonder if you can improve the existing code

@VincentMolinie VincentMolinie changed the title Fix/nested relationship smart view [*] Smart Views - Smart views can access nested relationship Oct 11, 2018
@VincentMolinie
Copy link
Member Author

P2 - Plugify can't get his smart view to work properly

@VincentMolinie VincentMolinie requested review from arnaudbesnier and removed request for ArnaudValensi October 16, 2018 12:27
@VincentMolinie
Copy link
Member Author

To add more context about how JSON API works:
First thing we need to know is that if in a relationships, there is a link specified even if the data are retrieved in the included, Ember Data will do a GET request on the url. So link must be here ONLY if the data is not included. Then where there is data in relationships { data: { id: '1', type: 'student'}} and a link the data will always be the first one to be called. If none of them is specified, the relation is considered as null.

VincentMolinie
VincentMolinie commented Oct 16, 2018
View reviewed changes
package.json Outdated
@@ -35,7 +35,7 @@
"express-jwt": "5.3.1",
"inflected": "2.0.4",
"ip-utils": "git+https://github.com/ForestAdmin/ip-utils.git#5f88562ba53fedcdc0374937fca0fdb71fa4923c",
"jsonapi-serializer": "3.4.1",
"jsonapi-serializer": "3.6.1",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix some issue with the serializer and allow to return null from relationshipLinks function and not include the link in this case

src/serializers/resource.js
function getAttributesFor(dest, fields) {
function getRelatedLinkForHasMany(modelName, relationshipName) {
return function (record, current, parent) {
if (current && current[relationshipName]) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not want the link to be set if the relation is already included in the payload

src/serializers/resource.js
dest[fieldName] = {
ref: fieldReference,
nullIfMissing: true,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this or the links will not be included if the relation is not in the records sent to the serializer

src/serializers/resource.js
if (_.isArray(field.type)) {
dest[fieldName].ignoreRelationshipData = true;
if (_.isArray(field.type) || !include) {
dest[fieldName].ignoreRelationshipData = include;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not include from second level (We never include relations of a relation)

src/serializers/resource.js Outdated
@@ -141,7 +158,35 @@ function ResourceSerializer(Implementation, model, records, integrator, opts, me
});
}

var serializationOptions = {
function defineRelationshipId(records, fields, goDeeper = true) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recursive function to initialize belongsTo with an id if not set (based on foreignKey) so the serializer can return the { data: { id: 2 } }

src/serializers/resource.js
const fieldName = field.field;
const [referenceType,referenceKey] = field.reference.split('.');
const referenceSchema = Schemas.schemas[referenceType];
if (goDeeper && record[fieldName]) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only do it on one level. But we could do it recursively like on forest-rails (@arnaudbesnier Your call)

@arnaudbesnier
Copy link
Member

Let's keep this for later. We need stability now and I don't want to introduce unecessary feature. Especially on the liana side.

@arnaudbesnier arnaudbesnier removed their assignment Oct 23, 2018
@VincentMolinie VincentMolinie force-pushed the fix/nested-relationship-smart-view branch from 12de3c3 to 33e1a19 Compare February 28, 2020 16:51
@SteveBunlon SteveBunlon removed their request for review March 2, 2020 08:41
@SteveBunlon SteveBunlon removed their assignment Mar 2, 2020
rap2hpoutre
rap2hpoutre reviewed Mar 26, 2020
View reviewed changes
Copy link
Contributor

@rap2hpoutre rap2hpoutre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this contribution! Since this repository now uses (experimental) semantic release, you might have to:

  • Remove entry to changelog.
  • Change the target to master.
  • Change the title (you could fix(smart views): smart views can access nested relationship).

Let me know if I can help!

@VincentMolinie VincentMolinie changed the base branch from devel to master May 13, 2020 08:47
@VincentMolinie VincentMolinie changed the title [*] Smart Views - Smart views can access nested relationship feat(smart views): smart views can access nested relationship May 13, 2020
@VincentMolinie VincentMolinie force-pushed the fix/nested-relationship-smart-view branch from b4141a8 to 55fd940 Compare April 27, 2022 07:59
@arnaud-moncel arnaud-moncel removed their request for review April 27, 2022 08:01
@arnaud-moncel arnaud-moncel removed their assignment Apr 27, 2022
@codeclimate
Copy link

codeclimate bot commented Apr 29, 2022

Code Climate has analyzed commit 9ece720 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 78.3% (70% is the threshold).

This pull request will bring the total coverage in the repository to 60.6% (0.6% change).

View more on Code Climate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rap2hpoutre rap2hpoutre rap2hpoutre left review comments

@arnaudbesnier arnaudbesnier Awaiting requested review from arnaudbesnier

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@VincentMolinie @arnaudbesnier @rap2hpoutre @ArnaudValensi @SteveBunlon @arnaud-moncel