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

[ch140093] Fix subscription/sample filter for datasets #16254

Merged

Conversation

Shylpx
Copy link
Collaborator

@Shylpx Shylpx commented Apr 13, 2021

Resources

Context

In the visualizations controller we are paginating the visualizations, and then filtering by subscriptions or samples if we select "Subscriptions" or "Samples" in the dashboard. It means that we will only see the subscribed dataset located in the current page.

The idea would be adding the subscription and sample filters to the rest of ActiveRecord filters. This way we would filter, and then we would paginate the result. The way to know if a visualization is a "subscription" or a "sample" is calling to the method subscription?/sample?. So in this case I'm trying to use those methods to select the visualizations with a sample/subscription, get the IDs, and then add a where filter by ID (to keep using an ActiveRecord::Relation, in order to not paginate it as an array and avoid potential issues).

I understand that it is not the most elegant option and I could use ActiveRecord for samples with something similar to the following (not tested):

query
  .includes(map: { user_table: :data_import, layers: { user_tables: :data_import } })
  .where(layers: { kind: %w{carto torque} })
  .where(
    "data_imports.service_name = :service_name OR " \
    "data_imports_user_tables.service_name = :service_name",
    service_name: 'connector'
  )
  .where(
    "data_imports.service_item_id LIKE :provider OR " \
    "data_imports_user_tables.service_item_id LIKE :provider",
    provider: '%"provider":"do-v2-sample"%'
  )

This wouldn't be enough for subscriptions, because there is an additional check: if the user subscription is active. The list of subscriptions is stored in Redis, and we need to check if the subscription_id is in the Redis' list, like in https://github.com/CartoDB/db-connectors/blob/master/lib/do_sync_service.rb#L189.

But since we are calling anyway the subscription?/sample? method for all visualizations to calculate the DO totals in https://github.com/CartoDB/cartodb/blob/master/app/controllers/carto/api/visualizations_controller.rb#L505-L512, I would say that filtering using select is "safe" (in terms of adding additional load).

Changes

  • Add two new filters (visualization with sample/subscription) to the Carto::VisualizationQueryFilterer.
  • Update the controller properly to use the new filters and calculate the DO totals.

@shortcut-integration
Copy link

@Shylpx Shylpx changed the title [ch140093] Fix the dataset filter by subscription/sample [ch140093] Fix subscription/sample filter for datasets Apr 13, 2021
@Shylpx Shylpx self-assigned this Apr 13, 2021
@Shylpx Shylpx requested a review from amiedes April 13, 2021 17:05
@Shylpx Shylpx marked this pull request as ready for review April 13, 2021 17:06
Copy link
Contributor

@amiedes amiedes left a comment

Choose a reason for hiding this comment

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

Thanks a lot for explaining the original issue and the decisions taken. This greatly simplifies the process of reviewing 🙂

Good work 💪

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