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

Move Backends to this repo #21

Merged
merged 1 commit into from
Mar 3, 2022
Merged

Move Backends to this repo #21

merged 1 commit into from
Mar 3, 2022

Conversation

shadinaif
Copy link
Contributor

@shadinaif shadinaif commented Feb 15, 2022 •
edited
Loading

Change description

Move Backends to this repo

Type of change

  • Bug fix (fixes an issue)
  • New feature (adds functionality)

Related issues

https://appsembler.atlassian.net/browse/RED-2707

Checklists

Development

  • Lint rules pass locally
  • Application changes have been tested thoroughly
  • Automated tests covering modified code pass

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

Code review

  • Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached as necessary
  • "Ready for review" label attached and reviewers assigned
  • Changes have been reviewed by at least one other contributor
  • Pull request linked to task tracker where applicable

@shadinaif
Copy link
Contributor Author

@OmarIthawi please ignore things related to #12. I'll create a new PR for that. Or do you prefer resolving that here?

OmarIthawi
OmarIthawi suggested changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

@shadinaif Thanks for making the migration. Overall this is a good (yeah, ignore the cosmetic comments).

I have few changes on the backend implementation on assuming multi-orgs per user.

I think the changes are minor but important to have.

tahoe_sites/api.py Outdated Show resolved Hide resolved
tahoe_sites/backends.py Outdated Show resolved Hide resolved
tahoe_sites/tests/test_backends.py Show resolved Hide resolved
tahoe_sites/backends.py Outdated Show resolved Hide resolved
tahoe_sites/backends.py Outdated Show resolved Hide resolved
OmarIthawi
OmarIthawi approved these changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @shadinaif!

One thing we may want to change is fail_if_inactive=True.

I hope it doesn't make you unhappy, but we actually never use the is_active and it's always True. But we may need it in the near future.

tahoe_sites/backends.py Outdated
site = get_current_site()
if not is_main_site(site) and user and not user.is_superuser:
try:
user_organizations = get_organization_for_user(user=user, fail_if_inactive=False)
Copy link
Contributor

@OmarIthawi OmarIthawi Feb 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is unrelated to https://github.com/appsembler/edx-organizations/blob/3708d65456e70328383fecae03bb48666f20e405/organizations/backends.py#L44

We have more than one is_active and we probably should remove this field after this feature is in production.

non-required:

Suggested change
user_organizations = get_organization_for_user(user=user, fail_if_inactive=False)
user_organizations = get_organization_for_user(user=user, fail_if_inactive=True)

See:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be fixed later as described above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be kept as it is because the issue is fixed in #31

@OmarIthawi OmarIthawi mentioned this pull request Feb 25, 2022
Open
@shadinaif shadinaif marked this pull request as draft March 3, 2022 11:01
@OmarIthawi
Copy link
Contributor

@shadinaif this pull request seems like it's ready to be merged. Anything blocking it?

@shadinaif shadinaif force-pushed the shadinaif/move-backends branch 2 times, most recently from 33c9fc5 to aeca194 Compare March 3, 2022 12:37
@shadinaif
Copy link
Contributor Author

@OmarIthawi please final look to OrganizationMemberBackend . Now the backend still accepts inactive users, but get_organization_for_user is now using User.is_active to do the check

@shadinaif shadinaif marked this pull request as ready for review March 3, 2022 12:43
OmarIthawi
OmarIthawi approved these changes Mar 3, 2022
View reviewed changes
Copy link
Contributor

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks @shadinaif!

Looking forward to see this backend being used in staging. It's hardcoded in staging, but should be doable to customize:

tahoe_sites/backends.py Outdated Show resolved Hide resolved
@shadinaif shadinaif merged commit a8a1e69 into main Mar 3, 2022
@shadinaif shadinaif deleted the shadinaif/move-backends branch March 3, 2022 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thraxil thraxil thraxil approved these changes

@OmarIthawi OmarIthawi OmarIthawi approved these changes

@estherjsuh estherjsuh Awaiting requested review from estherjsuh

@melvinsoft melvinsoft Awaiting requested review from melvinsoft

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@shadinaif @OmarIthawi @thraxil