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.

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

Issue 257: Configurable default player ID #262

Merged
merged 9 commits into from
Jul 14, 2017
Merged

Conversation

wowkalucky
Copy link
Collaborator

Customer requested the ability to set the initial default value to "player_id" xblock field.

Now we check if the DEFAULT_PLAYER_ID is set, we use it and settings falls back to 'default' if it's not.
This feature implementation includes per-microsite default settings ability.

@codecov
Copy link

codecov bot commented Jul 13, 2017

Codecov Report

Merging #262 into dev will increase coverage by 0.04%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev    #262      +/-   ##
=========================================
+ Coverage   85.76%   85.8%   +0.04%     
=========================================
  Files          41      41              
  Lines        2578    2593      +15     
  Branches       26      26              
=========================================
+ Hits         2211    2225      +14     
- Misses        364     365       +1     
  Partials        3       3
Flag Coverage Δ
#acceptance 88.91% <90.9%> (+0.02%) ⬆️
#unit 83.99% <90.9%> (+0.05%) ⬆️
Impacted Files Coverage Δ
video_xblock/video_xblock.py 67.69% <0%> (-0.27%) ⬇️
video_xblock/tests/unit/test_mixins.py 100% <100%> (ø) ⬆️
video_xblock/tests/unit/test_utils.py 100% <100%> (ø) ⬆️
video_xblock/mixins.py 88% <80%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b7b8aa...83762a1. Read the comment docs.

@wowkalucky wowkalucky requested a review from z4y4ts July 13, 2017 08:07
@wowkalucky wowkalucky force-pushed the 257-configurable-player-id branch from bd5022d to b86561f Compare July 13, 2017 10:12
@wowkalucky wowkalucky force-pushed the 257-configurable-player-id branch from 1a4ad5d to 102417d Compare July 13, 2017 12:02
Copy link

@z4y4ts z4y4ts left a comment

Choose a reason for hiding this comment

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

Looks good.
Couple things need to be addressed before the merge.

submit_data.setdefault(key, value)
# if key is present in submit data but set to empty value:
if key in submit_data and not submit_data[key]:
submit_data[key] = value
Copy link

Choose a reason for hiding this comment

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

It looks like this entire for-loop can be simplified down to:

submit_data.update(self.settings)
return submit_data

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dict.update() overrides existed values but wanna to set only those which are empty or are absent.

Copy link

Choose a reason for hiding this comment

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

Ah, ok. Good point.

@@ -133,3 +133,17 @@ def normalize_transcripts(transcripts):
Transcript = namedtuple('Transcript', [
'id', 'label', 'lang', 'lang_id', 'content', 'format', 'video_id', 'source', 'url'
])


def get_current_site_name():
Copy link

Choose a reason for hiding this comment

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

Move this method to SettingsMixin, it belongs there.

@@ -530,22 +530,37 @@ class SettingsMixin(XBlock):

Sample default settings in /edx/app/edxapp/cms.env.json:
"XBLOCK_SETTINGS": {
"video_xblock": {
"domain.com": {
Copy link

Choose a reason for hiding this comment

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

Here and everywhere, use example.com instead of example.com.

Example.com is established to be used for illustrative examples in documents.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't know that!

@wowkalucky wowkalucky merged commit eab48d3 into dev Jul 14, 2017
@wowkalucky wowkalucky deleted the 257-configurable-player-id branch July 14, 2017 13:05
wowkalucky added a commit that referenced this pull request Aug 3, 2017
* Reimplement population fields with default values

* Cover with unittest 3PlayMedia get_3pm_transcripts_list API failure case

* Cover with unittests subdomain fatching utility

* Refactor XBLOCK_SETTINGS format

* Merge branch 'dev' of https://github.com/raccoongang/xblock-video into 257-configurable-player-id

* Fix code review complaints
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants