Closed shaidar closed 2 years ago
@pdpinch Can we have someone from Arbisoft spend some time on this as it's being triggered fairly regularly and would be good to address prior to the Spring semester.
@asadiqbal08 Assigning it to you since you've been working on it.
Thanks @arslanashraf7 for that.
Demo: 'VerticalBlockWithMixins' object has no attribute 'get_metadata'
@pdpinch @shaidar Please take a look over the video before reading that comment for better understanding, For code point of view here are some details but I am not sure why such behaviour was required earlier but issue is pretty falls with Learning MFE
Two main things.
1- The following conditions for just making the navigation <Links>
, I am not sure why JumpNavMenuItem
component is not allowed to be there by adding this check.
2- The changes in the PR that had merged for Link from router to fix path based routing issue
.
It makes sense to use the <Link>
but in React the problem this creates is say you are on /blah
, while clicking on your Link will go to /page
, clicking on the <a href='page' />
will take you to /blah/page
(reference)
And also clicking on anchor <a>
earlier was refreshing the page and fetch metadata again so the exception did not occur but in case of <Link path=...>
caused this exception to occur as react routes
mapping has changed so when the call goes fetch sequences data then the sequenceId here points to vertical+block
(bit odd behaviour) and call failed to OpenEdx that is expecting a sequence
instead of a vertical
usage key.
Thanks for sharing the video. That's really helpful.
I believe that the edX product team wants to roll out the jump navigation feature slowly over time, but I'm not sure why. Maybe because they are concerned about technical issues, or they are concerned about user response. It's not unusual for them to release features to subsets of users first, to gather feedback.
Since you mention @arslanashraf7's PR, can you discuss with him? I believe he was trying to fix the jump navigation so it would work on systems like ours where the MFE is loaded via a path on the same server, instead of unique domain as its configured at edx.org.
Since you mention @arslanashraf7's PR, can you discuss with him? I believe he was trying to fix the jump navigation so it would work on systems like ours where the MFE is loaded via a path on the same server, instead of unique domain as its configured at edx.org.
Had a discussion with @asadiqbal08. Discussed why that Link
was added to support PUBLIC_PATH
which wasn't directly supported in anchor tags. Also, proposed a simpler/faster solution to him for using anchor
tags but with getConfig().PUBLIC_PATH
as a prefix. (This will get us back to how it was working before except that we won't see 404
s life before). Something like for Home/sequence/sections
breadcrumbs:
<a className="text-primary-500" href={`${getConfig().PUBLIC_PATH}course/${courseId}/${defaultContent.id}`}>
{defaultContent.label}
</a>
I believe, @asadiqbal08 might add/propose more details of any other possible solutions that might work for us.
I have created a PR for OpenEdx community to take a look. The solution at the moment is straight forward. I tried to update the routes but it was causing some other problems as you can see in Learning MFE Here as they are updating the History
object on basis of several conditions such as for avoiding infinite loop.
Secondly, as @pdpinch mentioned, edX product team wants to roll out the jump navigation feature slowly over time so it make sense to keep the anchor
tag logic there as it was earlier and wait for them to use the jump navigation component that seems me much attractive to move.
@arslanashraf7 take a look over the PR and let me know if you have any concerns.
@asadiqbal08 Mostly looks fine, A similar change like this will be needed for the Course breadcrumb too. Plus, there are some indentation errors that you might take a look at.
A similar change like this will be needed for the Course breadcrumb too.
Sorry I did not get that
Can you tell me why this change required below in this file ?
Can you tell me why this change required below in this file ?
Since we are reverting Link
API changes & moving to an alternate solution. That change for Course
breadcrumb was also done in the last PR where we moved from using anchor
tags to Link
.
Plus, If you think the router's Link
API isn't going to affect Course
breadcrumb then don't change. I just wanted to be on safer side.
I guess we need to revert that link too as validation was complaining about the BrowserRouter
.
We will need to backport Mike Terry's fixes to maple, in order to fix this in Residential MITx and xPRO:
https://github.com/openedx/build-test-release-wg/issues/129
FYI, this seems to be working on mitxonline. We should go ahead with the backporting to maple.
This should be fixed now. We haven't seen any sentry errors in 4 days.
This error seems to be triggered quite frequently on different courses since the Maple upgrade.