Feat : update org members API learners ENT-11260 #2503
Conversation
sjasti-sonata-svg
force-pushed
the
feat/update-org-members-API-learners-ENT-11260
branch
from
3726ebe to
5a73629
Compare
| WHERE | ||
| ecu.enterprise_customer_id = %s | ||
| AND | ||
| AND |
sjasti-sonata-svg
force-pushed
the
feat/update-org-members-API-learners-ENT-11260
branch
from
5fb55a8 to
5e12dea
Compare
|
Hi @kiram15 , |
| return response.Response( | ||
| {"detail": "Could not find enterprise uuid {}".format(enterprise_uuid)}, | ||
| status=status.HTTP_404_NOT_FOUND, | ||
| except OperationalError as exc: |
sjasti-sonata-svg
force-pushed
the
feat/update-org-members-API-learners-ENT-11260
branch
4 times, most recently
from
e94d224 to
da135d5
Compare
kiram15
left a comment
kiram15
left a comment
There was a problem hiding this comment.
Looks good! But I'm confused why you have a similar one open without the tests? I think this should be the only one that is merged in.
pwnage101
left a comment
pwnage101
left a comment
There was a problem hiding this comment.
The query SQL change looks good, but I have a concern about the exception handler fallback logic.
| # Fallback query without auth_userprofile | ||
| fallback_query = query.replace( | ||
| "LEFT JOIN auth_userprofile as aup on au.id = aup.user_id", | ||
| "" | ||
| ).replace( | ||
| "coalesce(NULLIF(aup.name, ''), au.username)", | ||
| "au.username" |
There was a problem hiding this comment.
In my opinion, this kind of SQL manipulation is extremely risky. Are you sure auth_userprofile is sometimes missing in test/minimal environments? The UserProfile model does not seem optional.
There was a problem hiding this comment.
Hi @pwnage101
I worked on your comments. could you please check once let me the same.
…ializers, and tests - Added comments to EnterpriseCustomerAdmin and EnterpriseCustomerMembers - Updated views and serializer tests for learners vs admins - Verified invited_date and joined_date handling in tests
sjasti-sonata-svg
force-pushed
the
feat/update-org-members-API-learners-ENT-11260
branch
from
ac2c05a to
9e107ea
Compare
| @staticmethod | ||
| def has_auth_userprofile_table(): | ||
| """ | ||
| Some minimal / test Open edX environments do not include auth_userprofile. |
There was a problem hiding this comment.
Where did you learn this?
There was a problem hiding this comment.
Hi @pwnage101
I actually I didn't understand your question.
can you please elaborate.
There was a problem hiding this comment.
Your code states "Some minimal / test Open edX environments do not include auth_userprofile."
I don't think this is true, so I am curious where you learned this.
4 participants
feat : [BE] Update Organizations Members API.
Update “Organizations Members” API to only fetch learner data.
This PR updates the existing Organization Members API to strictly return learner-type records for an enterprise customer. Previously, the API could return a mix of users with different roles
ENT-11260