fix: improve collections models and api [FC-0059] #208

rpenido · 2024-08-09T16:08:15Z

Description

This PR fixes some issues in the Collections model from #206 and adds some improvements to the implementation.

Testing Instructions

Please ensure that the tests cover the expected behavior

Private ref: FAL-3782

openedx-webhooks · 2024-08-09T16:08:19Z

Thanks for the pull request, @rpenido!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @axim-engineering. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

yusuf-musleh

👍 @rpenido Looks good to me, great work! Just had a question about the index, if you can kindly address it.

I tested this: I read through the code and confirmed tests are passing
I read through the code
~~I checked for accessibility issues~~
Includes documentation

yusuf-musleh · 2024-08-13T04:30:00Z

openedx_learning/apps/authoring/collections/models.py

+        indexes = [
+            models.Index(fields=["learning_package_id", "title"]),
+        ]


I just wanted to confirm, since the ForeignKey field defined is learning_package, should this index be learning_package instead of learning_package_id? I was looking at other places in the code where indexes are defined, and the ForeignKey fields are defined without the _id, so I just wanted to make sure if I am missing something.

I think you are right @yusuf-musleh !
Fixed here: 9417609

rpenido · 2024-08-14T14:24:18Z

Hi @pomegranited!

Could you do a last review, merge and tag a new version if everything is good?

yusuf-musleh

@rpenido I noticed a field was missing from the create_collection api as I was working on the REST endpoints, if you could take a look at it below.

yusuf-musleh · 2024-08-19T04:22:06Z

openedx_learning/apps/authoring/collections/api.py

 def create_collection(
    learning_package_id: int,


Suggested change

def create_collection(

learning_package_id: int,

def create_collection(

learning_package_id: int,

created_by: int | None,

I realized the created_by value was missing from the params and in the .create below, would you mind adding them in this PR since I needed it when I was working on the REST endpoints.

Good catch @yusuf-musleh!
Fixed here: a30e04a

yusuf-musleh · 2024-08-19T04:22:58Z

openedx_learning/apps/authoring/collections/api.py

        description=description,
    )


Suggested change

description=description,

)

description=description,

created_by=created_by,

)

Likewise we need to add the created_by here

because we're passing an int ID. Also adds a test.

pomegranited

👍 Looks great @rpenido ! I had a couple of nits, so just pushed them in to move this along. Will get this merged ASAP.

I tested this by running the tests.
I read through the code and ensured tests cover all of the Collections code.
~~I checked for accessibility issues~~ N/A
Includes documentation
User-facing strings are extracted for translation

rpenido · 2024-08-20T13:40:12Z

Thank you @pomegranited!

openedx-webhooks · 2024-08-21T01:27:39Z

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

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 9, 2024

rpenido force-pushed the rpenido/fal-3782-fix-collection-model-issues branch 2 times, most recently from db3b742 to daecf9f Compare August 9, 2024 16:15

rpenido marked this pull request as ready for review August 9, 2024 16:36

rpenido mentioned this pull request Aug 9, 2024

feat: add Collection model and api #206

Merged

rpenido requested a review from ormsbee August 9, 2024 16:39

fix: improve collections models and api

ef09229

rpenido force-pushed the rpenido/fal-3782-fix-collection-model-issues branch from daecf9f to ef09229 Compare August 9, 2024 16:40

rpenido changed the title ~~fix: improve collections models and api~~ fix: improve collections models and api [FC-0059] Aug 9, 2024

pomegranited requested a review from yusuf-musleh August 12, 2024 17:40

ormsbee approved these changes Aug 12, 2024

View reviewed changes

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Aug 12, 2024

yusuf-musleh approved these changes Aug 13, 2024

View reviewed changes

rpenido added 2 commits August 14, 2024 11:00

feat: export models

fa03042

fix: index field

9417609

rpenido requested a review from pomegranited August 15, 2024 13:28

yusuf-musleh reviewed Aug 19, 2024

View reviewed changes

rpenido force-pushed the rpenido/fal-3782-fix-collection-model-issues branch from 9cdd1e7 to 89f4cea Compare August 19, 2024 08:55

fix: add created_by parameter

a30e04a

rpenido force-pushed the rpenido/fal-3782-fix-collection-model-issues branch from 89f4cea to a30e04a Compare August 19, 2024 21:05

pomegranited added 3 commits August 20, 2024 14:19

fix: assign to created_by_id instead of created_by

87defd8

because we're passing an int ID. Also adds a test.

test: small changes to improve test coverage for collections

a4540ec

chore: bumps version to 0.11.1

1c3fdec

pomegranited approved these changes Aug 20, 2024

View reviewed changes

pomegranited merged commit b8610ff into openedx:main Aug 21, 2024

rpenido deleted the rpenido/fal-3782-fix-collection-model-issues branch August 22, 2024 14:13

ormsbee mentioned this pull request Aug 22, 2024

Basic asset response support #217

Merged

ormsbee mentioned this pull request Mar 24, 2025

add vscode pytest settings open-craft/openedx-learning#19

Closed

ormsbee mentioned this pull request Sep 4, 2025

version bump 0.28.0 open-craft/openedx-learning#20

Closed

fix: improve collections models and api [FC-0059] #208

fix: improve collections models and api [FC-0059] #208

Uh oh!

Conversation

rpenido commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing Instructions

Uh oh!

openedx-webhooks commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

Uh oh!

yusuf-musleh left a comment

Choose a reason for hiding this comment

Uh oh!

yusuf-musleh Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

rpenido Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

rpenido commented Aug 14, 2024

Uh oh!

yusuf-musleh left a comment

Choose a reason for hiding this comment

Uh oh!

yusuf-musleh Aug 19, 2024

Choose a reason for hiding this comment

Uh oh!

rpenido Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yusuf-musleh Aug 19, 2024

Choose a reason for hiding this comment

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

rpenido commented Aug 20, 2024

Uh oh!

openedx-webhooks commented Aug 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rpenido commented Aug 9, 2024 •

edited

Loading

openedx-webhooks commented Aug 9, 2024 •

edited

Loading

rpenido Aug 19, 2024 •

edited

Loading