[BB-6046] Prepare opencraft-release/maple.3 branch for edx-platform
#471
Conversation
There was a problem hiding this comment.
@pkulkark, could you please move this spreadsheet to the PR's description? Also, please include upstream PRs there (like we did in open-craft/configuration#193) - gathering these in one place is going to make our lives much easier.
I'm copy-pasting my remark from the ticket (context in the first point below):
Isn't testing these features a part of this task, provided that it is possible to test them? Even clean cherry-picks can still be broken if something else changes (imports, underlying functions/methods, etc.).
We are missing two things:
- The sandbox - we should be able to reuse this one by simply changing the commit hash of the
edx-platform. We need to fix the code first - the test pipeline indicates that some imports are indeed broken. It will prevent migrations from running. - A way to test it. We should perform the instance checklist, but we should also verify that these cherry-picked features work correctly in Maple. If a client-specific change will be too complex, then we can let client owners know that they will need to test in a specific Stage environment. Does it sound reasonable?
cc: @navinkarkera
pkulkark
force-pushed
the
pooja/cherrypick-code-drift-to-maple
branch
2 times, most recently
from
4063d57 to
f371e32
Compare
|
@Agrendalath I've added the commits in the description. And I've fixed the imports. I have updated the commit hash and started a new appserver deployment on the test sandbox. I will post an update once it's ready for testing. |
|
@Agrendalath The test sandbox was deployed successfully with the edx-platform commit set to this PR branch. I also completed the instance checklist and everything seems to be working fine. I just realised however, that the configuration branch for the sandbox was set to "viadanna/maple.3" and not the one from open-craft/configuration#193. I'm not sure if we need to test with the configuration repo branch as well or would that fall under the scope of BB-6112? CC @navinkarkera |
Agrendalath
force-pushed
the
pooja/cherrypick-code-drift-to-maple
branch
from
d97d726 to
ffd6e45
Compare
There was a problem hiding this comment.
@pkulkark, I:
- Updated the description.
- Dropped incorrect commits from the last 5 commits in the table and cherry-picked correct references.
- Rebased this branch to resolve conflicts.
I'm leaving my initial review below.
General notes:
- Please use the
git commit -xevery time. It helps us to determine where the commit came from because otherwise there is no reference to the original commit in the commit body. It might not sound too useful, but GitHub shows all repositories in which a specific commit, so it's easy to verify which named releases included it. Otherwise, we should leave a different reference, e.g. to the upstream PR. This applies to multiple commits from this PR. - Please try to avoid merging the upstream branch into the PR (i.e. c962f0f3e3b6f5e3cc4714b08c2a7707aadec3fa). This is tricky to handle when we're cherry-picking things. Let's use rebase instead - I rebased this branch on
open-release/maple.masterto resolve conflicts and to drop this commit. - Please always cherry-pick features from either the backport PR or an upstream one. Some of these cherry-picked changes contain different diffs than we are proposing/have merged to the upstream. I haven't changed how many of these could affect the functionality, but we shouldn't add code drift to our code drift. I've added a table below with my remarks.
Commit-specific notes:
| Ticket | Commit | Expected commit | Notes |
|---|---|---|---|
| BB-4951 | openedx/edx-platform@1a21824 | openedx/edx-platform@373f328 | Why would we want to cherry-pick a merge commit? |
| BB-5815 | 6ca5a45 | https://github.com/edx/edx-platform/pull/27857 | These commit messages do not contain anything special, so let's just squash them on the original branch, and cherry-pick a single commit from there. Also, this branch is conflicting with master, so we should resolve it there to avoid adding the redundant f371e32ee305f5c997617625fdd58f15dad92479. |
| BB-5271 | 406b0ab | dd69d8e54d5b43b869835d0f877744e8776d4849 | Why would we want to cherry-pick an internal merge commit? |
| BB-5819 | 7da10ab | openedx#29850 | Let's squash first, and then cherry-pick. |
| BB-5429 | a20514c | openedx#30314 | Let's squash first, and then cherry-pick. |
Ticket-specific notes:
- BB-5849: I believe you've accidentally picked the name from the fixup commit in the cherry-pick. We should use the commit message from the base one, as it preserves the context. The same remark applies to [BACKPORT] fixup! feat: options for excluding courses from search (#28518) openedx/edx-platform#30373. Also, we don't have any ticket to track this backport.
- BB-4381: are we planning to backport this to Maple? I don't see any open tickets, so it'll likely be forgotten as soon as we merge this PR.
- FAL-867: I can see that we have closed it, as we are no longer working with this client. CC: @pomegranited, as you might have more context on this item.
- BB-5815: this entry is duplicated and uses two different commits. Please remove one of them after addressing my "Commit-specific notes".
- BB-5819: this ticket is closed. How are we tracking the upstream PR?
- SE-5299: this ticket is closed. How are we tracking upstream PRs?
- BB-6041: should we create a follow-up for the backport?
- BB-1389: once we resolve BB-5815, we can squash the leftovers from f371e32ee305f5c997617625fdd58f15dad92479 into this cherry-pick.
Agrendalath
changed the base branch from
opencraft-release/maple.3
to
opencraft-release/maple.master
Agrendalath
changed the base branch from
opencraft-release/maple.master
to
opencraft-release/maple.3
Agrendalath
force-pushed
the
pooja/cherrypick-code-drift-to-maple
branch
from
ffd6e45 to
021159f
Compare
Good catch. Regardless of which ticket will include testing these changes, we need to test them anyway before merging this. I changed the commit to the HEAD of the branch* from open-craft/configuration#193, updated its settings to a new template (we are preparing Ocim Maple support in open-craft/opencraft/pull#901), and spawned a new AppServer, * I've also found an odd bug (feature?) in Ocim: SE-5566 (cc: @0x29a). |
I've commented on FAL-867, but TL;DR we could make a case to upstream this now given the CC/community changes. However without a client who needs it, it's questionable whether we should. So I guess, remove it, and if anyone notices, we resurrect and upstream it? |
pkulkark
force-pushed
the
pooja/cherrypick-code-drift-to-maple
branch
3 times, most recently
from
0dd40c8 to
ba18fb6
Compare
|
@Agrendalath Thanks so much for the additional help and cleanup. I initially started cherrypicking the commits from our internal lilac.2 branch, that's why it had merge commits and some that didn't match the upstream changes. I later tracked down the upstream commits and replaced it but I might've missed some. Anyway here is what I've done now:
There were cases where either some code changes were missed to be upstreamed or the upstream PR were merged to master (but not backported to maple), which led to the tickets being marked as closed. There are still some upstream PRs which are yet to be reviewed and so can't be backported to maple yet. We'll need a better process for this. Also the test sandbox was deployed successfully. I'll go through the instance checklist once again on it. But for a more detailed testing (like checking if the changes in the commits work or not) I think a follow-up task would be better, since those will involve the changes in the configuration repo as well. What do you think? |
|
@Agrendalath I went through the instance checklist on the latest test sandbox, but it looks like Account MFE is not working. It was working on the previous one (i.e. before changing the configuration version) so it looks like this is something that should be addressed in BB-6112? CC @navinkarkera |
@pkulkark, I checked what's wrong, and it looks like Current: MFES:
- name: gradebook
repo: frontend-app-gradebook
public_path: /gradebook/Should be: MFES:
- name: profile
repo: frontend-app-profile
public_path: "/profile/"
- name: gradebook
repo: frontend-app-gradebook
public_path: "/gradebook/"
- name: account
repo: frontend-app-account
public_path: "/account/"
- name: learning
repo: frontend-app-learning
public_path: "/learning/" |
pkulkark
force-pushed
the
pooja/cherrypick-code-drift-to-maple
branch
from
ba18fb6 to
3fa7005
Compare
I thought that was only for learning MFE. Anyway this is breaking the accounts page (results in 404). Is there some configuration that forces to not use the MFE for the accounts endpoint? CC @navinkarkera @0x29a |
pkulkark
force-pushed
the
pooja/cherrypick-code-drift-to-maple
branch
5 times, most recently
from
158f733 to
e32336c
Compare
Agrendalath
force-pushed
the
pooja/cherrypick-code-drift-to-maple
branch
3 times, most recently
from
12ac208 to
1d8c206
Compare
pkulkark
force-pushed
the
pooja/cherrypick-code-drift-to-maple
branch
from
1d8c206 to
a4d1f35
Compare
Agrendalath
force-pushed
the
pooja/cherrypick-code-drift-to-maple
branch
4 times, most recently
from
c8b954f to
293874b
Compare
… endpoint (cherry picked from commit ba4ae79)
(cherry picked from commit a26bb85)
edx-platform supports COMPREHENSIVE_THEME_LOCALE_PATHS setting, which appends paths to the end of LOCALE_PATHS, but there's currently no way to add additional paths to the start of the list. https://tasks.opencraft.com/browse/SE-5299
This: 1. Introduces a variable for the Course Outline view in Studio. A custom theme can override it to add new editors. 2. Exports a function for creating new editor modals. A custom theme can use it to create editors without adding boilerplate code. 3. Adds a pluggable override for XBlock fields that are passed to the Studio. Without this, custom editors in Studio cannot retrieve values of XBlock fields. (cherry picked from commit e633cc9)
edx/edx-platform#24365 has changed the completion mode of these blocks. Before Koa, it was sufficient to view the block to get a completion checkmark. Since Koa, all children of the block must be completed. This adds a toggle to change the completion behavior back to the previous one so that the user experience can be consistent if needed. (cherry picked from commit d05e5c6)
Most tags that could contain solutions or hints were already being removed, but the regex did not include the case when they contained attributes. (cherry picked from commit 0ef57eb)
This makes the reset button to refresh the contents of a Randomized Content Block (RCB) without reloading the full page by fetching a new set of problems in the "reset" response and replacing the DOM contents. The reset button returns the student view as a string and the client uses the HtmlUtils package to replace the contents and reinitializes the XBlock. This allows students to use the RCB as a flash card system. Co-authored-by: tinumide <[email protected]>
(cherry picked from commit c812a6c1d5c0961900507a6e7abe3d0f3b8a7570)
Adds the ability to edit the default course description shown in certificates. (cherry picked from a89baafe0575c94fac0cb4ce9db1d38ce0b71bc6)
This feature help to configure the date formatt in Schedule and Details settings page. Signed-off-by: Farhaan Bukhsh <[email protected]> Co-authored-by: Joseph Curtin <[email protected]>
- add pytest_hooks import back in common/lib/conftest.py. This import was removed during refactoring work (cherry picked from commit b4ac8d2)
…enedx#29676) (cherry picked from commit 4e22a38)
- add verify unit test workflow also - use composite github action for syncing (cherry picked from commit 6ccadbb)
Agrendalath
force-pushed
the
pooja/cherrypick-code-drift-to-maple
branch
from
2955a22 to
b9a5e74
Compare
Agrendalath
force-pushed
the
pooja/cherrypick-code-drift-to-maple
branch
from
b9a5e74 to
1a7ea09
Compare
There was a problem hiding this comment.
👍
- I tested this: the sandbox instance
- I read through the code
- I checked for accessibility issues: n/a
- Includes documentation: n/a
- I made sure any change in configuration variables is reflected in the corresponding client's
configuration-securerepository: n/a
Description
This PR prepares the shared
opencraft-release/maple.3branch for our fork of https://github.com/openedx/edx-platform.Code-drift commits
opencraft-release/maple.3.edx-searchto3.4.0.This PR also contains a backport of openedx/edx-platform@6ccadbb (openedx#29979) to fix running the CI from a fork.