update readme for AdditionalTranslationsComponentSlot

[jacobo-dominguez-wgu]

Description

Updating docs for AdditionalTranslationsComponentSlot adding description, example code and example image.

• AdditionalTranslationsComponentSlot readme

[openedx-webhooks]

Thanks for the pull request, @jacobo-dominguez-wgu!

This repository is currently maintained by @bradenmacdonald.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.41%. Comparing base (8e3ea89) to head (6e89add).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2321   +/-   ##
=======================================
  Coverage   94.40%   94.41%           
=======================================
  Files        1167     1167           
  Lines       24901    24928   +27     
  Branches     5408     5433   +25     
=======================================
+ Hits        23508    23535   +27     
  Misses       1320     1320           
  Partials       73       73           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bradenmacdonald]

Thanks for taking the time to document these! Your docs are nice, but I find these plugin slots to be pretty messy and I have a few questions.

[bradenmacdonald]

Suggested change

	**AdditionalCourseContentPluginSlot** is dependent of [AdditionalCoursePluginSlot](../AdditionalCoursePluginSlot/README.md), to make it visible is also needed to provide settings for **AdditionalCoursePluginSlot**.
	**AdditionalCourseContentPluginSlot** is depends on [AdditionalCoursePluginSlot](../AdditionalCoursePluginSlot/README.md); to make it visible, you must also provide configuration for the **AdditionalCoursePluginSlot**.

This is weird to me. Why does one of these slots depend on the other? There isn't a technical reason as far as I can see, because if I do this:

    'org.openedx.frontend.authoring.additional_course_content_plugin.v1': {
      plugins: [
        {
          op: PLUGIN_OPERATIONS.Insert,
          widget: {
            id: 'custom_additional_course_content_id',
            type: DIRECT_PLUGIN,
            RenderWidget: () => (
              <Card className={'shadow justify-content-between'} >
                    <Card.Header
                        title={'Additional Course Content'}
                    />
                    <Card.Body>
                        <Card.Section>
                          <p>Additional course contentfrom slot plugins. Add all the content needed here.</p>
                        </Card.Section>
                    </Card.Body>
                </Card>

            ),
          },
        },
      ],
    },
    'org.openedx.frontend.authoring.additional_course_plugin.v1': {
      plugins: []
    }

it works fine. Even though this is the same as not having an additional_course_plugin at all.

[bradenmacdonald]

[bradenmacdonald]

Checking for null seems strange because it means that if you don't define 'org.openedx.frontend.authoring.additional_course_plugin.v1' at all, this will be false, but if you define it as being empty, this will be true:

const config = {
  pluginSlots: {
    'org.openedx.frontend.authoring.additional_course_content_plugin.v1': {
      plugins: [],
    },
    'org.openedx.frontend.authoring.additional_course_plugin.v1': {
      plugins: []
    }
  },
}

[bradenmacdonald]

Out of scope for this PR, but this is a bad name. Shouldn't it be something like pages_resources_content_permissions_additional_cards ?

[brian-smith-tcril]

The alias can't change for backwards compatibility reasons, but I'm very open to changing the id and making the current id (org.openedx.frontend.authoring.additional_course_content_plugin.v1) another alias.

[bradenmacdonald]

Does anyone know why we even have "Content permissions" as a separate section? It doesn't seem that useful to me to have one or two cards separate from the others. Why are the permissions settings not just managed within the "regular" settings for the particular app?

[bradenmacdonald]

@brian-smith-tcril do you have any additional context on these "Pages & Resources" plugin slots?

[brian-smith-tcril]

I did some digging through the blame and found:

---

The slot was originally added in

• chore: add slot to allow additional course app plugin #941

at that point it was just

<PluginSlot id="additional_course_plugin" />

hardcoded into the PageGrid component.

---

It was then updated in

• fix: allow page grid to take plugin slot id instead #994

which changed the PageGrid component from using a hardcoded slot id to one passed in as a prop

In PagesAndResources:

<PageGrid pages={pages} pluginSlotId="additional_course_plugin" />
[...]
<PageGrid pages={contentPermissionsPages} pluginSlotId="additional_course_content_plugin" />

In PageGrid:

- <PluginSlot id="additional_course_plugin" />
+ {pluginSlotId && <PluginSlot id={pluginSlotId} />}

---

I then made the "standardize slot ids" PR

• feat: standardize slot ids #1854
  • Jansen noticed an issue with this and fixed it in fix: pages and resources plugins not rendered #1885

With that I moved PageGrid away from taking in a slot id as a prop to instead take in a slot component as a prop.

In PageGrid:

- const PageGrid = ({ pages, pluginSlotId, courseId }) => (
+ const PageGrid = ({ pages, pluginSlotComponent, courseId }) => (

- {pluginSlotId && <PluginSlot id={pluginSlotId} />}
+ {pluginSlotComponent}

---

I think the image showing the bug in #994 helps provide some context

In that screenshot we can see the 2 PageGrid components rendered by the PagesAndResources component:

The top grid is from

frontend-app-authoring/src/pages-and-resources/PagesAndResources.jsx

Line 95 in 11de402

<PageGrid pages={pages} pluginSlotComponent={<AdditionalCoursePluginSlot />} courseId={courseId} />

The bottom grid is from

frontend-app-authoring/src/pages-and-resources/PagesAndResources.jsx

Line 103 in 11de402

<PageGrid pages={contentPermissionsPages} pluginSlotComponent={<AdditionalCourseContentPluginSlot />} />

---

I know this is a lot. Hopefully that helps a bit! Let me know if you have more questions!

[brian-smith-tcril]

As for the dependency issue, it seems to be stemming from

frontend-app-authoring/src/pages-and-resources/PagesAndResources.jsx

Line 71 in 11de402

const hasAdditionalCoursePlugin = getConfig()?.pluginSlots?.additional_course_plugin != null;

and

frontend-app-authoring/src/pages-and-resources/PagesAndResources.jsx

Lines 96 to 106 in 11de402

	{
	(contentPermissionsPages.length > 0 || hasAdditionalCoursePlugin)
	&& (
	<>
	<div className="d-flex justify-content-between my-4 my-md-5 align-items-center">
	<h3 className="m-0">{intl.formatMessage(messages.contentPermissions)}</h3>
	</div>
	<PageGrid pages={contentPermissionsPages} pluginSlotComponent={<AdditionalCourseContentPluginSlot />} />
	</>
	)
	}

specifically

frontend-app-authoring/src/pages-and-resources/PagesAndResources.jsx

Line 97 in 11de402

(contentPermissionsPages.length > 0 || hasAdditionalCoursePlugin)

---

It seems to me this started as a mistake in #994 - it seems it should have been

const hasAdditionalCoursePlugin = getConfig()?.pluginSlots?.additional_course_content_plugin != null;

---

Unfortunately, this logic seems to be even worse now than when it started now that we've standardized slot ids and added alias functionality. I'd want to double check this but I'm pretty sure with the current code hasAdditionalCoursePlugin would be falsy if someone used the proper org.openedx.frontend.authoring.additional_course_plugin.v1 id instead of the additional_course_plugin alias.

I absolutely think cleaning up the logic surrounding the conditional rendering of the "Content permissions" grid makes sense. I'd also like to know if anyone has an answer to @bradenmacdonald's previous question:

Does anyone know why we even have "Content permissions" as a separate section? It doesn't seem that useful to me to have one or two cards separate from the others. Why are the permissions settings not just managed within the "regular" settings for the particular app?

[bradenmacdonald]

@jristau1984 I think the "Content Permissions" thing may have originated from a 2U plugin - do you know the answer to that question ^ ? (not urgent)

[jristau1984]

It looks like the reason it's just to call out that the plugin will be accessing course content in some way. There are disclaimers in our widget and precedent that we don't need another section like this now... I think it can be deprecated from 2U's Palestine.

[brian-smith-tcril]

Thanks for that context @jristau1984!

So it sounds like we can simplify this quite a bit!

If we remove this block:

frontend-app-authoring/src/pages-and-resources/PagesAndResources.jsx

Lines 48 to 58 in 11de402

	// We want the Xpert learning assistant and unit summaries to appear in the "Content Permissions" section instead,
	// so we remove them from pages and add them to contentPermissionsPages.
	const contentPermissionsPages = [];
	['xpert_unit_summary', 'learning_assistant'].forEach(separateAppId => {
	const index = pages.findIndex(app => app.id === separateAppId);
	if (index !== -1) {
	const [page] = pages.splice(index, 1);
	contentPermissionsPages.push(page);
	}
	});

and this line

frontend-app-authoring/src/pages-and-resources/PagesAndResources.jsx

Line 71 in 11de402

const hasAdditionalCoursePlugin = getConfig()?.pluginSlots?.additional_course_plugin != null;

and this block

frontend-app-authoring/src/pages-and-resources/PagesAndResources.jsx

Lines 96 to 106 in 11de402

	{
	(contentPermissionsPages.length > 0 || hasAdditionalCoursePlugin)
	&& (
	<>
	<div className="d-flex justify-content-between my-4 my-md-5 align-items-center">
	<h3 className="m-0">{intl.formatMessage(messages.contentPermissions)}</h3>
	</div>
	<PageGrid pages={contentPermissionsPages} pluginSlotComponent={<AdditionalCourseContentPluginSlot />} />
	</>
	)
	}

then everything should be in the one PageGrid.

We then need to handle the 2 slot id issue. By removing those and making no other changes we'd only be supporting:

• Site operators using only org.openedx.frontend.authoring.additional_course_plugin.v1/additional_course_plugin

And we need to continue to also support:

• Site operators using only org.openedx.frontend.authoring.additional_course_content_plugin.v1/additional_course_content_plugin
• Site operators using both org.openedx.frontend.authoring.additional_course_plugin.v1/additional_course_plugin and org.openedx.frontend.authoring.additional_course_content_plugin.v1/additional_course_content_plugin

I think the best way to handle this would be to update the AdditionalCoursePluginSlot component

frontend-app-authoring/src/plugin-slots/AdditionalCoursePluginSlot/index.tsx

Lines 3 to 8 in 11de402

	export const AdditionalCoursePluginSlot = () => (
	<PluginSlot
	id="org.openedx.frontend.authoring.additional_course_plugin.v1"
	idAliases={['additional_course_plugin']}
	/>
	);

to include the plugin slot from the AdditionalCourseContentPluginSlot component

frontend-app-authoring/src/plugin-slots/AdditionalCourseContentPluginSlot/index.tsx

Lines 4 to 7 in 11de402

	<PluginSlot
	id="org.openedx.frontend.authoring.additional_course_content_plugin.v1"
	idAliases={['additional_course_content_plugin']}
	/>

so we'd have

export const AdditionalCoursePluginSlot = () => (
  <PluginSlot
    id="org.openedx.frontend.authoring.additional_course_plugin.v1"
    idAliases={['additional_course_plugin']}
  />
  <PluginSlot
    id="org.openedx.frontend.authoring.additional_course_content_plugin.v1"
    idAliases={['additional_course_content_plugin']}
  />
);

at that point we can completely remove the AdditionalCourseContentPluginSlot component.

We can then put out a DEPR for the org.openedx.frontend.authoring.additional_course_content_plugin.v1/additional_course_content_plugin id/alias and let site operators know they should move anything they're currently putting in that slot to the org.openedx.frontend.authoring.additional_course_plugin.v1 slot instead.

[jacobo-dominguez-wgu]

Alright then. Do you guys @bradenmacdonald, @brian-smith-tcril agree to use this current pr just for the AdditionalTranslationsComponentSlot docs and create a new issue to handle the AdditionalCourseContentPluginSlot deprecation?

[brian-smith-tcril]

This looks great as it stands now!

Thanks for making the #2330 issue!

If you could update the PR description of this to be something like "docs: update AdditionalTranslationsComponentSlot README" to clarify the new scope of this PR that'd be perfect!