Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

SE-1069 Module Engagement: handle case when no elasticsearch host is configured #751

Conversation

samuelallan72
Copy link

@samuelallan72 samuelallan72 commented Aug 8, 2019

Runs just ModuleEngagementRosterPartitionTask instead of the full ModuleEngagementRosterIndexTask if no elasticsearch host is configured.

JIRA tickets: OSPR-3784

Discussions: open-craft#10

Dependencies: None

Merge deadline: None

Testing instructions:

  1. don't configure an elasticsearch host (TODO: full instructions)
  2. verify that ModuleEngagementRosterPartitionTask shows in the ModuleEngagementWorkflowTask task logs, instead of ModuleEngagementRosterIndexTask

Author notes and concerns:

Reviewers

Settings

… configured

Upstreamable version of code drift contained in cloudera's branch,

1777df2
(cherry picked from commit ff19850)
@openedx-webhooks
Copy link

Thanks for the pull request, @swalladge! I've created OSPR-3784 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Aug 8, 2019
@codecov
Copy link

codecov bot commented Aug 8, 2019

Codecov Report

Merging #751 into master will decrease coverage by 0.01%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #751      +/-   ##
==========================================
- Coverage   74.57%   74.56%   -0.02%     
==========================================
  Files         207      207              
  Lines       23404    23413       +9     
==========================================
+ Hits        17453    17457       +4     
- Misses       5951     5956       +5
Impacted Files Coverage Δ
...s/tasks/tests/acceptance/test_module_engagement.py 36.95% <18.75%> (+1.95%) ⬆️
edx/analytics/tasks/insights/module_engagement.py 79.13% <25%> (-0.28%) ⬇️

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 c28f775...2dbcd65. Read the comment docs.

@natabene
Copy link

natabene commented Aug 8, 2019

@swalladge Thank you for your contribution. Please let me know once it is ready for our review.

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Aug 8, 2019
and ensures that the ModuleEngagementWorkflowTask.host parameter matches the base class parameter type.
Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

@swalladge 👍

In the process of getting the acceptance tests working, I discovered a bug in original commit from the client's branch, so we'll need to make sure this fix makes it there too.

  • I tested this by verifying that this code runs as expected on our client's instance, which does not have [elasticsearch] host configured.
    Also ran the updated acceptance tests from this branch:
    1. Set up the Analytics Pipeline docker devstack
    2. Check out this branch and install it:
      make analytics-pipeline-shell
      # From devstack/Makefile#L300
      sudo chown -R hadoop:hadoop /edx/app/analytics_pipeline
      source /edx/app/hadoop/.bashrc
      make install
      
    3. From the analytics-pipeline-shell, ran:
      make develop-local
      make docker-test-acceptance-local ONLY_TESTS=edx.analytics.tasks.tests.acceptance.test_module_engagement
      
  • I checked the code against the client's branch ff19850
  • I checked for accessibility issues N/A
  • Includes documentation: ModuleEngagementWorkflowTask.host field description provided, tasks are automatically documented from parameters.

@pomegranited
Copy link
Contributor

This is ready for edX review, @natabene

Uncertain what to do about the unit test coverage drop -- the only way I can think to unit test this basically mimics the code change itself. But we've added an acceptance test which verifies the config change is ok, so hopefully that is enough?

@natabene
Copy link

@edx/analytics Please review this when you have a chance.

@openedx-webhooks openedx-webhooks added awaiting prioritization and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Aug 12, 2019
@brianhw brianhw merged commit 747636a into openedx-unsupported:master Aug 14, 2019
@openedx-webhooks
Copy link

@swalladge 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

@brianhw
Copy link
Contributor

brianhw commented Aug 14, 2019

I think the unit test coverage drop is small enough to not be worth worrying about.

@samuelallan72 samuelallan72 deleted the samuel/module-engagement-no-elasticsearch-host branch August 15, 2019 00:23
@pomegranited
Copy link
Contributor

Thank you @brianhw !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

None yet

5 participants