Skip to content

Conversation

@Ian2012
Copy link
Contributor

@Ian2012 Ian2012 commented Sep 12, 2022

Description

This PR adds a dropdown to select the organization. The dropdown will only be activated for users with CourseCreator permission to specific organizations in Studio.

Use cases:

When FEATURES['ENABLE_CREATOR_GROUP'] = True and the user has CourseCreator permission granted, a dropdown will appear with all specific organizations allowed. In case of all_organizations setting is enabled, all organizations will appear in the dropdown.

In case the user is staff, he can create organizations it will work as before:

image

Testing instructions

  1. Enable course creator group in studio.
  2. Create a student account and give it CourseCreator permission only for some organizations.
    image
  3. Login as this user and try to create a new course.

Result:
image

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 12, 2022
@openedx-webhooks
Copy link

Thanks for the pull request, @Ian2012! 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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@natabene
Copy link
Contributor

@Ian2012 Thank you for the contribution. Is this ready for our review?

@Ian2012
Copy link
Contributor Author

Ian2012 commented Sep 19, 2022

@natabene I was waiting for feedback from the community. Do you know someone that maybe interested or has knowledge of the CourseCreator permission?

cc @MaferMazu @mariajgrimaldi

@natabene
Copy link
Contributor

@Ian2012 I am not aware.

@mariajgrimaldi
Copy link
Member

Hello @natabene!
I can review this one. But I have a question: since it changes in some way the user experience, should we include a product review? Or this minimal change doesn't need it? It's unclear to me how this is handled.

@jfay1
Copy link

jfay1 commented Oct 5, 2022

Hi, Jon here from the UX team. I'm unable to test, but based on the screenshot this looks like a nice improvement.

@Ian2012
Copy link
Contributor Author

Ian2012 commented Oct 5, 2022

Hi @jfay1, thanks for the reply

Would you mind explaining why you were unable to test it?

Are you facing some error?

@jfay1
Copy link

jfay1 commented Oct 5, 2022

Hi @Ian2012, no errors but I'm wondering if I need to test on a certain site? Sometimes I'm sent a test link on stage etc.

I'm not sure how to get to the screen in step 2 but I do have permission to create courses in studio. When I create a new course I see the old field, but that's what I would expect.

@santiagosuarezedunext
Copy link

Hey @jmakowski1123 what do you think of this feature.
I think that currently it is very annoying to have to know the name of the organization, there is a lot of room for error and this could be solved
Do you think anything could be improved?

@MaferMazu
Copy link
Contributor

@Ian2012 I think this is a good improvement; you should try to fix the test to have a proper review.

@Ian2012 Ian2012 force-pushed the cag/organization-dropdown branch from 5bd8267 to 5fe5ab7 Compare October 10, 2022 18:01
@mariajgrimaldi
Copy link
Member

Hello. I can review this one if we're clear from the product side.

@Ian2012 Ian2012 force-pushed the cag/organization-dropdown branch 2 times, most recently from 578bd57 to cabea10 Compare October 10, 2022 20:40
@jmakowski1123
Copy link

Hey @jmakowski1123 what do you think of this feature. I think that currently it is very annoying to have to know the name of the organization, there is a lot of room for error and this could be solved Do you think anything could be improved?

@santiagosuarezedunext It seems to be a useful feature in terms of fixing a known pain point. What does the behavior look like if there are multiple organizations listed in the dropdown? Do they display alphabetically? Is a user able to scroll through the full list of enabled organizations, as well as start typing an organization and be offered a dropdown of selections based on what he/she typed?

@Ian2012
Copy link
Contributor Author

Ian2012 commented Oct 10, 2022

Hi @jmakowski1123,

What does the behavior look like if there are multiple organizations listed in the dropdown?

The organizations will be sorted by id (creation date).

Is a user able to scroll through the full list of enabled organizations, as well as start typing an organization and be offered a dropdown of selections based on what he/she typed?

No, the dropdown was added because it worked like that before (with the option to type in) but users don't really know which organizations they have access.

Staff users and users with permission to create organizations still have the old input (type and get suggestions)

@mariajgrimaldi mariajgrimaldi self-requested a review October 20, 2022 20:46
Comment on lines 1938 to 1944
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if user.count() == 0:
return []
elif user[0].all_organizations:
# User is defined to be unique, can assume a single entry.
return Organization.objects.all().values_list('short_name', flat=True)
else:
return user[0].organizations.all().values_list('short_name', flat=True)
if not user:
organizations = []
elif user[0].all_organizations:
organizations = Organization.objects.all().values_list('short_name', flat=True)
else:
organizations = user[0].organizations.all().values_list('short_name', flat=True)
return organizations

Copy link
Member

Choose a reason for hiding this comment

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

Also, I believe some testing would be beneficial. You could add tests to test_organizations, extend the existing ones or create a new test suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test related to CourseCreators have been modified because this is the main functionality that is affected by those changes

Copy link
Member

Choose a reason for hiding this comment

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

what I see is that you're only testing get_allowed_organizations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More test have been added to can_create_courses. This permission is only validation on the frontend, because this permission is checked once a request is made to create a course or library in the backend

Copy link
Member

Choose a reason for hiding this comment

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

is new-library-org missing 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.

In fact, I need to verify if this change is applied correctly but I don't know how to compile those SCSS files. Do you know who can give me a hand with the styles?

Copy link
Member

Choose a reason for hiding this comment

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

If you're using tutor you could try this from the docs and then inspect the browser. You could do a before and after and post it under this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, you were right:
image

image

@mariajgrimaldi mariajgrimaldi self-assigned this Oct 20, 2022
@Ian2012 Ian2012 force-pushed the cag/organization-dropdown branch from 837feff to 4ce0fab Compare October 27, 2022 18:59
@jmakowski1123 jmakowski1123 added the product review PR requires product review before merging label Oct 27, 2022
@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Oct 31, 2022

I've tested the following in my local environment:

  • ENABLE_CREATOR_GROUP disabled
  • Instructor with permission to all organizations
  • Instructor with permission to just one organization

Technically, this is working like a charm—also, thanks @Ian2012, for considering my suggestions. Given this is under product review, I won't leave my approval yet.

@Ian2012
Copy link
Contributor Author

Ian2012 commented Nov 16, 2022

@mariajgrimaldi Is there anything that we can do to push this forward?

@mariajgrimaldi
Copy link
Member

@Ian2012: we need to wait for the product review.

Is there anything we could do in the meantime? @jmakowski1123

@Ian2012 Ian2012 force-pushed the cag/organization-dropdown branch from 4ce0fab to 7c32305 Compare November 17, 2022 21:29
@openedx-webhooks openedx-webhooks removed the product review PR requires product review before merging label Nov 17, 2022
@mphilbrick211
Copy link

@Ian2012 can you rebase with the master branch? so we can merge. Also @mphilbrick211, thanks for the ping! This PR still has the product review label, should we remove it?

@mariajgrimaldi - we're planning to leave those labels on just in case for some reason we need to go back to this PR (or any PR that was marked for Product review. Thank you for checking!

@mariajgrimaldi
Copy link
Member

@mphilbrick211 good! -just checking- this can be merged, right -from the product view-?

@mphilbrick211
Copy link

mphilbrick211 commented Feb 1, 2023

@mphilbrick211 good! -just checking- this can be merged, right -from the product view-?

@mariajgrimaldi Looks like it per @cablaa77's comment above!

@mphilbrick211
Copy link

@mariajgrimaldi just checking in to see if this can be merged?

@mariajgrimaldi
Copy link
Member

@mphilbrick211: the branch is still out-of-date, once @Ian2012 updates it, I'll merge this :)

Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

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

I'll be merging this tomorrow :)

@Ian2012 Ian2012 force-pushed the cag/organization-dropdown branch from 8c0253b to c8efb16 Compare February 7, 2023 21:20
@mariajgrimaldi mariajgrimaldi merged commit 3427a9a into openedx:master Feb 8, 2023
@openedx-webhooks
Copy link

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

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@connorhaugh
Copy link
Contributor

connorhaugh commented Feb 13, 2023

Hi @Ian2012 @mariajgrimaldi our authors on edx.org are running into a 500 error attempting to load the v1 library page as:

File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/views/course.py", line 622, in library_listing return render_to_response('index.html', data)

File "/tmp/mako_cms/17d4a5cdf1015f510e44357b40e03bbb/index.html.py", line 242, in render_content
for org in allowed_organizations:
TypeError: 'Undefined' object is not iterable

The course home page is loading fine, so I presume this has to do with the fact that library_listing_return does not specify an allowed_organizations value?

Can you confirm this behavior with V1 libraries is working as expected, and is a peculiarity of our deployment? It might be, as
/home/#libraries-tab works for me in our staging environment, but not on production.

@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Feb 13, 2023

@connorhaugh I'll check this right away.

BTW, you tagged the wrong Maria 😅

@mariajgrimaldi
Copy link
Member

@Ian2012 can you give us a hand, please? thanks

@Ian2012
Copy link
Contributor Author

Ian2012 commented Feb 13, 2023

Hi @connorhaugh,

The issue is caused because in the context of the library_listing view the new variables are not added to the context and use the same template:

  'allowed_organizations': get_allowed_organizations(request.user),
  'can_create_organizations': user_can_create_organizations(request.user),

@connorhaugh can you revert the commit or should I open a new PR with the fix?

Also, this issue is triggered only when this waffle flag is enabled:

### contentstore.split_library_on_studio_dashboard
SPLIT_LIBRARY_ON_DASHBOARD = WaffleFlag(
    f'{CONTENTSTORE_NAMESPACE}.split_library_on_studio_dashboard',
    __name__,
    CONTENTSTORE_LOG_PREFIX,
)

@Ian2012
Copy link
Contributor Author

Ian2012 commented Feb 13, 2023

@connorhaugh Temporarily, you can disable this waffle flag: contentstore.split_library_on_studio_dashboard

@mariajgrimaldi
Copy link
Member

Thank you, Cristhian.

@connorhaugh: that flag is turned off by default, so it wasn't included as one of our test cases. Please, let us know how to proceed.

@connorhaugh
Copy link
Contributor

Based on what I have heard from our support team, a fix forward is acceptable. Thanks so much your prompt response @Ian2012 @mariajgrimaldi, and all of your help. Let me know if there is anything I can do to help! Seems like the fix is rather straightforward.

We have that waffle flag (split_library_on_studio_dashboard) on on our production env, but not our staging env.

@connorhaugh
Copy link
Contributor

I will review #31752 promptly.

@mariajgrimaldi
Copy link
Member

thank you! @connorhaugh @Ian2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U product review PR requires product review before merging

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.