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

Support for product_type = degree in Django ORM POC for SFMC Catalog #4419

Merged
merged 2 commits into from
Sep 4, 2024
Merged

Support for product_type = degree in Django ORM POC for SFMC Catalog #4419

merged 2 commits into from
Sep 4, 2024

Conversation

hamza-56
Copy link
Contributor

@hamza-56 hamza-56 commented Aug 29, 2024 •
edited
Loading

PROD-4153

This PR enhances the existing management command for populating the Course Catalog by adding support for the Degree Catalog.

Key Updates:

  • Degree Catalog Support: Added logic to populate required fields from the Program Model into a CSV file.
  • Filtering: Introduced filters for product_type and product_source.
  • Google Sheets Update: Integrated with gspread to update an existing Google Sheet.
  • Unit Tests: Added tests to ensure functionality for the Degree Catalog.

@hamza-56 hamza-56 marked this pull request as draft August 29, 2024 10:03
@hamza-56 hamza-56 force-pushed the hamza/PROD-4153 branch 2 times, most recently from 65185b2 to 0c6b5c4 Compare August 29, 2024 21:15
@hamza-56 hamza-56 marked this pull request as ready for review August 30, 2024 09:20
@hamza-56 hamza-56 requested a review from AfaqShuaib09 August 30, 2024 13:08
DawoudSheraz
DawoudSheraz reviewed Sep 2, 2024
View reviewed changes
...se_discovery/apps/course_metadata/management/commands/tests/test_populate_product_catalog.py
Verify get_transformed_data method is working correctly for degree
"""
product = self.degrees[0]
command = Command()
Copy link
Contributor

Choose a reason for hiding this comment

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

why is Command() created here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're using command.get_transformed_data(product, "degree") on line 186

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. How does it evaluate that the command to execute is populate catalog command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from course_discovery.apps.course_metadata.management.commands.populate_product_catalog import Command

DawoudSheraz
DawoudSheraz reviewed Sep 2, 2024
View reviewed changes
Copy link
Contributor

@DawoudSheraz DawoudSheraz left a comment

Choose a reason for hiding this comment

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

Please update PR description to include the relevant context.

@hamza-56 hamza-56 force-pushed the hamza/PROD-4153 branch 2 times, most recently from 11a4ad6 to c9c9de7 Compare September 3, 2024 10:52
@hamza-56 hamza-56 self-assigned this Sep 3, 2024
AfaqShuaib09
AfaqShuaib09 approved these changes Sep 4, 2024
View reviewed changes
Copy link
Contributor

@AfaqShuaib09 AfaqShuaib09 left a comment

Choose a reason for hiding this comment

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

Few nits to address, otherwise LGTM

...se_discovery/apps/course_metadata/management/commands/tests/test_populate_product_catalog.py Outdated Show resolved Hide resolved
...se_discovery/apps/course_metadata/management/commands/tests/test_populate_product_catalog.py Outdated Show resolved Hide resolved
...se_discovery/apps/course_metadata/management/commands/tests/test_populate_product_catalog.py Outdated Show resolved Hide resolved
...se_discovery/apps/course_metadata/management/commands/tests/test_populate_product_catalog.py Outdated Show resolved Hide resolved
...se_discovery/apps/course_metadata/management/commands/tests/test_populate_product_catalog.py Outdated Show resolved Hide resolved
@hamza-56 hamza-56 force-pushed the hamza/PROD-4153 branch 2 times, most recently from 7777ab5 to 01fc7e2 Compare September 4, 2024 11:04
@hamza-56 hamza-56 merged commit ec8fc50 into master Sep 4, 2024
24 of 26 checks passed
@hamza-56 hamza-56 deleted the hamza/PROD-4153 branch September 4, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AfaqShuaib09 AfaqShuaib09 AfaqShuaib09 approved these changes

@DawoudSheraz DawoudSheraz DawoudSheraz approved these changes

@Ali-D-Akbar Ali-D-Akbar Awaiting requested review from Ali-D-Akbar

Assignees

@hamza-56 hamza-56

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hamza-56 @DawoudSheraz @AfaqShuaib09