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

LPS-113582|LPS-113583 Replaces the usages of Liferay.Notification and Liferay.Notice with Liferay.Util.openToast #5

Closed
wants to merge 30 commits into from

Conversation

wincent
Copy link

@wincent wincent commented Jun 16, 2020

Forwarded here from wincent#309, where it was stuck in CI hell. Let's see if we have any better luck over here.

Original description:

Notification related things depends on review from Product Teams.

Previously:

  1. diegonvs#28
  2. wincent#297
  3. jbalsas#2197 + wincent#307

cc @diegonvs @jbalsas

…he given string is a HTML or not, uses a property for using `title` and `message ` as HTML in openToast utility.

From: jbalsas#2179 (comment)
…and use directly `dangerouslySetInnerHTML` on Text component
…penToast`

Found usages via:

`git grep '\bLiferay\.Notification\b'`

`git grep liferay-notification`
Usages were found running

`git grep '\bLiferay\.Notice\b'`
`git grep liferay-notice`

`Liferay.Util.openToast` utility was created for opening toasts and considering that Notice usages are for opening dismissible toasts, I could replace directly
…of Liferay.Notice AUI component and Liferay.Notice usages were replaced with `openToast` and ClayAlerts doesn't have animations
…s defined in whole file, these default values aren't needed
`React will automatically append a “px” suffix to certain numeric inline style properties.`

From: https://reactjs.org/docs/dom-elements.html
…ring markup.

For some cases in JSPs, HTML strings are being passed to openToast.
…he given string is a HTML or not, uses a property for using `title` and `message ` as HTML in openToast utility.

From: jbalsas#2179 (comment)
…and use directly `dangerouslySetInnerHTML` on Text component
Early this property was being used for handling the instance of the Liferay.Notice component but now it's unnecessary
…in and wasn't using it. However, it was using `aui-alert` but not declaring it. `liferay-notice` depends on `aui-alert` and I'm assuming that it was working due to this
…ser don't pass the title/message value React will not throw an error trying to render undefined
… use `messageType` and `titleType` for handling toast contents
Apparently another module was calling `plugin` module and It was working in the past due to this and this dep wasn't described on the `liferay-session` module. Adding as a dependency of `liferay-session`  solves the problem
…sion`

In some cases, apparently depending on the hardware, `aui-component` is not being loaded causing an error not localizing `A.component` on session AUI component
@wincent
Copy link
Author

wincent commented Jun 16, 2020

ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 5 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 59cee07e5f5ab5c6b7de9b3fc4bba98834208e0c

Sender Branch:

Branch Name: LPS-113582-LPS-113583
Branch GIT ID: 699e17bb05c43a12695ba50025eb338316cecf81

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 20 out of 20 jobs passed

❌ ci:test:relevant - 151 out of 154 jobs passed in 2 hours 56 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 59cee07e5f5ab5c6b7de9b3fc4bba98834208e0c

Copied in Private Modules Branch:

Branch Name: master-private
Branch GIT ID: 1048e471849c8076c4823cc7dd4b82abffdac7c4

ci:test:stable - 20 out of 20 jobs PASSED
20 Successful Jobs:
ci:test:relevant - 151 out of 154 jobs PASSED
151 Successful Jobs:
For more details click here.

Failures unique to this pull:


Failures in common with acceptance upstream results at bed295e:

@john-co
Copy link

john-co commented Jun 16, 2020

ci:test:bundle

@john-co
Copy link

john-co commented Jun 16, 2020

[unrelated]

[possibly unrelated]

  • will verify KaleoformsUsecase#CompleteProcessWithUploadedDefinition manually since it fails on a different error message. Likely will need a test fix to update the assertion to a verify to avoid possible inconsistent failure.

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:bundle - 1 out of 1 jobs passed in 37 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: ee8af6d3c052b2e98f267e2ca397582fa816f1a7

Copied in Private Modules Branch:

Branch Name: master-private
Branch GIT ID: 1048e471849c8076c4823cc7dd4b82abffdac7c4

ci:test:bundle - 1 out of 1 jobs PASSED
For more details click here.
Test bundle downloads:

@john-co
Copy link

john-co commented Jun 16, 2020

@rodrigocunhaa , I think the failng kaleoforms test is unrelated. The behavior seems to be reproducible on test server test-6-1-2 at GIT ID: a5641e2. There is no success message after importing a workflow definition. I didn't see this error message on testray history though. Was this recently fixed?

@john-co
Copy link

john-co commented Jun 16, 2020

@wincent , I'm able to reproduce the failing test behavior on our test server so the failing tests look unrelated.

Reviewed. Test failures are unrelated. ✔️

Seeing the amount of tests run here, there's no need to retest.

@rodrigocunhaa
Copy link

Hi @john-co , It's actually a bug. I'll open the ticket and quarantine this test. Thanks!

@wincent
Copy link
Author

wincent commented Jun 17, 2020

So, there are 303 comments (the majority of them are bot interaction, I think) on this PR and its 5 previous incarnations). I'll be honest and confess that I do not have the stomach to look over it all with a magnifying glass again, but it seems ok based on a quick skim. Given that the test failures are confirmed to be unrelated, I am going to try a manual forward of this.

Last call for objections, anyone? (especially @jbalsas, who commented previously) because soon this will be gone. If it turns out that that's a mistake, we can revert.

@wincent
Copy link
Author

wincent commented Jun 17, 2020

Manual forward is here: brianchandotcom#90226

@wincent wincent closed this Jun 17, 2020
@jbalsas
Copy link

jbalsas commented Jun 17, 2020

Well...

Screen Shot 2020-06-17 at 18 35 02

Screen Shot 2020-06-17 at 18 35 00

It definitely looks a bit daunting, all those commits for such a relatively small diff change...

@jbalsas
Copy link

jbalsas commented Jun 17, 2020

LGTM overall... changes in session.js and portal-web really old components make me a bit uneasy about this. I think I remember some comments about session in one of the previous incarnations...

In any case let's get this in and see if we live to fight another day 😉

@wincent
Copy link
Author

wincent commented Jun 17, 2020

It definitely looks a bit daunting, all those commits for such a relatively small diff change...

It's a common pattern. Evidently I am bad at giving review feedback, and people are bad at understanding it.

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

Successfully merging this pull request may close these issues.

None yet

6 participants