ENT-201 use catalog contains endpoint for varifying the course seats against …

[zubair-arbi]

…a course catalog range
@saleem-latif @asadiqbal08 @mattdrayer

Previously we were using two API calls for checking if a course run exists for a catalog by first retrieving the catalog and then using the course_runs contains endpoint for the provided course run, from discovery service.

Now we are using the updated catalog contains endpoint which tells us if a course run is available in its query.

Depends on: add support for course run key in catalog contains endpoint

[saleem-latif]

Overall looks good, however, I have added a couple of comments.

[saleem-latif]

I think it would be best to define a helper method in ecommerce/coupons/utils.py that makes network calls to discovery API, and use that method here. This way will have all procedures that perform API calls in a single place.
Name of the utility function can be something like catalog_contains_course(self, course) or something better.

[saleem-latif]

Also, in my opinion I think it would be more readable if catalog_contains_product here returns a boolean if it is possible. and we changes function signature accordingly. thoughts ?

[zubair-arbi]

@saleem-latif @mattdrayer I would recommend a separate story for refactoring/moving the range model API calls to the ecommerce/coupons/utils.py file as we have to update both this method and run_catalog_query method. Also we have to add separate tests for the utils too. The scope of current story already seems a bit extended and its involves working with 3 repos.

[mattdrayer]

I'm okay with logging (another) follow-up story -- we've put in a fair amount of effort on this job and we have some higher priorities that we'll need to address next.

[zubair-arbi]

I have logged a ticket ENT-257 to address this feedback.

[saleem-latif]

It would be good to also add resource key-value pair in cache key to make sure cache key does not collide with any other.

[zubair-arbi]

added

[saleem-latif]

nit: please add a comment here explaining what would be the final URL for the API call, may be something like

# GET: /api/v1/catalogs/{catalog_id}/contains?course_run_ids={course_run_id}

[zubair-arbi]

@mattdrayer @mjfrey please review.

[mattdrayer]

Looks like this can be moved to the try block:

try: 
    return response['courses'][course_id]
except KeyError:
    return False

[mattdrayer]

One change, otherwise 👍

[saleem-latif]

@zubair-arbi commits need to be squashed otherwise LGTM 👍

[mattdrayer]

@vkaracic and/or @marjev may want to take a quick look at this one.

[marjev]

Can you set [] as the default value in line 42? That way you won't need this line.

[zubair-arbi]

@marjev This would result in pylint error "dangerous-default-value":

pep8 --config=.pep8 ecommerce e2e
pylint --rcfile=pylintrc ecommerce e2e
************* Module ecommerce.courses.tests.mixins
W: 42, 4: Dangerous default value [] as argument (dangerous-default-value)

[marjev]

You can do this during the initialization like this:
courses = {course_run_id: True for course_run_id in course_run_ids}

[marjev]

You can import unicode_literals like this: https://github.com/edx/ecommerce/blob/master/ecommerce/coupons/views.py#L1

Then, you won't need to mark the string as Unicode.

[zubair-arbi]

@marjev I have addressed your feedback, please have another look.

[marjev]

LGTM

[clintonb]

If this feature relies on the Catalog Service, please add a feature flag to disable it until the Catalog Service is released.

[zubair-arbi]

@clintonb we already have a waffle switch enable_enterprise_on_runtime which enables enterprise functionality in Ecom and we can use this feature flag instead of adding a new one.
@mattdrayer what is your suggestion?

[mattdrayer]

We're sort of in moot-point territory now because the updated Catalog service has already deployed. However, for this particular PR, I think I would have recommended that we avoid binding the catalog query logic to the enterprise feature flag. That flag is for enterprise features, and while they might use the updated query logic, the actual code was more of a core ecommerce change. So a new feature flag probably should have been introduced if we were expecting to merge both PRs to their respective masters and subsequently manage the releases.

TBH I much prefer simply merging+deploying the updated Catalog service, ensuring no rollback was required, and then merging+deploying the updated Ecommerce service at a later point in time (which is what we're doing now). It's easier to manage and we don't have to worry about juggling feature flags.

Feature flag no longer matters.