Skip to content

Conversation

@pomegranited
Copy link
Contributor

@pomegranited pomegranited commented Nov 9, 2016

Updates problem-builder to v2.6.2 to address performance concerns raised by problem-builder #119.

cf. open-craft/problem-builder#129

Sandbox URL:

Partner information: DavidsonX

Testing Instructions:

To test this change, compare the v2.6.2 sandbox from this PR with the v2.6.1 sandbox from https://github.com/edx/edx-platform/pull/13938.

  1. Navigate to the Problem Builder Demo course, section Features > Instructor Tools.
  2. Note that the list of course block options is the same for both sandboxes.

Reviewers

CC @ormsbee

Settings

EDXAPP_EXTRA_REQUIREMENTS:
  - name: git+https://github.com/open-craft/problem-builder.git@v2.6.2#egg=xblock-problem-builder==2.6.2

@openedx-webhooks
Copy link

Thanks for the pull request, @pomegranited! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=13953

Copy link
Contributor

@itsjeyd itsjeyd left a comment

Choose a reason for hiding this comment

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

@pomegranited Looking great. There's a couple comments left to address on open-craft/problem-builder#129; this PR will be good to go once you've done that 👍

  • I tested this: Course trees are the same on the different sandboxes. Also SSH'ed into the sandbox for this PR and verified that the correct version of problem-builder got installed (2.6.2).
  • I read through the code
  • I checked for accessibility issues N/A
  • Includes documentation N/A

@openedx-webhooks
Copy link

Thanks for the pull request, @pomegranited! I've created OSPR-1573 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 Nov 10, 2016
@pomegranited
Copy link
Contributor Author

jenkins run a11y

@gsong
Copy link
Contributor

gsong commented Nov 14, 2016

@pomegranited Can you please add the environments and the expected deployment date to the PR description? This will help with our internal workflow. Thank you.

@openedx-webhooks openedx-webhooks added product review PR requires product review before merging and removed needs triage labels Nov 14, 2016
@pomegranited
Copy link
Contributor Author

Hi @gsong , looks like we've got a problem-builder bugfix in this week's sprint, so this PR probably won't be necessary.

I'll close this and reference the new PR when one is ready.

For my reference, what do you mean by "environments"?

@gsong
Copy link
Contributor

gsong commented Nov 14, 2016

@pomegranited Environments = edx.org or edge.edx.org

@pomegranited
Copy link
Contributor Author

@gsong Ah gotcha! I know Problem Builder is deployed on edx.org, but I don't know whether it's on edge or not.. And I don't have course creation permissions on studio.edge.edx.org.

@bradenmacdonald Do you know?

@bradenmacdonald
Copy link
Contributor

@pomegranited Problem Builder is enabled on Edge, yes. However, from a quick look, it appears that pb-instructor-tool is not whitelisted on Edge as it is on edx.org, which is something we'll need. Would you be able to follow up on that?

@mtyaka mtyaka mentioned this pull request Nov 17, 2016
3 tasks
@pomegranited
Copy link
Contributor Author

@bradenmacdonald Sure thing, I've submitted DEVOPS-5075 requesting the xblock-configuration change be run on edge.edx.org as well.

@pomegranited
Copy link
Contributor Author

FYI @gsong CC @ormsbee

https://github.com/edx/edx-platform/pull/14013 removes the need for this PR to be merged and deployed, so I'm closing this PR.

@pomegranited
Copy link
Contributor Author

@gsong Sorry, I only answered half of your question above.

For the desired Merge Deadline, we don't usually provide one unless there is a critical deadline to request. Would you rather we always specified something in our PRs, even if it just says "Merge Deadline: none" ? I'm updating our PR template now, and can make sure this change is included.

@gsong
Copy link
Contributor

gsong commented Nov 18, 2016

@pomegranited I think that would be valuable information to help the teams prioritize. So if something isn't urgent, you can definitely put None, or conversely "ASAP because this is a critical bug fix!".

🙂

@pomegranited
Copy link
Contributor Author

@gsong Lovely, I'll update our docs to reflect this. Thanks for your insight!

@pomegranited pomegranited deleted the jill/pb-2.6.2 branch November 16, 2017 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U product review PR requires product review before merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants