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

When a user has no roles, logout on button click #830

Merged

Conversation

leanneeliatra
Copy link
Contributor

@leanneeliatra leanneeliatra commented Sep 7, 2023

Description

Cypress tests for testing the no role button. On click, the user should be logged out.
opensearch-project/security-dashboards-plugin#1564

Issues Resolved

opensearch-project/security-dashboards-plugin#1564

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@leanneeliatra leanneeliatra force-pushed the missing-roles-button-tests branch 2 times, most recently from ebd5edf to c5276f8 Compare September 7, 2023 14:06
@leanneeliatra leanneeliatra changed the title adding test to check when a user has no roles, clicking button will l… When a user has no roles, logout on button click Sep 7, 2023
@kavilla
Copy link
Member

kavilla commented Sep 7, 2023

@leanneeliatra looks like changes were requested on the original PR, should that be addressed here too?

@leanneeliatra
Copy link
Contributor Author

@leanneeliatra looks like changes were requested on the original PR, should that be addressed here too?

Hi @kavilla thanks for the comment, the outstanding comment referenced the integration tests which are here now. I have requested a re-review so hopefully that's it! Thanks. If you were referring to something else please let me know but everything is addressed on the original PR now.

Copy link
Contributor

@derek-ho derek-ho left a comment

Choose a reason for hiding this comment

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

May need to change the button text here as well if that is done

updating as per updates & request

Signed-off-by: leanneeliatra <[email protected]>
@leanneeliatra
Copy link
Contributor Author

May need to change the button text here as well if that is done

That's complete thanks.

…_role_button_logout.js

Co-authored-by: Yulong Ruan <[email protected]>
Signed-off-by: leanneeliatra <[email protected]>
@leanneeliatra
Copy link
Contributor Author

@ruanyl @wanglam @Hailong-am @raintygao could the work in this MR please be reviewed, I have been in discussions to get the bug change itself over the line and when that is ready, I don't want the test to fall behind! If it could be reviewed and ready that would be great, thank you.

@Hailong-am
Copy link
Collaborator

@leanneeliatra would you mind fix the lint check https://github.com/opensearch-project/opensearch-dashboards-functional-test/actions/runs/6470978871/job/17568385980?pr=830

@leanneeliatra
Copy link
Contributor Author

@leanneeliatra would you mind fix the lint check https://github.com/opensearch-project/opensearch-dashboards-functional-test/actions/runs/6470978871/job/17568385980?pr=830

I was having issues doing this change locally yesterday, then I was getting 347 linting errors on other files. I made the change by editing the file here, in GitHub. If you have any advice about this please share.

Thanks @Hailong-am. The lint fix is now complete.

@leanneeliatra
Copy link
Contributor Author

@RyanL1997 was going to have a look at the cypress tests for me (thank you :) )

@leanneeliatra
Copy link
Contributor Author

I'm not sure if I should be requesting reviews in the meantime, if all the checks aren't passed but in case it is okay to proceed (let me know if I should wait) can I please get a review @wanglam, @raintygao, @Hailong-am

@kavilla
Copy link
Member

kavilla commented Oct 11, 2023

@leanneeliatra ive been pulling stuff down and verifying against a targetted release candidate as i still need to fix the ci here.

In the meantime can you post screen shots of these test passing and what is ran against? Also could post a screen shot of the dashboard sanity tests result as it will help verify this PR doesnt interfer with other tests.

This will help other maintainers with velocity

@leanneeliatra
Copy link
Contributor Author

In the meantime can you post screen shots of these test passing and what is ran against? Also could post a screen shot of the dashboard sanity tests result as it will help verify this PR doesnt interfer with other tests.

For sure thanks @kavilla and thanks very much for checking this for me too.
I will add the test images.
For this request: Also could post a screen shot of the dashboard sanity tests result as it will help verify this PR doesn't interfere with other tests. I'm not certain what tests you are referring to here/where to run them please.

@leanneeliatra
Copy link
Contributor Author

leanneeliatra commented Oct 12, 2023

@kavilla I ran the test locally & am attaching screenshots:
Testing the changes and tests locally.
repo: opensearch-dashboards-functional-test
branch: leanneeliatra:missing-roles-button-tests
repo: security-dashboards-plugin
branch: leanneeliatra:2.8

terminal output PR830

test PR380

Hopefully that's okay.

And can you please clarify which tests are the dashboard sanity tests and where I can run them please.

@leanneeliatra
Copy link
Contributor Author

leanneeliatra commented Oct 23, 2023

Are there remaining tests needed here from my end? I've added the passing cypress tests locally - perhaps this is enough.

Would it also be possible to get the remaining review if all is okay now please @Hailong-am Thanks a million for all your help.

@leanneeliatra
Copy link
Contributor Author

The fix itself has now been merged opensearch-project/security-dashboards-plugin#1564
If the associated tests (this MR) could be reviewed and merged that would be great. The faling cypress test check above is due to cypress itself, the successfully running local test can be seen in a screenshot above.

Can this be merged please? @ruanyl could you confirm that for me please? I seem to have 2 reviews, yourself and @derek-ho, but I still need one more as only one of these is registering. Is that correct? Thanks.

@ruanyl
Copy link
Member

ruanyl commented Oct 24, 2023

@leanneeliatra I cannot perform merge right now, still need one more approval.

@leanneeliatra
Copy link
Contributor Author

Thank you for checking this @ruanyl , I am requesting the last review via slack and PR.

The failing cypress test check above is due to cypress itself, the successfully running local test can be seen in a screenshot above.

The associated bug fix is merged, can I please get a final review of the tests (this PR). Thanks very much. cc @Hailong-am , @xluo-aws , @SuZhou-Joe

@Hailong-am Hailong-am merged commit ab57fb3 into opensearch-project:main Oct 24, 2023
36 of 37 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 24, 2023
* adding test to check when a user has no roles, clicking button will log them out

Signed-off-by: leanneeliatra <[email protected]>
Signed-off-by: [email protected] <[email protected]>

* Remove unneeded code & depreciated server()

Signed-off-by: leanneeliatra <[email protected]>
Signed-off-by: [email protected] <[email protected]>

* remove extra comments

Signed-off-by: leanneeliatra <[email protected]>
Signed-off-by: [email protected] <[email protected]>

* removed whitespace

Signed-off-by: [email protected] <[email protected]>

* Update missing_role_button_logout.js

updating as per updates & request

Signed-off-by: leanneeliatra <[email protected]>

* Update cypress/integration/plugins/security-dashboards-plugin/missing_role_button_logout.js

Co-authored-by: Yulong Ruan <[email protected]>
Signed-off-by: leanneeliatra <[email protected]>

* Update missing_role_button_logout.js

Signed-off-by: leanneeliatra <[email protected]>

* Update missing_role_button_logout.js

Signed-off-by: leanneeliatra <[email protected]>

* Update missing_role_button_logout.js

Signed-off-by: leanneeliatra <[email protected]>

---------

Signed-off-by: leanneeliatra <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: leanneeliatra <[email protected]>
Co-authored-by: Yulong Ruan <[email protected]>
(cherry picked from commit ab57fb3)
Hailong-am pushed a commit that referenced this pull request Oct 24, 2023
* adding test to check when a user has no roles, clicking button will log them out

Signed-off-by: leanneeliatra <[email protected]>
Signed-off-by: [email protected] <[email protected]>

* Remove unneeded code & depreciated server()

Signed-off-by: leanneeliatra <[email protected]>
Signed-off-by: [email protected] <[email protected]>

* remove extra comments

Signed-off-by: leanneeliatra <[email protected]>
Signed-off-by: [email protected] <[email protected]>

* removed whitespace

Signed-off-by: [email protected] <[email protected]>

* Update missing_role_button_logout.js

updating as per updates & request

Signed-off-by: leanneeliatra <[email protected]>

* Update cypress/integration/plugins/security-dashboards-plugin/missing_role_button_logout.js

Co-authored-by: Yulong Ruan <[email protected]>
Signed-off-by: leanneeliatra <[email protected]>

* Update missing_role_button_logout.js

Signed-off-by: leanneeliatra <[email protected]>

* Update missing_role_button_logout.js

Signed-off-by: leanneeliatra <[email protected]>

* Update missing_role_button_logout.js

Signed-off-by: leanneeliatra <[email protected]>

---------

Signed-off-by: leanneeliatra <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: leanneeliatra <[email protected]>
Co-authored-by: Yulong Ruan <[email protected]>
(cherry picked from commit ab57fb3)

Co-authored-by: leanneeliatra <[email protected]>
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.

5 participants