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

[WIP] Fixes #37752 - Update to Patternfly 5 #11120

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MariaAga
Copy link
Contributor

@MariaAga MariaAga commented Aug 27, 2024

Updating pf4 to pf5
some work is done by running npx @patternfly/pf-codemods@latest webpack --fix

search& replace "pf-c-" to "pf-v5-c-"

Check all "onChange={[^(]" to make sure that pure pf5 are using the right arguments, as they changed order, and most onChange are onChange(event,value) and not onChange(value)

And adding act wrapper around tests.
To test this pr you will need the foreman core pf5 pr, foreman js pr, link foreman-js, and to remove any foreman-js/patternfly folder in the katello node_modules (as this breaks React shared context in tests).

This is a WIP since the other prs were just done and not polished, and also because some of the tests I had troubles fixing and dont have capacity currently to fix. These tests fail with Error: Uncaught [Error: Warning: An update to %s inside a test was not wrapped in act(...) even if every line in the test is wrapped in act, or

 Error: Error: Nock: No match for request {
      "method": "GET",
      "url": "http://localhost/api/v2/hosts/1/subscriptions/available_release_versions",
      ...

@MariaAga
Copy link
Contributor Author

MariaAga commented Sep 6, 2024

Fixed more tests but not sure what to do about the last 8 failing with this type of error as in some of them I do see the request being mocked in the test

    Error: Error: Nock: No match for request {
      "method": "GET",
      "url": "http://localhost/api/hosts/test-host",
      "headers": {
        "accept": "application/json, text/plain, */*",
        "x-requested-with": "XMLHttpRequest",
        "x-csrf-token": "",
        "referer": "http://localhost/",
        "user-agent": "Mozilla/5.0 (linux) AppleWebKit/537.36 (KHTML, like Gecko) jsdom/11.12.0",
        "accept-language": "en",
        "host": "localhost",
        "accept-encoding": "gzip, deflate"
      }
    }

@MariaAga MariaAga changed the title [WIP] pf5 [WIP] Fixes #37752 - Update to Patternfly 5 Sep 6, 2024
@@ -387,6 +389,7 @@ test('Can display create wizard and create simplified ACS', async (done) => {
assertNockRequest(productScope);
assertNockRequest(contentCredentialScope);
assertNockRequest(smartProxyScope);
assertNockRequest(createScope, done);
assertNockRequest(createScope);
done();
Copy link
Member

Choose a reason for hiding this comment

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

Think we have the act(done) in the tests to stop further API calls from being recorded and for nocks to get cleaned up..I am not entirely sure we need the done() here..I can try locally..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running assertNockRequest(X, done); did not work for tests, I had to change it to:

assertNockRequest(X);
done();

In all of them

@sjha4
Copy link
Member

sjha4 commented Jan 3, 2025

This should fix the lint errors:

npx eslint --fix webpack/

This leaves only some no-unused-vars issues in 4 files..

@sjha4
Copy link
Member

sjha4 commented Jan 3, 2025

Overall I am not seeing any issues so far in the code/testing.
Minor notes I'll document in comments.

  1. Notifications seem a little off:
    PF4:
    Screenshot from 2025-01-03 15-47-21
    PF5:
    Screenshot from 2025-01-03 15-47-51

  2. Some sub-tabs are not rendering full-width:
    PF4:
    Screenshot from 2025-01-03 15-51-32
    PF5:
    Screenshot from 2025-01-03 15-51-16

@@ -11,15 +11,15 @@ const EnvironmentLabels = (environments) => {
<React.Fragment key={env.id} style={{ marginBottom: '5px' }}>
<Label
color={labelColor}
isTruncated
Copy link
Member

Choose a reason for hiding this comment

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

Did isTruncated get deprecated too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Removed the isTruncated property from Label. This is now the default behavior. In addition, you can limit the text width using the new textMaxWidth property."
https://v5-archive.patternfly.org/get-started/upgrade/release-notes

@sjha4
Copy link
Member

sjha4 commented Jan 6, 2025

For the conflicts here:

webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVReassignActivationKeysForm.js, the branch code is right.

For webpack/scenes/ContentViews/Details/Versions/Delete/RemoveSteps/CVVersionRemoveReview.js , The master branch has some logic which needs to be maintained.
This looks like the right code with only isTruncated removed from label in the master branch code.

{singleCVActivationKeysCount > 0 && (
            <Flex>
              <FlexItem><ExclamationTriangleIcon /></FlexItem>
              <FlexItem><p>{__(`${pluralize(singleCVActivationKeysCount, 'activation key')} will be moved to content view ${selectedCVNameForAK} in `)}</p></FlexItem>
              <FlexItem><Label color="purple" href={`/lifecycle_environments/${selectedEnvForAK[0].id}`}>{selectedEnvForAK[0].name}</Label></FlexItem>
            </Flex>
          )}
          {multiCVActivationKeysCount > 0 && (
            <Flex>
              <FlexItem><ExclamationTriangleIcon /></FlexItem>
              <FlexItem>
                <p>
                  {__(`Content view environment will be removed from ${pluralize(multiCVActivationKeysCount, 'multi-environment activation key')}.`)}
                </p>
              </FlexItem>
            </Flex>
          )}

@MariaAga
Copy link
Contributor Author

MariaAga commented Jan 6, 2025

Fixed the notifications in foreman, and sub tabs in Katello. Also fixed the lint issues, will resolve the conflicts (once laptop will get unstuck 🙃 )

@sjha4
Copy link
Member

sjha4 commented Jan 6, 2025

The new changes work good..Notification is proper and sub-tab tables occupy full width.

Noticed some weirdness in the global registration page, randomly the dropdowns show up like this:
Screenshot from 2025-01-06 15-54-00

@sjha4
Copy link
Member

sjha4 commented Jan 6, 2025

Locally, I am seeing only 7 failures, some nock ones and others where react test is failing to read by label text on 2 pages..Looking at those..Not sure what's causing the CI failures here though..

@MariaAga
Copy link
Contributor Author

MariaAga commented Jan 7, 2025

CI failures are due to foreman-js pr not being merged/released, so the packages are wrong

@MariaAga
Copy link
Contributor Author

MariaAga commented Jan 7, 2025

Fixed select2 issue in foreman core

@sjha4
Copy link
Member

sjha4 commented Jan 7, 2025

The dropdown looks good now..Functionally seems to be in a good state..Looking into tests to see if they can be fixed locally..

Copy link
Member

@sjha4 sjha4 left a comment

Choose a reason for hiding this comment

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

With the foreman-js, foreman PRs, this PR works for Katello components in as much as I tested. The code looks right..The CI errors here are due to dependency and there are some relevant failures when running locally..I'm ok to merge this and get a look at the actual CI errors when the dependencies are resolved and have a follow-up PR for those.

@sjha4
Copy link
Member

sjha4 commented Jan 8, 2025

I have the commit with local test fixes but we need to run the CI once to make sure the same tests fail on the CI with PF5..
sjha4@08aae79

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants