openedx / aspects-dbt

The dbt project for Open edX Aspects!
Apache License 2.0
2 stars 5 forks source link

Adds models for enrollments_by_day #1

Closed SoryRawyer closed 1 year ago

SoryRawyer commented 1 year ago

Also includes:

openedx-webhooks commented 1 year ago

Thanks for the pull request, @SoryRawyer! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

SoryRawyer commented 1 year ago

While I'm looking at the macros, I think it makes more sense to remove the get_event_attributes macro and defer pulling event attributes out of event_str until they're actually needed. It was a helpful exercise while getting started but I don't think it makes sense longer-term.

Ian2012 commented 1 year ago

I know this is still in very early stages and that this will be run probably in background, a cronjob, a init job or in a tutor command, but can you leave testing instructions and a bit of documentation?

SoryRawyer commented 1 year ago

Absolutely! I will check in some additional documentation along with a requirements.txt. I have some thoughts about making setup a little easier but I was thinking of saving those updates for a PR that's more focused on documentation and setup.

Ian2012 commented 1 year ago

@SoryRawyer Is there anything blocking this PR? Can you rename all commits or squash them into one?

SoryRawyer commented 1 year ago

@Ian2012 I don't think there's anything blocking this. I'll go ahead and squash my commits so they follow the conventional commit framework.

openedx-webhooks commented 1 year ago

@SoryRawyer 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.