feat: Make courseware URLs more human-readable #28174

KristinAoki · 2021-07-13T17:13:33Z

Description

Describe what this pull request changes, and why. Include implications for people using this change.
Design decisions and their rationales should be documented in the repo (docstring / ADR), per
OEP-19, and can be
linked here.

This change impacts the Learner.

Before:

After:

Supporting information

Jira issue: TNL-8511

Testing instructions

Navigate to the home page of a course.
Click on the "Start Course"/"Resume Course" button.
The page will fully load.
The URL in the address bar should follow this pattern:
- host/c/:courseId/:sequenceHash/:unitHash
  Ex: http://localhost:2000/c/course-v1:edx+kris1+2021_T1/YmE3OGQwNzMyYWE2/ZmY0MzRjMTViYjBm
Navigate back to the home page.
Click on a specific section from the outline.
The page will fully load.
The URL should match the same formatting as above.
In the unit view, click on the bookmark button.
The bookmark saves.
Navigate back to the home page.
Click on the bookmark page.
All bookmarks are seen, even the one just added.
Navigate back to the previous unit.
Click on the bookmark button.
The bookmark is removed.
Go back to the bookmark page.
The removed bookmark is no longer listed.

Deadline

August 11, 2021: Must be completed before the end of the internship

Other information

This change is dependent on Frontend-App-Learning PR-540

…to KristinAoki/TNL-8511

kdmccormick · 2021-08-06T20:19:20Z

@KristinAoki I played around with this change on my devstack, and I'm psyched to see the short URLs working! 🔥 Your progress here is great, but I want to adjust your direction a bit so that it'll be easier for us to merge and enable this confidently.

I notice that in several places, your PR swaps out unit/sequences usage keys with their hashes, in-place. For instance, it modifies the path_to_location function to return hashes for units and sequences when ENABLE_SHORT_MFE_URL is toggled on, but return normal old usage keys otherwise. Since your PR modifies Python APIs and REST APIs that are used in several different places and which are built to expect usage keys, I fear that enabling this feature could break other existing features.

To get a feel for the impact of your current changes, you might temporarily ENABLE_SHORT_MFE_URL to True and see what tests fail. My guess it that it might break lot of code--many of those failures would be false-positives, but I believe at least some of them would represent code that actually fails because a hash is returned instead of a usage key.

Given that, my advice is actually to build the backend of this feature so that it doesn't actually check the value of feature flag, other than to pass the value of the flag to the frontend. In other words, see if you can make the flag frontend-only (although it's fine to keep it defined in common.py, just in case I'm wrong and you do need to check the value on the backend).

Why? Because I believe that you can pass the new data you need to the frontend (that is: (1) block's hash and (2) whether the feature is enabled) by only adding fields to existing APIs responses, instead of modifying fields that are already in use. So, if an API has a block_id field in its response or expects a block_id in the URL, don't change either of those aspects; just add a new hash field. If you're able to do this, then you shouldn't need to look at the feature flag on the backend, because the new hash data won't be disruptive.

The upsides of this approach would be:

You know that the unit tests are in fact testing the full surface area of your backend changes, since the new code paths aren't behind a feature flag.
You would be able to merge this without first merging the frontend changes (remember - your PRs cannot be released at the same time. The way we release code, one will always go out before the other, and you can't count on the time difference between release to be small)
Toggling the feature flag on will be less risky, since we know it's only really impacting the frontend.
Your change should touch fewer parts of edx-platform and will likely have a smaller linecount. These things will make it easier for your reviewers to understand your changes and feel confident that they're correct.

I hope that makes sense! Let me know if it doesn't, or if you think I'm missing a critical piece here.

kdmccormick · 2021-08-12T20:03:26Z

cms/envs/common.py

+############# Shorten MFE URL ##########################
+# .. toggle_name: enable_short_mfe_url
+# .. toggle_implementation: DjangoSetting
+# .. toggle_default: False
+# .. toggle_description: Flag would be used to hash block_ids and shorten the MFE url
+# .. toggle_use_cases: temporary
+# .. toggle_tickets: https://github.com/edx/edx-platform/pull/28174/
+# .. toggle_creation_date: 2021-08-03
+# .. toggle_target_removal_date: 2022-01-01
+ENABLE_SHORT_MFE_URL = False
+


do we still need the CMS setting, or can we drop this?

I am not sure, when I didn't have it before some of the cms tests failed, but that was also before I refactored the code.

common/lib/xmodule/xmodule/modulestore/search.py

kdmccormick · 2021-08-12T20:15:48Z

lms/djangoapps/course_api/blocks/views.py

+
+            if 'blocks' in response.data:
+                blocks = response.data['blocks']
+            else:
+                blocks = response.data
+
+            for block in blocks:
+                current_block = block
+                if isinstance(blocks, dict):
+                    current_block = blocks[block]
+                if 'children' in current_block:
+                    new_child_ids = [get_usage_key_hash(child) for child in current_block['children']]
+                    current_block['hash_children'] = new_child_ids
+                current_block['hash_key'] = get_usage_key_hash(current_block['id'])
+


does the frontend code still need the course_blocks API response to be modified to contain the hash_key, or are the changes to the sequence metadata API sufficient?

It needs it for when it is calls normalizeBlocks in api.js

kdmccormick · 2021-08-12T20:24:16Z

lms/djangoapps/course_home_api/outline/v1/serializers.py

@@ -3,6 +3,7 @@
 """

 from django.utils.translation import ngettext
+from django.conf import settings


Am I correct that you're adding the hash_key to the URLs here so that the course outline can use them in their links?

If so, you might want to leave those links unchanged for now, and leave the course_home_api as-is. The old links should be fine to stay on the outline page for now, as they'll redirect to the new ones.

I think that is why I am adding them. Not 100%.

kdmccormick · 2021-08-12T20:26:30Z

openedx/core/djangoapps/courseware_api/views.py

@@ -522,8 +536,18 @@ def get(self, request, usage_key_string, *args, **kwargs):  # lint-amnesty, pyli
        """
        Return response to a GET request.
        """
+        if settings.ENABLE_SHORT_MFE_URL:


You should be able to revert the changes to this file and rebase upon my PR

kdmccormick · 2021-08-12T20:27:16Z

openedx/core/djangoapps/content/learning_sequences/data.py

@@ -125,6 +125,15 @@ class CourseLearningSequenceData:
    learning sequences in Courses vs. Pathways vs. Libraries. Such an object
    would likely not have `visibility` as that holds course-specific concepts.
    """
+


The changes to this file can be replaced with the model persistence in my PR

edx-status-bot · 2021-08-12T21:42:13Z

Your PR has finished running tests. There were no failures.

feanil · 2024-06-05T12:53:12Z

Closing since it looks like this stalled 3 years ago. Please re-open if it's still relevant.

Kristin Aoki and others added 20 commits July 13, 2021 13:10

Update current

a9f61b5

merge master

dfbcd79

Update path to location to hash the location of blocks

6239b68

Replace encoding with function to hash block id

3843786

Add function to store mapping of hash keys and block ids

4870e08

Change the decoding of hash to use mapping function

195d212

Add experience parameter to identify which url is being built

a355dac

Merge branch 'master' into KristinAoki/TNL-8511

cde19a1

Merge branch 'master' into KristinAoki/TNL-8511

d2312fd

Add loop to add hash_key to units via sequence metadata

f46df34

Update hash to return as a string instead of bytes string

1aba481

Merge branch 'master' into KristinAoki/TNL-8511

470b87c

Add check for hashed usage keys and decoded them

0f2f1c8

Set hash key as item id and store original id as decoded_id

8e15f6b

Merge branch 'KristinAoki/TNL-8511' of github.com:edx/edx-platform in…

081d79c

…to KristinAoki/TNL-8511

Fix PEP 8 errors

5bafb69

Add conditional block type check to add proper childre references

41122b9

Change available_seq_ids from usage keys to hash keys

958b994

Add hash_key to all blocks in course outline

c4d61d9

Update sequences list to be stored by hash key instead of usage key

8f88489

KristinAoki changed the title ~~Update current~~ feat: Make courseware URLs more human-readable Jul 26, 2021

KristinAoki mentioned this pull request Jul 26, 2021

feat: Make courseware URLs more human-readable openedx/frontend-app-learning#540

Open

Kristin Aoki added 4 commits July 28, 2021 10:45

Fix PEP 8 errors

d0f6450

Merge branch 'master' into KristinAoki/TNL-8511

8f07f17

Add hash_children parameter to block

8fe1ecd

Change sequence ids to be stored by hash key

5602d4e

KristinAoki marked this pull request as draft July 30, 2021 17:57

Kristin Aoki added 3 commits August 2, 2021 12:58

Fix undefined parameter error

290b61c

Update order of parameters to match parameters in function

2e82262

Removed unused import

d786d1f

KristinAoki and others added 9 commits August 4, 2021 14:52

Merge branch 'master' into KristinAoki/TNL-8511

2063d85

Add property to pass mfe short url to frontend

3bc7172

Fix Jenkins quality errors

ae88a4e

Fix Jenkins quality errors

32b3813

Remove all LMS specific code

e63a7a8

Update get_usage_key_hash function import

8a78776

Add get_usage_key_hash function

98d859f

Remove extra hash functions

093eef0

Add feature flag for short mfe urls

5ed3aae

Kristin Aoki added 11 commits August 9, 2021 09:44

Add condition to check if response is a dict or list

2851110

Update test to match new URL format

27968a0

Update test to match new URL format

64d14af

Remove print statements

dea27ab

Fix missing hash_key in block

c730754

Remove flag conditional checks

6f90485

Add parameter to pass flag to frontnd

41888c7

Fix Jenkins quality errors

5c65970

Fix Jenkins quality error

27d9c89

Fix broken indexing

632c3d3

Update mfe urls

ad18bce

kdmccormick mentioned this pull request Aug 11, 2021

feat: implement usage_key_hashes and expose via sequence metadata api #28436

Closed

Merge branch 'master' into KristinAoki/TNL-8511

e4c022b

kdmccormick reviewed Aug 12, 2021

View reviewed changes

Kristin Aoki added 2 commits August 12, 2021 16:51

Restore file to match master

0199c9e

Remove unused code

8ebd3ed

feanil closed this Jun 5, 2024

feanil deleted the KristinAoki/TNL-8511 branch June 5, 2024 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Make courseware URLs more human-readable #28174

feat: Make courseware URLs more human-readable #28174

KristinAoki commented Jul 13, 2021 •

edited

Loading

kdmccormick commented Aug 6, 2021

kdmccormick Aug 12, 2021

KristinAoki Aug 12, 2021

kdmccormick Aug 12, 2021

KristinAoki Aug 12, 2021

kdmccormick Aug 12, 2021

KristinAoki Aug 12, 2021

kdmccormick Aug 12, 2021

kdmccormick Aug 12, 2021

edx-status-bot commented Aug 12, 2021

feanil commented Jun 5, 2024

feat: Make courseware URLs more human-readable #28174

feat: Make courseware URLs more human-readable #28174

Conversation

KristinAoki commented Jul 13, 2021 • edited Loading

Description

Supporting information

Testing instructions

Deadline

Other information

kdmccormick commented Aug 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edx-status-bot commented Aug 12, 2021

feanil commented Jun 5, 2024

KristinAoki commented Jul 13, 2021 •

edited

Loading