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

["FEAT"] Organizations are not created on course_org_filter modification #165

Open
Ian2012 opened this issue Jan 20, 2023 · 10 comments
Open
Assignees
Labels
enhancement New feature or request nice to have

Comments

@Ian2012
Copy link
Member

Ian2012 commented Jan 20, 2023

Describe the bug
When you add an organization to course_org_filter in the tenant the Organization is not created.

To Reproduce
Steps to reproduce the behavior:

  1. Go to 'Tenant config admin table'
  2. Edit the field 'course_org_filter'
  3. Save the model
  4. Verify organization_organization is not created.

Expected behavior
An organization is created with its tenant organization model.

@Ian2012 Ian2012 added bug Something isn't working help wanted Extra attention is needed labels Jan 20, 2023
@Ian2012
Copy link
Member Author

Ian2012 commented Jan 26, 2023

Just to clarify, this is not a bug but a feature request. Before the migration, it was not necessary to create the Organization, but now it is.

cc @DonatoBD @Alec4r

@Alec4r Alec4r changed the title ["BUG"] Organizations are not created on course_org_filter modification ["FEAT"] Organizations are not created on course_org_filter modification Jan 30, 2023
@Alec4r Alec4r added enhancement New feature or request and removed bug Something isn't working labels Jan 30, 2023
@Alec4r
Copy link
Member

Alec4r commented Jan 30, 2023

Just to clarify, this is not a bug but a feature request. Before the migration, it was not necessary to create the Organization, but now it is.

cc @DonatoBD @Alec4r

@Ian2012 but is necessary for what?, what is not working? or for what we need it.

@Ian2012
Copy link
Member Author

Ian2012 commented Jan 30, 2023

@Alec4r On Lilac, the permission to create courses was based on custom permission that can store any value in the organization. When users create a new course with that organization, the organization is created automatically. On Nuez, this permission changed for a relationship to the Organization model which requires the organization to exists before granting course creator permission

@felipemontoya
Copy link
Member

felipemontoya commented Feb 2, 2023

I can see two general alternatives roads here:

  1. we make a sync tasks from the tenantorg model to the organizations model.
  2. we use a pre_save signal to create the organizations model before the permission needs to be created.

Do you guys see something more?

@Alec4r Alec4r added nice to have and removed help wanted Extra attention is needed labels Feb 2, 2023
@andrey-canon
Copy link
Contributor

I can see two general alternatives roads here:

  1. we make a sync tasks from the tenantorg model to the organizations model.
  2. we use a pre_save signal to create the organizations model before the permission needs to be created.

Do you guys see something more?

Please stop using signals, that inhibits readability and the result can be achieved by modifying the save method, however your solution could require that but the comment is not specific about that.

@santiagosuarezedunext
Copy link

santiagosuarezedunext commented Feb 22, 2023

Esto se hará sobre el LMS admin, no sobre el Studio admin verdad?

@felipemontoya
Copy link
Member

After discussing this with @Ian2012 we noticed that this case only happens very rarely when Orgs are created manually by the support team. We have left this with very low priority for now.

About the usage of signals, it is a well established pattern that both we and the platform use. We are the authors and maintainers of openedx-events which internally uses signals. I don't think it is reasonable to ask that we stop using them. Also I don't think this inhibits readability. I think you mean to say that it makes it more difficult to trace the consequences of an action, but that can be mitigated in many other ways.

@andrey-canon
Copy link
Contributor

andrey-canon commented Feb 22, 2023

openedx-events is a specific plugin which adds extension points, if I'm not wrong, so another plugins can take advantage of those signals and inserts a custom behavior or actions since the platform code is not accessible, here we don't have and, I think, we don't want that behavior therefore we cannot compare them, and again your comment was not clear enough about the implementation, so my comment is based on the current implementation of tenant organizations, that I made. In this case we can control and edit the plugin code so a signal is not necessary.

I think that is the most reasonable thing that I have asked, obviously you don't agree , so I here are some sites that explains better this idea

https://stackoverflow.com/a/35950538
https://seddonym.me/2018/05/04/django-signals/
https://lincolnloop.com/insights/django-anti-patterns-signals/
https://www.quora.com/Are-Django-signals-an-anti-pattern

btw: after debugging 3 days an issue, IMO the well established pattern the platform use is the worst thing that they have implemented

@santiagosuarezedunext
Copy link

This is not a priority to move it to in progress, but we have to fix it. I am going to move it to the backlog in the first position for when we have time

@Ian2012
Copy link
Member Author

Ian2012 commented Sep 21, 2023

Not creating the organization also breaks the permissions on Olive version and can cause some weird issues on Aspects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request nice to have
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

6 participants