search

mitodl / ocw-studio

Open Source Courseware authoring tool
BSD 3-Clause "New" or "Revised" License
7 stars 3 forks source link

Remove the internal external radio buttons #2188

Closed umar8hassan closed 1 month ago

umar8hassan commented 1 month ago

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/4101

Description (What does it do?)

It removes the Item type radio button in Menu Item Form.

Screenshot 2024-05-27 at 5 52 27 AM

How can this be tested?

  1. Login to cow-studio

  2. Add a new site course or use and existing one

  3. In the course setting, visit page for Menu

    Screenshot 2024-05-27 at 5 55 10 AM

  4. Add a new menu item or open an existing one

  5. The item form should not have radio button as shown in the above picture in description

arslanashraf7 commented 1 month ago

@ibrahimjaved12 Could you review this?

umar8hassan commented 1 month ago

The changes are working as expected. And I see that you also updated tests, and removed a lot outdated code.

However, I think we may be able to use some more cleanup.

When I search external within the codebase, there are some places where external links is still mentioned (I don't mean within context of external resources).

We should use this opportunity to update it also. Also it appears a few management commands are currently also using external URL.

For example, these are a few files: ocw-studio/websites/management/commands/markdown_cleaning/rules/validate_urls.py ocw-studio/websites/management/commands/markdown_cleaning/rules/broken_link_fix.py ocw-studio/websites/constants.py- EXTERNAL_IDENTIFIER_PREFIX = "external-" - is this used anywhere?

Also there is isExternalLinkId which seems to be used a lot in the codebase.

We should remove external link related stuff wherever possible, make sure it does not break the management commands. And update internal link related variables' names for their better clarity now.

Just search through external and internal in the codebase for cleanup. More thorough cleanup would depend on priority, but we should do at least what we can here.

@ibrahimjaved12 I have checked through a lot of code. external mostly shows up in management commands like nav_item_to_external_resource which is used to convert an obsolete type of external link to new type of external resource. This should be kept as we might need it in case some external links needs to be converted in production

There shouldn't be any breaking change as far as I can see. However, removing the suggested commands or constants will take a lot of effort alone with updating/removing the relevant tests. I shall track this in a separate issue and prioritize if need be.

umar8hassan commented 1 month ago

@pdpinch what's your take on this? ⬆️

pdpinch commented 1 month ago

@pdpinch what's your take on this? ⬆️

Sorry I haven't had a chance to comment until now. I appreciate @ibrahimjaved12's zeal for removing code, but I also don't want to hold up this change too long. We should remove the UI as soon as reasonably possible in order to avoid course authors being tempted to use it and creating more data we need to migrate.