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.

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

[*] Smart Segments - Prevent includes from queryBuilder to overwrite include from segment scope #179

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

Conversation

louisremi
Copy link

Considering three models City, Region, Country, their associations, and the scope and collection defined as follows:

const City = sequelize.define('City', {});
const Region = sequelize.define('Region', {});
const Country = sequelize.define('Country', {});

City.hasOne(Region);
Region.hasOne(Country);

City.addScope('region+country', {
  include: [{
    model: Region,
    include: [Country]
  }]
});

Liana.collection('City', {
  segments: [{
    name: 'with country',
    scope: 'region+country'
  }]
});

When requesting the segment with country, the current implementation of resources-getter.js would return the city and the associated region, but not the country associated to that region. This is because Region is added automatically to the .include list of the request, and overwrites the Region included in the scope.
This patch prevents includes returned by queryBuilder.getIncludes to overwrite the includes of the scope.

@arnaudbesnier arnaudbesnier changed the title Prevent includes from queryBuilder to overwrite include from segment scope [*] Smart Segments - Prevent includes from queryBuilder to overwrite include from segment scope Jun 18, 2018
model.unscoped().count(countOpts),
model.unscoped().findAll(findAllOpts)
]);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the refactoring ;)
I applied it on the latest code version here: a1cc888

@arnaudbesnier arnaudbesnier self-assigned this Jun 18, 2018
@arnaudbesnier
Copy link
Contributor

Hi @louisremi, the current segment feature has been implemented to filter the records to retrieve and not to customize the aggregation done on the retrieved data.

I am not sure to understand your use case here.
What do you do with the aggregated country sent with the city records?
Do you use this information in a Smart View?
Or is it because you use the Forest generated API for other purposes?

Thanks for your help.

@louisremi
Copy link
Author

@arnaudbesnier We use this patch to display aggregated info from different tables in list views. The only alternative is to use a "View as Model", (e.g. https://github.com/chez-nestor/backoffice/blob/master/src/models/AmountView.js ) but this is often overkill.
I'll recreate this patch on top of master.

@louisremi
Copy link
Author

@arnaudbesnier done! Looking forward to your review, as this is the last patch we have to backport everytime we want to update our Liana :-)

@SteveBunlon SteveBunlon force-pushed the devel branch 2 times, most recently from 660dc77 to 9f305b9 Compare April 17, 2020 09:39
@arnaudbesnier arnaudbesnier changed the base branch from devel to master May 13, 2020 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants