Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bookmark tool is not saving bookmarks #270

Closed
1 task done
jswope00 opened this issue Apr 22, 2023 · 7 comments
Closed
1 task done

Bookmark tool is not saving bookmarks #270

jswope00 opened this issue Apr 22, 2023 · 7 comments
Assignees
Labels
Milestone

Comments

@jswope00
Copy link

jswope00 commented Apr 22, 2023

Bookmark functionality appears to not be working in Palm demo instance.

Steps to recreate:

  1. Log in as a student user to a published, released course.
  2. Bookmark a page using the 'bookmark this page' button under a unit title.
  3. The page will appear as though it has been bookmarked; https://prnt.sc/ruM5FnrswFdA
  4. Refresh the page. Note the bookmark has disappeared: https://prnt.sc/WyWEBQtSsDPM
  5. Go to the bookmarks page for the course. Note that no bookmarks have been saved:

image

PRs

  1. core contributor open-source-contribution
    arbrandes
@Henrrypg
Copy link

assign me

@sambapete
Copy link

It's no longer working in Olive.3 according to some recent tests.
See https://discuss.openedx.org/t/olive-bookmarks-failing/10022

@jalondonot jalondonot added this to the Palm.1 milestone May 15, 2023
@DeanJayMathew
Copy link

Hi @Henrrypg

Are you still pursuing this?

@regisb
Copy link
Contributor

regisb commented May 17, 2023

When we click "bookmark this page", the learning MFE makes a POST to http://apps.local.overhang.io/api/bookmarks/v1/bookmarks/. Of course this page does not exist because there is no MFE running at "/api". So the MFE server does what it usually does when it doesn't know what to do with a request: make an empty 200 response... (don't blame me, this is part of the MFE spec!)

The POST should be made to http://local.overhang.io/api/bookmarks/v1/bookmarks/ (the lms container), and not http://apps.local.overhang.io/... (the mfe container). But I do not understand why this is the case. It does seem like the learning MFE is using the LMS_BASE_URL: https://github.com/openedx/frontend-app-learning/blob/open-release/olive.master/src/courseware/course/bookmark/data/api.js

The behaviour in "dev" is interesting. When I click "bookmark this page" a call is made to http://localhost:18000/csrf/api/v1/token instead of http://lms.overhang.io:8000/csrf/api/v1/token. This suggests that the LMS_BASE_URL setting is incorrectly read or defined by the bookmarks tool.

When I add a breakpoint to the learning MFE I see that the configuration seems to come from build time, not runtime:

Screenshot from 2023-05-17 13-25-56
Screenshot from 2023-05-17 13-26-15

(apologize the crude debugging, my frontend skills are not that great...)

My interpretation is that the bookmarks tool does not make use of the runtime configuration. How can I confirm this intuition?

@Henrrypg
Copy link

Hello @DeanJayMathew, i was testing and my first approach was same as @regisb. But i'm having problems with my frontend and MFEs skills to get a solutions for that. I'll unassign me to left it to somebody with more frontend skills and i'll take another task.

@Henrrypg Henrrypg removed their assignment May 17, 2023
ghassanmas added a commit to ghassanmas/frontend-app-learning that referenced this issue May 29, 2023
  This change makes it possible to use the latest  LMS_BASE_API
  if it was changed because of dynamic config API, which is the
  default case of tutor.

  This changes closes openedx/wg-build-test-release/issues/270

   Fixes that are simlar to this
  - gradebook openedx/frontend-app-gradebook/pull/290
  - course authoring openedx/frontend-app-authoring/pull/389
@ghassanmas
Copy link
Member

A propose changes to fix issue, has been summited at:

@arbrandes arbrandes moved this to In review in Frontend Working Group May 30, 2023
@github-project-automation github-project-automation bot moved this from In review to Closed in Frontend Working Group May 31, 2023
regisb pushed a commit to regisb/frontend-app-learning that referenced this issue Jun 1, 2023
  This change makes it possible to use the latest  LMS_BASE_API
  if it was changed because of dynamic config API, which is the
  default case of tutor.

  This changes closes openedx/wg-build-test-release/issues/270

   Fixes that are simlar to this
  - gradebook openedx/frontend-app-gradebook/pull/290
  - course authoring openedx/frontend-app-authoring/pull/389
@regisb
Copy link
Contributor

regisb commented Jun 1, 2023

Here's the backport PR: openedx/frontend-app-learning#1123 Can someone please review and merge?

arbrandes pushed a commit to openedx/frontend-app-learning that referenced this issue Jun 1, 2023
  This change makes it possible to use the latest  LMS_BASE_API
  if it was changed because of dynamic config API, which is the
  default case of tutor.

  This changes closes openedx/wg-build-test-release/issues/270

   Fixes that are simlar to this
  - gradebook openedx/frontend-app-gradebook/pull/290
  - course authoring openedx/frontend-app-authoring/pull/389
ghassanmas added a commit to ghassanmas/frontend-app-learning that referenced this issue Jun 6, 2023
  This change makes it possible to use the latest  LMS_BASE_API
  if it was changed because of dynamic config API, which is the
  default case of tutor.

  This changes closes openedx/wg-build-test-release/issues/270

   Fixes that are simlar to this
  - gradebook openedx/frontend-app-gradebook/pull/290
  - course authoring openedx/frontend-app-authoring/pull/389
arbrandes pushed a commit to openedx/frontend-app-learning that referenced this issue Jun 13, 2023
  This change makes it possible to use the latest  LMS_BASE_API
  if it was changed because of dynamic config API, which is the
  default case of tutor.

  This changes closes openedx/wg-build-test-release/issues/270

   Fixes that are simlar to this
  - gradebook openedx/frontend-app-gradebook/pull/290
  - course authoring openedx/frontend-app-authoring/pull/389
moonesque pushed a commit to edSPIRIT/frontend-app-learning that referenced this issue Nov 11, 2023
  This change makes it possible to use the latest  LMS_BASE_API
  if it was changed because of dynamic config API, which is the
  default case of tutor.

  This changes closes openedx/wg-build-test-release/issues/270

   Fixes that are simlar to this
  - gradebook openedx/frontend-app-gradebook/pull/290
  - course authoring openedx/frontend-app-authoring/pull/389
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Closed
Development

No branches or pull requests

8 participants