Skip to content

Conversation

@symbolist
Copy link
Contributor

@symbolist symbolist commented Jun 15, 2018

Description: Moving some helpers method from https://github.com/edx/edx-platform/pull/18273 to the Completion Service.

JIRA: Link to JIRA ticket

Dependencies: None

Installation instructions: N/A

Testing instructions: N/A

Reviewers:

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed
  • PR author is listed in AUTHORS

Post merge:

  • Create a tag
  • Check new version is pushed to PyPi after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

@symbolist symbolist force-pushed the symbolist/complete-on-view-utils branch from bea8f71 to af8da47 Compare June 15, 2018 20:40
Copy link
Contributor

@jcdyer jcdyer left a comment

Choose a reason for hiding this comment

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

Only nits. Consider resolving them, but otherwise this is ready for upstream review.

def test_blocks_to_mark_complete_on_view(self):

block_1 = XBlock(Mock(), scope_ids=Mock(spec=ScopeIds))
block_1.location = UsageKey.from_string("i4x://edX/100/a/1").replace(course_key=self.course_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this use of replace the same as usage_key.map_into_course(course_key)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect so. I just copied this pattern from line 37 above.

block_2.location = UsageKey.from_string("i4x://edX/100/a/2").replace(course_key=self.course_key)
block_3 = XBlock(Mock(), scope_ids=Mock(spec=ScopeIds))
block_3.location = UsageKey.from_string("i4x://edX/100/a/3").replace(course_key=self.course_key)
block_3.completion_mode = XBlockCompletionMode.AGGREGATOR
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Consider giving these blocks descriptive names (E.g., s/block_3/aggregator_block/).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated names.


@ddt.data(1, 1000, 0)
def test_get_completion_by_viewing_delay_ms(self, delay):
def test_get_complete_on_view_delay_ms(self, delay):
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you made sure you caught all uses of this method in edx-platform?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. They are used in two views and there is is bokchoy coverage for both.

@symbolist symbolist force-pushed the symbolist/complete-on-view-utils branch from 85dfcd5 to 4987159 Compare June 18, 2018 11:37
Copy link
Contributor

@iloveagent57 iloveagent57 left a comment

Choose a reason for hiding this comment

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

👍

@symbolist
Copy link
Contributor Author

@iloveagent57 Can you please merge this as well? And if I create a tag, will that push a release to PyPi?

@iloveagent57
Copy link
Contributor

@symbolist yup, merging now. And yes, if you create a tag (I usually just use the github release feature) it will kick off a travis build that will push a new release to PyPI.

@iloveagent57 iloveagent57 merged commit 82d523c into openedx:master Jun 19, 2018
@symbolist symbolist deleted the symbolist/complete-on-view-utils branch June 19, 2018 13:46
@symbolist
Copy link
Contributor Author

@iloveagent57 Thanks! Looks like I do not have access to either push a tag or create a release. Can you please take care of this?

@iloveagent57
Copy link
Contributor

@symbolist yup, will do (I'll also make a note of this for future PRs).

@iloveagent57
Copy link
Contributor

https://pypi.org/project/edx-completion/ is now at the latest version, 0.1.7

@symbolist
Copy link
Contributor Author

@iloveagent57 Thanks!

@openedx-webhooks
Copy link

Although this pull request is already merged, I've created OSPR-5543 so that we can track it in Jira.

There is nothing you have to do. No action is needed from your side. Thanks again for your contribution.

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U and removed open-source-contribution PR author is not from Axim or 2U labels Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants