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

Added Enterprise Data Exports Report #35462

Merged
merged 4 commits into from
Jan 16, 2025
Merged

Added Enterprise Data Exports Report #35462

merged 4 commits into from
Jan 16, 2025

Conversation

mjriley
Copy link
Contributor

@mjriley mjriley commented Dec 2, 2024

Product Description

Enterprise Dashboard:
image

API:
image

Generated Report:
image

Technical Summary

Associated ticket: https://dimagi.atlassian.net/browse/SAAS-16298

Feature Flag

No feature flag, as we're doing iterative releases for this

Safety Assurance

Safety story

Tested locally

Automated test coverage

No automated tests so far. The generation logic should eventually be moved into its own functionality. That is the functionality I'd like to write tests for, but I think that is pending the approval of the iterators PR, which sets the precedent for this.

QA Plan

TBD, but this should go through QA.
QA should verify that the following are listed on the report/API:

  • Form Export
  • Case Export
  • Form Daily Saved Export
  • Case Daily Saved Export
  • Form Excel Integration
  • Case Excel Integration

Nothing else should appear on the report, so its valid to test that something like PowerBI feeds do not appear here.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@mjriley mjriley added the product/admin Change affects admin pages only visible to super users / staff label Dec 2, 2024
@mjriley mjriley force-pushed the mjr/data-exports-report branch from 9fd706e to cc754ce Compare December 2, 2024 20:32
Copy link
Contributor

@jingcheng16 jingcheng16 left a comment

Choose a reason for hiding this comment

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

Left a few comment also left a question in tech spec.

return bundle

def get_primary_keys(self):
return ('domain', 'export_type', 'export_subtype', 'name')
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested on staging, I can have two export in same domain, with same export_type, export_subtype and name. In this case, adding owner won't be helpful. Should we add some other attribute like export_id?

Copy link
Contributor Author

@mjriley mjriley Jan 14, 2025

Choose a reason for hiding this comment

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

That's a good catch. I'm not sure its necessary, however, for us to treat these primary keys as typical database primary keys, where keys must be unique. Given that OData feeds are also data exports, I was using ODataFeedResource as a comparison point, and that has a primary key of just report_name, when we allow multiple odata feeds to share the same name within a domain. The more important point, to me, is that the OData report has not provided any real way to differentiate between duplicate names (unless you count report_rows, but (name, report_rows) isn't guaranteed to be unique either) and, to my knowledge, no one has complained.

Basically -- making a real primary key would come at the cost of making the report more complex (adding an additional ID field). In practice, I don't believe a customer would ever want to duplicate names. If PowerBI handles primary keys not being unique, I think we are probably fine leaving the definition as is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just verified using duplicate report names for the OData Feed, and PowerBI handled it without any complaints. Unless you think we should be including the ID anyway, I think I'd prefer to keep this as is

Copy link
Contributor

Choose a reason for hiding this comment

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

Primary keys are conventionally unique for good reason—they simplify row identification and avoid ambiguities. I just think it is the best practice. But I see your points too... I don't know. @biyeun thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the name 'primary key' is misleading. Given my testing, this primary key not being unique does not cause errors, like it would in a typical database. Our objective is to give the users a feed they can import into powerBI, not conform strictly to the OData spec.

Copy link
Contributor

@jingcheng16 jingcheng16 left a comment

Choose a reason for hiding this comment

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

Approved. It's just primary key being non-unique caught me off. Maybe you're right the name "primary key" is misleading. Anyway, I decide to approve it. If something goes wrong in the future, we can still fix it! And we will learn our lesson too.

@mjriley mjriley merged commit dd99fca into master Jan 16, 2025
13 checks passed
@mjriley mjriley deleted the mjr/data-exports-report branch January 16, 2025 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/admin Change affects admin pages only visible to super users / staff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants