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

[FC-0049] feat: Other Tags section added on tags drawer #987

Merged

Conversation

ChrisChV
Copy link
Contributor

@ChrisChV ChrisChV commented May 7, 2024

Description

This adds the new "Other tags" Section to tags drawer that contains the taxonomies/tags that the user doesn't have permission to see/edit. It allow to delete that tags.

Supporting information

Testing instructions

  • Use this branch of edx-platform: [FC-0049] feat: Update remove object-tag permission edx-platform#34728
  • Run make requirements on cms-shell.
  • Run the script on https://github.com/open-craft/taxonomy-sample-data
  • Go to taxonomy list page and import an small taxonomy: example: template (3).csv
  • Ensure that the new taxonomy doesn't have associated organizations
  • Go to Studio and create a new course usign SampleTaxonomyOrg1 as organization
  • Import a course with taxonomies using the new taxonomy. You can use this example, but yu need to update the export_id of the new taxonomy: course.phoprzm8.tar.gz
  • Open the manage tags of the course or any Unit
  • Check the Other tags section
  • Click on Edit mode
  • Verify that you can't add more tags in Other tags section.
  • Delete some tags and save. Verify that the tags and count are updated.
  • Delete all Other tags and Save. Verify that the Other tags section is gone.

Other Information

TODO: It's missing to add examples to test this on https://github.com/open-craft/taxonomy-sample-data

@ChrisChV ChrisChV requested a review from a team as a code owner May 7, 2024 19:10
@openedx-webhooks
Copy link

openedx-webhooks commented May 7, 2024

Thanks for the pull request, @ChrisChV! 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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 7, 2024
@ChrisChV ChrisChV marked this pull request as draft May 7, 2024 20:19
@ChrisChV ChrisChV changed the title feat: Other Tags section added on tags drawer [FC-0049] feat: Other Tags section added on tags drawer May 7, 2024
Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 92.28%. Comparing base (55adcfe) to head (5d3e23c).

Files Patch % Lines
...rc/content-tags-drawer/ContentTagsDrawerHelper.jsx 90.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #987      +/-   ##
==========================================
- Coverage   92.28%   92.28%   -0.01%     
==========================================
  Files         707      707              
  Lines       12469    12493      +24     
  Branches     2671     2679       +8     
==========================================
+ Hits        11507    11529      +22     
- Misses        925      927       +2     
  Partials       37       37              

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

@yusuf-musleh
Copy link
Contributor

yusuf-musleh commented May 8, 2024

@ChrisChV I wasn't able to test this. Whenever I try to import a taxonomy, the /import endpoint hangs indefinitely. I made sure I checked the correct branches in all the repos (including openedx-learning). I even rebuilt everything from scratch (using tutor dev launch -I) to make sure it's not some weird state my env is in, but it leads to the same thing.

I also tried importing different kinds of taxonomies.

I'm not sure what the issue is, could you confirm that it is working correctly on your end?

Edit: Found these in the logs when I restarted the server:

Traceback (most recent call last):
  File "/openedx/venv/lib/python3.8/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
  File "/openedx/venv/lib/python3.8/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/opt/pyenv/versions/3.8.18/lib/python3.8/contextlib.py", line 75, in inner
    return func(*args, **kwds)
  File "/openedx/venv/lib/python3.8/site-packages/django/views/decorators/csrf.py", line 56, in wrapper_view
    return view_func(*args, **kwargs)
  File "/openedx/venv/lib/python3.8/site-packages/rest_framework/viewsets.py", line 125, in view
    return self.dispatch(request, *args, **kwargs)
  File "/openedx/venv/lib/python3.8/site-packages/rest_framework/views.py", line 509, in dispatch
    response = self.handle_exception(exc)
  File "/openedx/venv/lib/python3.8/site-packages/rest_framework/views.py", line 469, in handle_exception
    self.raise_uncaught_exception(exc)
  File "/openedx/venv/lib/python3.8/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
    raise exc
  File "/openedx/venv/lib/python3.8/site-packages/rest_framework/views.py", line 506, in dispatch
    response = handler(request, *args, **kwargs)
  File "/openedx/edx-platform/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py", line 98, in create_import
    response = super().create_import(request=request, **kwargs)  # type: ignore
  File "/mnt/openedx-learning/openedx_tagging/core/tagging/rest_api/v1/views.py", line 325, in create_import
    resync_object_tags()
  File "/mnt/openedx-learning/openedx_tagging/core/tagging/api.py", line 169, in resync_object_tags
    object_tag.save()
  File "/openedx/venv/lib/python3.8/site-packages/django/db/models/base.py", line 814, in save
    self.save_base(
  File "/openedx/venv/lib/python3.8/site-packages/django/db/models/base.py", line 877, in save_base
    updated = self._save_table(
  File "/openedx/venv/lib/python3.8/site-packages/django/db/models/base.py", line 990, in _save_table
    updated = self._do_update(
  File "/openedx/venv/lib/python3.8/site-packages/django/db/models/base.py", line 1054, in _do_update
    return filtered._update(values) > 0
  File "/openedx/venv/lib/python3.8/site-packages/django/db/models/query.py", line 1231, in _update
    return query.get_compiler(self.db).execute_sql(CURSOR)
  File "/openedx/venv/lib/python3.8/site-packages/django/db/models/sql/compiler.py", line 1984, in execute_sql
    cursor = super().execute_sql(result_type)
  File "/openedx/venv/lib/python3.8/site-packages/django/db/models/sql/compiler.py", line 1562, in execute_sql
    cursor.execute(sql, params)
  File "/openedx/venv/lib/python3.8/site-packages/debug_toolbar/panels/sql/tracking.py", line 235, in execute
    return self._record(super().execute, sql, params)
  File "/openedx/venv/lib/python3.8/site-packages/debug_toolbar/panels/sql/tracking.py", line 160, in _record
    return method(sql, params)
  File "/openedx/venv/lib/python3.8/site-packages/django/db/backends/utils.py", line 102, in execute
    return super().execute(sql, params)
  File "/openedx/venv/lib/python3.8/site-packages/django/db/backends/utils.py", line 67, in execute
    return self._execute_with_wrappers(
  File "/openedx/venv/lib/python3.8/site-packages/django/db/backends/utils.py", line 80, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/openedx/venv/lib/python3.8/site-packages/django/db/backends/utils.py", line 89, in _execute
    return self.cursor.execute(sql, params)
  File "/openedx/venv/lib/python3.8/site-packages/django/db/utils.py", line 91, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/openedx/venv/lib/python3.8/site-packages/django/db/backends/utils.py", line 89, in _execute
    return self.cursor.execute(sql, params)
  File "/openedx/venv/lib/python3.8/site-packages/django/db/backends/mysql/base.py", line 75, in execute
    return self.cursor.execute(query, args)
  File "/openedx/venv/lib/python3.8/site-packages/MySQLdb/cursors.py", line 179, in execute
    res = self._query(mogrified_query)
  File "/openedx/venv/lib/python3.8/site-packages/MySQLdb/cursors.py", line 330, in _query
    db.query(q)
  File "/openedx/venv/lib/python3.8/site-packages/MySQLdb/connections.py", line 261, in query
    _mysql.connection.query(self, query)
django.db.utils.OperationalError: (1205, 'Lock wait timeout exceeded; try restarting transaction')
Internal Server Error: /api/content_tagging/v1/taxonomies/import/

It seems to also be happening when I tried it on master/main for all repos, so it's probably not related to the changes in these PRs?

@ChrisChV
Copy link
Contributor Author

ChrisChV commented May 8, 2024

@yusuf-musleh It's working using devstack, but I will test this using tutor

@ChrisChV
Copy link
Contributor Author

ChrisChV commented May 8, 2024

@yusuf-musleh I have tried it and it has worked for me. It takes a little longer due to indexing in meilisearch. Maybe it's a configuration in meilisearch that's missing from your locale?

@bradenmacdonald
Copy link
Contributor

@yusuf-musleh That sounds like it could be related to openedx/modular-learning#213 . I also couldn't reproduce it though (using tutor). Let's consider it part of that issue ^ and unrelated to this PR, if you're seeing on master too.

@ChrisChV
Copy link
Contributor Author

ChrisChV commented May 8, 2024

I wasn't able to test this. Whenever I try to import a taxonomy, the /import endpoint hangs indefinitely. I made sure I checked the correct branches in all the repos (including openedx-learning). I even rebuilt everything from scratch (using tutor dev launch -I) to make sure it's not some weird state my env is in, but it leads to the same thing.

@yusuf-musleh If you are blocked for the import of the taxonomy, you can test this without import a taxonomy. You can use an already created taxonomy and delete any associated organization.

Copy link
Contributor

@bradenmacdonald bradenmacdonald left a comment

Choose a reason for hiding this comment

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

Works well. Just one minor thing.

{intl.formatMessage(messages.otherTagsHeader)}
</p>
<p className="h6 text-gray-500">
{intl.formatMessage(messages.otherTagsDescription)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This shouldn't be h6 because that's making the text bold. The text should not be bold.

Screenshot 2024-05-08 at 10 37 18 AM

vs.

Screenshot 2024-05-08 at 10 38 16 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated here: 5a45416

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Update styles on Other tags description section in tag drawer
@bradenmacdonald
Copy link
Contributor

@ChrisChV Could you please rebase this or resolve the conflicts?

bradenmacdonald added a commit to open-craft/frontend-app-course-authoring that referenced this pull request May 8, 2024
Copy link
Contributor

@yusuf-musleh yusuf-musleh left a comment

Choose a reason for hiding this comment

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

@ChrisChV @bradenmacdonald I tried it again today and it worked, not sure what changed, but its definitely not related to the changes here as you mentioned. It's working well!

👍

  • I tested this: I followed the testing instructions in the PR
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@ChrisChV
Copy link
Contributor Author

ChrisChV commented May 9, 2024

@bradenmacdonald Done!

@brian-smith-tcril @arbrandes @viktorrusakov @xitij2000 Does anyone have time to approve and merge this before Redwood gets cut? Thank you 🙂

@xitij2000 xitij2000 merged commit e0fb41d into openedx:master May 9, 2024
6 checks passed
@openedx-webhooks
Copy link

@ChrisChV 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@bradenmacdonald bradenmacdonald deleted the chris/FAL-3720-tags-without-permission branch May 9, 2024 16:25
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
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants