Organization specific transcript credentials state #99

muhammad-ammar · 2017-09-20T07:29:00Z

mushtaqak

@muhammad-ammar This is looking good, a few miner changes.

The OrganizationTranscriptCredentialsState model needs to available in Admin.

We also need to add video_source_language in TranscriptPreference model, FYI @muzaffaryousaf

mushtaqak · 2017-09-20T11:59:43Z

edxval/models.py

@@ -605,6 +605,48 @@ def __unicode__(self):
        return u'{course_id} - {provider}'.format(course_id=self.course_id, provider=self.provider)


+class OrganizationTranscriptCredentialsState(models.Model):


This should be included in django admin panel

I would rename it to TranscriptOrganizationCredentialsState

These are transcription credentials for an organization, So that's why I used the name OrganizationTranscriptCredentialsState

mushtaqak · 2017-09-20T12:00:37Z

edxval/models.py

+        return instance, created
+
+    def __unicode__(self):
+        return u'OrganizationTranscriptCredentialsState(org={}, provider={}, exists={})'.format(


I don't think we need to show exists here

mushtaqak · 2017-09-20T12:02:27Z

edxval/models.py

+@receiver(models.signals.pre_save, sender=OrganizationTranscriptCredentialsState)
+def validate_provider(sender, instance, **kwargs):  # pylint: disable=unused-argument
+    """
+    Validate that input provider value mus be one of TranscriptProviderType.CHOICES.


typo: value must be one

mushtaqak · 2017-09-20T12:13:25Z

edxval/models.py

+
+
+@receiver(models.signals.pre_save, sender=OrganizationTranscriptCredentialsState)
+def validate_provider(sender, instance, **kwargs):  # pylint: disable=unused-argument


We are not validating providers anywhere else. Is this necessary here? Should we not add in other places too ?

This is the cleanest way I found to validate provider value before saving.

mushtaqak · 2017-09-21T10:58:47Z

edxval/api.py

@@ -143,6 +143,55 @@ def update_video_status(edx_video_id, status):
    video.save()


+def get_organization_transcript_credentials_state(org, provider=None):


I would rename it to get_transcript_organization_credentials_state

get_3rd_party_transcript_credentials_state_for_org would be good.

mushtaqak · 2017-09-21T11:15:42Z

edxval/tests/test_api.py

+        Verify that `get_organization_transcript_credentials_state` method works as expected
+        """
+        state_info = api.get_organization_transcript_credentials_state(org=org, provider=provider)
+        credentials_state = OrganizationTranscriptCredentialsState.objects.get(org=org, provider=provider)


We may not need to query db, if we add a exists parameter in ddt. Something like :

@data( {'org': 'edX', 'provider': 'Cielo24', 'exists': False}, {'org': 'edX', 'provider': '3PlayMedia', 'exists': False}, ) @unpack def test_get_credentials_state(self, org, provider, exists):

We need reading from db to verify that data received from api method is same as stored in db.

mushtaqak · 2017-09-21T11:16:39Z

edxval/tests/test_api.py

+        self.assertEqual(state_info['org'], credentials_state.org)
+        self.assertEqual(state_info[provider], credentials_state.exists)
+
+    def test_get_credentials_state_with_org_only(self):


This test can be combined in above test with proper ddt

mushtaqak · 2017-09-21T11:17:21Z

edxval/tests/test_api.py

+        {'org': 'edX', 'provider': '3PlayMedia'},
+    )
+    @unpack
+    def test_get_credentials_state(self, org, provider):


We should also add a test if org does not match, what is expected result we should expect

mushtaqak · 2017-09-21T11:18:32Z

edxval/tests/test_api.py

+            org='edX', provider='3PlayMedia', exists=False
+        )
+
+    def assert_credential_state():


assert_credential_state is not used anywhere

mushtaqak · 2017-09-21T11:19:24Z

edxval/tests/test_api.py

+        {'org': 'MAX', 'provider': '3PlayMedia', 'exists': True},
+    )
+    @unpack
+    def test_credentials_state_update(self, **kwargs):


We should also add a test the expected behaviour when org does not match

In case of update/create, new credentials state record will be created if org does not match.

I think we missed this test for No org found case?

muhammad-ammar · 2017-09-22T06:50:44Z

@mushtaqak Feedback Addressed

mushtaqak · 2017-09-22T06:55:50Z

edxval/models.py

+        """
+        Returns unicode representation of provider credentials state for an organization.
+
+        NOTE: Message will look like below:


I am not sure if this the best way ?

mushtaqak · 2017-09-22T06:59:00Z

edxval/tests/test_api.py

+        {'org': 'MAX', 'provider': '3PlayMedia', 'exists': True},
+    )
+    @unpack
+    def test_credentials_state_update(self, **kwargs):


I think we missed this test for No org found case?

mushtaqak

@muhammad-ammar This is almost ready. I haven't still seen the TranscriptPreference model change that is required to store video_source_language

mushtaqak · 2017-09-22T07:01:03Z

edxval/models.py

+        """
+        Returns unicode representation of provider credentials state for an organization.
+
+        NOTE: Message will look like below:


I am not sure if this is the best way to show record name ?

muhammad-ammar · 2017-09-22T07:24:24Z

edxval/tests/test_api.py

+        {'org': 'edx', 'provider': '3PlayMedia', 'exists': True},
+    )
+    @unpack
+    def test_credentials_state_update(self, **kwargs):


@mushtaqak updated this test for an existing org.

mushtaqak · 2017-09-22T07:59:03Z

edxval/models.py

@@ -605,6 +605,55 @@ def __unicode__(self):
        return u'{course_id} - {provider}'.format(course_id=self.course_id, provider=self.provider)


+class OrganizationTranscriptCredentialsState(models.Model):


Also, we should probably use TimeStampedModel to keep track of created and modified time

Qubad786

@muhammad-ammar It is looking good except few minor concerns.

Qubad786 · 2017-09-22T14:48:25Z

edxval/models.py

@@ -605,6 +605,55 @@ def __unicode__(self):
        return u'{course_id} - {provider}'.format(course_id=self.course_id, provider=self.provider)


+class OrganizationTranscriptCredentialsState(models.Model):


I don't think that the name should specify the inner relations as such --> I vote for ThirdPartyTranscriptCredentialsState. Relation would be obvious by looking at its fields.

Qubad786 · 2017-09-22T14:53:28Z

edxval/models.py

+        """
+        Update or create credentials state.
+        """
+        instance, created = cls.objects.update_or_create(


Qubad786 · 2017-09-22T15:00:13Z

edxval/models.py

+        unique_together = ('org', 'provider')
+
+    org = models.CharField(verbose_name='Course Organization', max_length=32)
+    provider = models.CharField(


@muhammad-ammar we don't need to verify provider on pre-save signal. Model will automatically raises Exception if the you try to save it with a Provider which is not in its choices.

This is not happening. Model allows to save the incorrect data if I remove the signal.

Qubad786 · 2017-09-22T15:00:35Z

edxval/models.py

+        )
+
+
+@receiver(models.signals.pre_save, sender=OrganizationTranscriptCredentialsState)


No need for this.

Qubad786 · 2017-09-22T15:03:44Z

edxval/models.py

+            edX has no 3PlayMedia credentials
+        """
+        return u'{org} {state} {provider} credentials'.format(
+            org=self.org, provider=self.provider, state='has' if self.exists else 'has no'


doesn't have instead of has no sounds better.

Qubad786 · 2017-09-22T15:09:41Z

edxval/api.py

+        provider (unicode): transcript provider
+
+    Returns:
+        dict: contains the information regarding organization's transcript provider credentials


organization's --> organization-specific

Qubad786 · 2017-09-22T15:13:48Z

edxval/api.py

+    {
+        'org': 'edX',
+        'Cielo24': False,
+        '3PlayMedia': True,


@muhammad-ammar this is also an idea, can we just change our model like this?

org has_3play_creds has_cielo_creds

Qubad786 · 2017-09-22T15:16:16Z

edxval/api.py

+        provider (unicode): transcript provider
+        exists (bool): state of credentials
+    """
+    OrganizationTranscriptCredentialsState.update_or_create(org, provider, exists)


return something? like rerialized updated or created record?

no need, looks good.

Qubad786 · 2017-09-22T15:16:55Z

edxval/api.py

+        return credentials_state_info
+
+
+def update_organization_transcript_credentials_state(org, provider, exists):


update_3rd_party_transcript_credentials_state_for_org

Qubad786 · 2017-09-22T15:20:30Z

edxval/tests/test_api.py

+        Verify that `update_organization_transcript_credentials_state` method works as expected
+        """
+        api.update_organization_transcript_credentials_state(
+            org=kwargs['org'], provider=kwargs['provider'], exists=kwargs['exists']


simpler: api.update_organization_transcript_credentials_state(**kwargs)

mushtaqak · 2017-09-25T12:01:45Z

edxval/models.py

+    class Meta:
+        unique_together = ('org', 'provider')
+
+    org = models.CharField(verbose_name='Course Organization', max_length=32)


max_length=50 to be consistent with edx-pipeline

This has been done in #102

mushtaqak · 2017-10-03T07:47:35Z

edxval/migrations/0008_auto_20170922_0238.py

@@ -0,0 +1,28 @@
+# -*- coding: utf-8 -*-


This would also need to change as 0006 migration has been renamed in #102

muhammad-ammar · 2017-10-05T06:05:16Z

@muzaffaryousaf @Qubad786 @mushtaqak Feedback addressed. Whoever is using this needs to update the hash and api function names.

muzaffaryousaf · 2017-10-05T07:45:27Z

@Qubad786 @mushtaqak can you guys finalize this PR as per the usage in our other branches.

muzaffaryousaf · 2017-10-06T11:12:33Z

@mushtaqak @Qubad786 This needs review from you guys.

Qubad786 · 2017-10-06T12:51:45Z

edxval/models.py

+
+        NOTE: Message will look like below:
+            edX has Cielo24 credentials
+            edX has no 3PlayMedia credentials


nit: edX doesn't have 3PlayMedia credentials.

Qubad786

Looking great 👍! I've just a single concern about the validation.

Qubad786 · 2017-10-06T13:03:03Z

edxval/models.py

+        Update or create credentials state.
+        """
+        instance, created = cls.objects.update_or_create(
+            org=org, provider=provider,


nit: proper line break for kwargs.

Qubad786 · 2017-10-06T13:08:35Z

edxval/models.py

+    Validate that input provider value must be one of TranscriptProviderType.CHOICES.
+    """
+    if instance.provider not in dict(TranscriptProviderType.CHOICES).keys():
+        raise ValidationError('Invalid transcript provider value')


@muhammad-ammar I think its alright to remove this signal. We can validate it in api utils which are responsible for creating an actual state record in db, Or even we may not validate as its getting validated from the view which is calling val's api.

Also, if we look at Video model, status is just CharField without choices.

mushtaqak

@muhammad-ammar Looking great, just one thing to use TimeStampedModel and may be a better name for migration ?

mushtaqak · 2017-10-10T07:09:58Z

edxval/migrations/0007_auto_20171005_0154.py

@@ -0,0 +1,28 @@
+# -*- coding: utf-8 -*-


I think we should rename the migration to better name ? just as we did for 006_

mushtaqak · 2017-10-10T07:12:15Z

edxval/models.py

@@ -612,6 +612,55 @@ def __unicode__(self):
        return u'{course_id} - {provider}'.format(course_id=self.course_id, provider=self.provider)


+class ThirdPartyTranscriptCredentialsState(models.Model):


We should use TimeStampedModel

muhammad-ammar · 2017-10-10T09:00:09Z

@mushtaqak @Qubad786 Feedback addressed.

muhammad-ammar · 2017-10-10T12:05:57Z

@mushtaqak @Qubad786 Feedback addressed.

Qubad786

Just one concern.

Qubad786 · 2017-10-10T12:19:42Z

edxval/api.py

+            credential.provider: credential.exists
+            for credential in ThirdPartyTranscriptCredentialsState.objects.filter(**query_filter)
+        }
+    except ThirdPartyTranscriptCredentialsState.DoesNotExist:


filter won't raise ThirdPartyTranscriptCredentialsState.DoesNotExist. We are good without try: except.. here.

I think just return the following would work.

{ credential.provider: credential.exists for credential in ThirdPartyTranscriptCredentialsState.objects.filter(**query_filter) }

Qubad786

👍 !

mushtaqak

👍 Good to go

EDUCATOR-1376

EDU-1369

Remove subtitles code from VAL.

mushtaqak

👍 once we increment the VAL version

…/edx-val into ammar/org-specific-credentials-state

mushtaqak suggested changes Sep 21, 2017

View reviewed changes

muhammad-ammar force-pushed the ammar/org-specific-credentials-state branch from 814fbd8 to 3ab8aa5 Compare September 22, 2017 06:41

mushtaqak reviewed Sep 22, 2017

View reviewed changes

mushtaqak suggested changes Sep 22, 2017

View reviewed changes

muhammad-ammar force-pushed the ammar/org-specific-credentials-state branch 2 times, most recently from d186312 to 27ef47d Compare September 22, 2017 07:16

muhammad-ammar commented Sep 22, 2017

View reviewed changes

muhammad-ammar requested review from muzaffaryousaf and Qubad786 September 22, 2017 07:49

mushtaqak reviewed Sep 22, 2017

View reviewed changes

Qubad786 suggested changes Sep 22, 2017

View reviewed changes

mushtaqak reviewed Sep 25, 2017

View reviewed changes

muhammad-ammar force-pushed the ammar/org-specific-credentials-state branch from 27ef47d to 2b85892 Compare October 3, 2017 07:41

mushtaqak suggested changes Oct 3, 2017

View reviewed changes

muhammad-ammar force-pushed the ammar/org-specific-credentials-state branch from 2b85892 to f5182f8 Compare October 4, 2017 07:51

Qubad786 reviewed Oct 6, 2017

View reviewed changes

Qubad786 approved these changes Oct 6, 2017

View reviewed changes

mushtaqak suggested changes Oct 10, 2017

View reviewed changes

muhammad-ammar force-pushed the ammar/org-specific-credentials-state branch from 3b8222c to 27e9af9 Compare October 10, 2017 08:19

Qubad786 approved these changes Oct 10, 2017

View reviewed changes

Qubad786 reviewed Oct 10, 2017

View reviewed changes

Qubad786 approved these changes Oct 10, 2017

View reviewed changes

mushtaqak approved these changes Oct 10, 2017

View reviewed changes

muhammad-ammar force-pushed the ammar/org-specific-credentials-state branch from 3e1b105 to 2222670 Compare October 11, 2017 10:14

muzaffaryousaf force-pushed the mrehan/val-transcripts-backend-api branch from 6395732 to 0a9c823 Compare October 12, 2017 07:45

Qubad786 changed the base branch from mrehan/val-transcripts-backend-api to master October 18, 2017 07:47

Organization specific transcript credentials state

adc7696

EDUCATOR-1376

Qubad786 force-pushed the ammar/org-specific-credentials-state branch from 2222670 to adc7696 Compare October 18, 2017 07:59

muzaffaryousaf added 3 commits October 26, 2017 13:02

Remove subtitles code from VAL.

4ad6348

EDU-1369

Update admin class for org transcript credentials.

4b7f0f2

EDU-1369

Merge pull request #106 from edx/remove-subtitles

1ed30a3

Remove subtitles code from VAL.

mushtaqak approved these changes Oct 26, 2017

View reviewed changes

muzaffaryousaf added 2 commits October 26, 2017 17:05

Bump the version.

ad1c2da

Merge branch 'ammar/org-specific-credentials-state' of github.com:edx…

b27cdc0

…/edx-val into ammar/org-specific-credentials-state

muzaffaryousaf merged commit 13c7186 into master Oct 26, 2017

muzaffaryousaf deleted the ammar/org-specific-credentials-state branch October 26, 2017 13:18

		@@ -605,6 +605,48 @@ def __unicode__(self):
		return u'{course_id} - {provider}'.format(course_id=self.course_id, provider=self.provider)


		class OrganizationTranscriptCredentialsState(models.Model):



		@receiver(models.signals.pre_save, sender=OrganizationTranscriptCredentialsState)
		def validate_provider(sender, instance, **kwargs): # pylint: disable=unused-argument

		@@ -143,6 +143,55 @@ def update_video_status(edx_video_id, status):
		video.save()


		def get_organization_transcript_credentials_state(org, provider=None):

		@@ -605,6 +605,55 @@ def __unicode__(self):
		return u'{course_id} - {provider}'.format(course_id=self.course_id, provider=self.provider)


		class OrganizationTranscriptCredentialsState(models.Model):

		return credentials_state_info


		def update_organization_transcript_credentials_state(org, provider, exists):

		@@ -612,6 +612,55 @@ def __unicode__(self):
		return u'{course_id} - {provider}'.format(course_id=self.course_id, provider=self.provider)


		class ThirdPartyTranscriptCredentialsState(models.Model):

Organization specific transcript credentials state #99

Organization specific transcript credentials state #99

Conversation

muhammad-ammar commented Sep 20, 2017

mushtaqak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhammad-ammar commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mushtaqak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhammad-ammar Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qubad786 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qubad786 Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhammad-ammar commented Oct 5, 2017

muzaffaryousaf commented Oct 5, 2017

muzaffaryousaf commented Oct 6, 2017

Choose a reason for hiding this comment

Qubad786 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qubad786 Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

mushtaqak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhammad-ammar commented Oct 10, 2017

muhammad-ammar commented Oct 10, 2017

Qubad786 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qubad786 left a comment

Choose a reason for hiding this comment

mushtaqak left a comment

Choose a reason for hiding this comment

mushtaqak left a comment

Choose a reason for hiding this comment

muhammad-ammar Sep 22, 2017 •

edited

Loading

Qubad786 Sep 22, 2017 •

edited

Loading

Qubad786 Oct 6, 2017 •

edited

Loading