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

feat: fetching of a secured Algolia key [BB-8083] #887

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0x29a
Copy link

@0x29a 0x29a commented Dec 6, 2023

Description

Adds a fallback mechanism, so when the ALGOLIA_SEARCH_API_KEY environment variable is not set, the key is fetched from the endpoint defined by the ALGOLIA_SECURED_KEY_ENDPOINT environment variable. It expects this endpoint to return a JSON like:

{
  "key": "<ALGOLIA_SEARCH_API_KEY>"
}

This is intended, but not limited to, to be used with openedx/edx-enterprise#1962.

Testing steps

We're going to test this and openedx/edx-enterprise#1962 PRs at once by creating three courses and three enterprise customers, uploading them to Algolia through enterprise-catalog and checking that learners are able to browse catalogs of their enterprises with the help of this MFE, but unable to access courses of enterprises they don't belong to.

Prerequisites

You'll need an Algolia account. Create an application and enterprise-catalog index. Write down the application id, admin API key, and search API key somewhere.

Installing master devstack

mkdir master && cd master
export DEVSTACK_WORKSPACE=$(pwd)
git clone git@github.com:openedx/devstack.git && cd devstack
virtualenv -p python3 .fenv
source .fenv/bin/activate
make requirements
make dev.clone
make dev.provision
make dev.up.large-and-slow

After that:

  1. cd ../edx-platform
  2. Add ENTERPRISE_ALGOLIA_SEARCH_API_KEY = '<YOUR_ALGOLIA_SEARCH_(NOT ADMIN)_API_KEY>' to the end of lms/envs/devstack.py.

Installing edx-enterprise fork

sudo chown $USER:$USER ../src
cd ../src
git clone -b 0x29a/bb8083/per-user-algolia-key git@github.com:open-craft/edx-enterprise.git
cd ../devstack
make lms-shell
cd /edx/src/edx-enterprise/
pip uninstall edx-enterprise
pip install -e .
exit
make lms-shell
python manage.py lms migrate
exit
make cms-shell
cd /edx/src/edx-enterprise/
pip uninstall edx-enterprise
pip install -e .
exit
make lms-restart
make cms-restart

Installing enterprise-catalog

cd ..
git clone git@github.com:openedx/enterprise-catalog.git && cd enterprise-catalog

After that:

  1. Open enterprise_catalog/apps/catalog/constants.py and add 'source' to the CONTENT_PRODUCT_SOURCE_ALLOW_LIST set.
  2. Add the following to the end of enterprise_catalog/settings/devstack.py:
    ALGOLIA = {
        'INDEX_NAME': 'enterprise-catalog',
        'APPLICATION_ID': '<YOUR_ALGOLIA_APPLICATION_ID>',
        'API_KEY': '<YOUR_ADMIN_ALGOLIA_API_KEY>',
    }
  3. Run make dev.provision.

Installing frontend-app-learner-portal-enterprise

cd ..
git clone -b 0x29a/bb8083/per-user-algolia-key git@github.com:open-craft/frontend-app-learner-portal-enterprise.git && cd frontend-app-learner-portal-enterprise
export ALGOLIA_APP_ID=<YOUR_ALGOLIA_APPLICATION_ID>
export ALGOLIA_INDEX_NAME=enterprise-catalog
nvm use
npm install
npm start

Leave this terminal open, do all the remaining steps in a separate one.

Creating test entities

  1. Create three courses here: http://localhost:18010/home/. Use ABC, XYZ, and PSY for all fields, like here:
    image
  2. Add a section with a dropdown unit to each course, publish each.
  3. Set Course Start Date and Enrollment Start Date for each course here to 01/01/2019.
  4. Add all three courses here, like this:
    image
  5. Create a source with a Source name here: http://localhost:18381/admin/course_metadata/source/add/
  6. Go here and create three queries with the following content filters for each course:
    {
        "content_type":[
            "course",
            "courserun"
        ],
        "aggregation_key":[
            "course:ABC+ABC",
            "courserun:ABC+ABC"
        ],
        "partner":"edx",
        "availability":[
            "Current",
            "Starting Soon",
            "Upcoming"
        ],
        "status":"published"
    }
    Change ABC to XYZ and PSY for each query.
  7. Go here and create three enterprise customers: XXX, YYY, ZZZ. Change their slugs respectively.
  8. Go here and create three catalogs for each customer selecting different course queries we created earlier. XXX should be assigned to the query selecting ABC course, YYY -> XYZ and ZZZ -> PSY.

Indexing

Go to the <DEVSTACK_WORKSPACE> and replace in course_discovery/settings/base.py this:

DEFAULT_PRODUCT_SOURCE_NAME = 'edX'
DEFAULT_PRODUCT_SOURCE_SLUG = 'edx'

with this:

DEFAULT_PRODUCT_SOURCE_NAME = 'Source'
DEFAULT_PRODUCT_SOURCE_SLUG = 'source'

Then, go to the <DEVSTACK_WORKSPACE>/devstack directory run the following to pull the data from LMS to Course Discovery:

make discovery-shell
./manage.py refresh_course_metadata

Then:

  1. Go here and set a name for each organization.
  2. Go here and change every course run to be "Published"
  3. Run the following in the discovery shell to fill in the product source for each course and update the ES index:
./manage.py populate_default_product_source
./manage.py update_index --disable-change-limit
exit

Now we need to copy course queries and other enterprise-related data from LMS to enterprise-catalog. Run the following:

make lms-shell
./manage.py lms migrate_enterprise_catalogs --api_user enterprise_catalog_worker

After that you should see three of your course queries here.

Now, go to the <DEVSTACK_WORKSPACE>/enterprise-catalog and run the following to fetch content metadata (courses and course runs) from Course Discovery:

make app-shell
./manage.py update_content_metadata --force

After that you should see Course and Course Run objects for each of your courses here. If you don't, then something went wrong. Also, Json metadata for each course should contain a non-empty advertised_course_run_uuid.

Now you can upload all courses to Algolia:

./manage.py reindex_algolia

If everything go fine, you should see three courses in your enterprise-catalog Algolia index.

Enrolling users

  1. Register a new user: abc@example.com.
  2. Go the XXX enterprise customer and click Manager Learners (example of the admin panel URL).
  3. Now, enroll abc@example.com user to the course-v1:ABC+ABC+ABC course. Specify Audit track and test reason for manual enrollment.
  4. Repeat points 2 and 3, but for YYY enterprise customer and course-v1:XYZ+XYZ+XYZ course, so we have a single learner present in two different enterprises.

Testing frontend-app-learner-portal-enterprise

  1. Sign out as abc@example.com.
  2. Go to http://localhost:8734.
  3. You should be redirected to the LMS.
  4. Upon signing in again as abc@example.com, you should be offered to choose an organization. Choose XXX.
  5. You should be redirected to the MFE. You shoud see the ABC course here.
  6. Now, if you visit this URL again and select YYY, you should see the XYZ course.

Now, open the browser dev tools and locate a query like this:

https://ql3i9veweo-dsn.algolia.net/1/indexes/*/queries?x-algolia-agent=Algolia for JavaScript (4.6.0); Browser; JS Helper (3.12.0); react (17.0.2); react-instantsearch (6.38.1)
  1. Click the right mouse button on it and Edit and Resend (that's for Firefox).
  2. Paste the following JSON to the Body field and click Send:
    {
      "requests": [
        {
          "indexName": "enterprise-catalog",
          "params": "facetingAfterDistinct=true&facets=%5B%5D&highlightPostTag=%3C%2Fais-highlight-0000000000%3E&highlightPreTag=%3Cais-highlight-0000000000%3E&tagFilters="
        }
      ]
    }
    Normally, this request would fetch all courses from the index. But the MFE is using our secured Algolia key now.
  3. Observe what this query returned. There should be only 2 hits, for the ABC and XYZ courses, omitting the PSY course.

To test the default MFE behavior, try setting the ALGOLIA_SEARCH_API_KEY environment variable, re-starting the npm server and testing that the MFE is pulling all courses from the index.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 6, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Dec 6, 2023

Thanks for the pull request, @0x29a! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

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

  • supporting documentation
  • Open edX discussion forum 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 be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Dec 6, 2023
@0x29a 0x29a changed the title feat: fetching of a secured Algolia key [WIP] feat: fetching of a secured Algolia key [BB-8083] Dec 7, 2023
Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

👍

@@ -22,17 +24,41 @@ export const useRenderContactHelpText = (enterpriseConfig) => {
return renderContactHelpText;
};

let cachedApiKey = null;

export const useAlgoliaSearchApiKey = (config) => {
Copy link
Member

Choose a reason for hiding this comment

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

[inform] We've been starting to use @tanstack/react-query's useQuery hook to make API GET requests moving forward, as it removes a fair amount of boilerplate (e.g., loading states, useEffect, etc.), the need for custom client-side caching support, etc.

Copy link
Author

@0x29a 0x29a Dec 15, 2023

Choose a reason for hiding this comment

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

Thank you for letting me know, @adamstankiewicz. Would you like me to re-write this using useQuery, or it's fine as is?

@e0d
Copy link

e0d commented Dec 14, 2023

@0x29a I've run the tests, but looks like there are some conflicts that need attention.

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Dec 14, 2023
@0x29a 0x29a force-pushed the 0x29a/bb8083/per-user-algolia-key branch 3 times, most recently from 6c9c804 to 0510cef Compare December 15, 2023 12:27
@0x29a 0x29a force-pushed the 0x29a/bb8083/per-user-algolia-key branch from 0510cef to 52c8526 Compare December 15, 2023 12:28
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

Attention: 147 lines in your changes are missing coverage. Please review.

Comparison is base (d36b78a) 84.87% compared to head (52c8526) 84.97%.
Report is 109 commits behind head on master.

Files Patch % Lines
src/components/search/Search.jsx 0.00% 28 Missing ⚠️
...ntent/course-enrollments/CourseAssignmentAlert.jsx 27.27% 8 Missing ⚠️
src/components/course/data/hooks.jsx 88.70% 7 Missing ⚠️
...course-enrollments/course-cards/BaseCourseCard.jsx 89.85% 7 Missing ⚠️
src/components/my-career/VisualizeCareer.jsx 74.07% 6 Missing and 1 partial ⚠️
src/components/my-career/data/service.js 56.25% 7 Missing ⚠️
src/components/course/data/utils.jsx 95.89% 6 Missing ⚠️
src/components/my-career/AddJobRole.jsx 60.00% 5 Missing and 1 partial ⚠️
src/components/my-career/data/hooks.jsx 50.00% 6 Missing ⚠️
...rc/components/search/AssignmentsOnlyEmptyState.jsx 0.00% 6 Missing ⚠️
... and 29 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #887      +/-   ##
==========================================
+ Coverage   84.87%   84.97%   +0.10%     
==========================================
  Files         320      338      +18     
  Lines        6399     7102     +703     
  Branches     1552     1739     +187     
==========================================
+ Hits         5431     6035     +604     
- Misses        941     1035      +94     
- Partials       27       32       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@0x29a
Copy link
Author

0x29a commented Dec 15, 2023

@e0d, thanks. I rebased the branch and fixed all linting & tests issues.

@mphilbrick211
Copy link

Hi @0x29a! Flagging that some branch conflicts have popped up for when you have a minute. Thanks!

@0x29a
Copy link
Author

0x29a commented Jan 18, 2024

Hi @mphilbrick211, unless the base branch gets some major changes that make changes in dependent PRs fundamentally wrong, I'd prefer to fix conflicts after PRs are reviewed and approved or refused. Here is why:

  • Rebasing doesn't take much time (around 10-15 minutes usually) but given how active is frontend-app-learner-portal-enterprise and edx-enterprise development, it quickly adds up.
  • For the same reason conflicts often pop up even before a maintainer has a chance to take another look, making approval impossible.
  • Finally, PRs with conflicts can't be merged, and only maintainers can merge PRs, so they always can do final checks before clicking "Merge" and request changes if something is off. But conflicts alone, in my opinion, shouldn't block approval / refusal if the code is fine / needs rework.

Does this make sense? @e0d, maybe you have suggestions on what we can improve from our side to streamline the review process?

@mphilbrick211
Copy link

Hi @openedx/2u-titans! Is someone able to review / merge this for us?

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
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

None yet

6 participants