feat: update license assign view to set source for assigned licenses #526
Conversation
muhammad-ammar
force-pushed
the
ammar/update-license-assign-view-to-set-license-source
branch
from
54ee2ac
to
1ac30c0
Compare
| raise serializers.ValidationError('Wrong Salesforce Ids.') | ||
|
|
||
| # if saleforce ids list is present then not every item in the list evaluates to False | ||
| allFalse = not any(user_sfids) |
There was a problem hiding this comment.
nit: snake_case instead of camelCase
| if user_sfids: | ||
| # if saleforce ids list is present then its length must be equal to number of user emails | ||
| if len(user_emails) != len(user_sfids): | ||
| raise serializers.ValidationError('Wrong Salesforce Ids.') |
There was a problem hiding this comment.
nit: maybe "Number of Salesforce IDs did not match number of provided user emails." ?
There was a problem hiding this comment.
nit: ... would including the number of user_emails vs. number of user_sfids be helpful in this message, too?
"Number of Salesforce IDs (X) did not match number of provided user emails (Y)."?
| raise serializers.ValidationError('Wrong Salesforce Ids.') | ||
|
|
||
| # if saleforce ids list is present then not every item in the list evaluates to False | ||
| allFalse = not any(user_sfids) |
There was a problem hiding this comment.
What's the reasoning for this check?
There was a problem hiding this comment.
Also curious about this, as I was a bit confused about the intent for a seemingly similar check in the associated frontend PR: openedx/frontend-app-admin-portal#1034 (comment)
There was a problem hiding this comment.
ah...frontend will never send such a list that will make this condition true. I think this may be overthinking on my side. I will think about this more and see if we can remove this check?
muhammad-ammar
force-pushed
the
ammar/update-license-assign-view-to-set-license-source
branch
3 times, most recently
from
9a94dc0
to
18c36af
Compare
muhammad-ammar
force-pushed
the
ammar/update-license-assign-view-to-set-license-source
branch
from
18c36af
to
c00f383
Compare
license_manager/apps/api/v1/views.py
Outdated
| @@ -769,8 +800,19 @@ def _assign(self, request, subscription_plan): | |||
| # Validate the user_emails and text sent in the data | |||
| self._validate_data(request.data) | |||
|
|
|||
| # Dedupe all lowercase emails before turning back into a list for indexing | |||
| user_emails = list({email.lower() for email in request.data.get('user_emails', [])}) | |||
| emails_and_sfids = None | |||
There was a problem hiding this comment.
nit: perhaps set the initial variable to be an empty list so the data type is consistent throughout? The if emails_and_sfids conditional below would still work with [].
| ), | ||
| allow_empty=False, | ||
| required=False, | ||
| error_messages={"empty": "Wrong Salesforce Ids."} |
There was a problem hiding this comment.
nit/curious: will this error message only get returned when an empty list is provided? If so, I wonder if this error message should say something to the effect of "No Salesforce Ids provided."? Or, is this error message what appears if a given Salesforce Id doesn't pass the validators above (in which case, would a message like "Incorrect format of Salesforce Ids" make sense?).
There was a problem hiding this comment.
The above message will come in action when we pass an empty list to api endpoint.
muhammad-ammar
force-pushed
the
ammar/update-license-assign-view-to-set-license-source
branch
from
c00f383
to
ab8da8d
Compare
muhammad-ammar
deleted the
ammar/update-license-assign-view-to-set-license-source
branch
Description
Add support into license assign endpoint to set source for assigned licenses if salesforce ids are coming in the request.
Link to the associated ticket: https://2u-internal.atlassian.net/browse/ENT-7642
Testing considerations
already been performed.
Post-review
Squash commits into discrete sets of changes