Hide filter components when sharing

[Avey777]

What type of PR is this?

• [ ] Refactor
• [x] Feature
• [ ] Bug Fix
• [ ] New Query Runner (Data Source)
• [ ] New Alert Destination
• [x] Other

Description

How is this tested?

• [x] Unit tests (pytest, jest)
• [ ] E2E Tests (Cypress)
• [x] Manually
• [ ] N/A

Related Tickets & Documents

27

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

[Avey777]

@justinclift
What should I do next to merge the data successfully?

[justinclift]

@Avey777 Not sure, my head space is more into Go programming than jsx at the moment. @gaecoli Any ideas? :smile:

[Avey777]

Not sure, my head space is more into Go programming than jsx at the moment. @gaecoli Any ideas?

My local testing is normal, what is the reason why the code cannot be merged?

[Avey777]

@gaecoli Can you review this code merge?

[gaecoli]

@gaecoli Can you review this code merge?

I think some scenarios are not tested from the code

[gaecoli]

@gaecoli Can you review this code merge?

I think some scenarios are not tested from the code

@Avey777

[gaecoli]

I think this is a PR with a relatively large impact, so it needs to pass the CI test.

[gaecoli]

Can you post some renderings? @Avey777 It makes others understand what do you want to do. Thank you!

[Avey777]

@gaecoli I have optimized the code and tested it locally.

[justinclift]

Wonder if we should add a new Cypress test (or tests) for it, to make sure it's always working as intended?

[Avey777]

Wonder if we should add a new Cypress test (or tests) for it, to make sure it's always working as intended?

@gaecoli I can't understand the test file. I just made a small change to the presentation of the sharing page. Can I merge the code directly?

What should I do now to make it pass the test?

[Avey777]

Can you post some renderings? @Avey777 It makes others understand what do you want to do. Thank you!

[justinclift]

I've just restarted the failing end-to-end test piece. It uses a javascript test framework called Cypress. It's generally ok, but does seem to be unreliable.

As in, when first starting out with adding Cypress tests to a project it seems reliable. But once you have a reasonable number of tests then it's fairly common for them to just occasionally fail when running. :frowning:

That being said I've only used it on this project and one other, so maybe other places have it run more reliably. I'm not sure. :smile:

[justinclift]

Can I merge the code directly?

Nah, lets make sure it doesn't break the test suite. If it keeps on causing the testing to fail, then there's probably an actual problem that needs fixing. :smile:

[justinclift]

Ahhh. It looks like the tests are actually failing now, because they're expecting a user interface element to be present... but it's now hidden:

AssertionError: Timed out retrying: Expected to find element: `[data-test="ParameterApplyButton"]`, but never found it.

I'm guessing we'll probably need to remove that specific piece of the test.

[Avey777]

Ahhh. It looks like the tests are actually failing now, because they're expecting a user interface element to be present... but it's now hidden:

AssertionError: Timed out retrying: Expected to find element: `[data-test="ParameterApplyButton"]`, but never found it.

I'm guessing we'll probably need to remove that specific piece of the test.

"I will handle this."

[justinclift]

Cool. As a data point, the version of Cypress being used by Redash is fairly old. Version 5.3.0, while the current version of Cypress is 12.14.0.

So, if you need to read Cypress docs make sure you read the appropriate ones for the old version.

Btw, there were some major changes around Cypress version 10, so it's not straight forward for us to just switch to a recent version. :wink:

[Avey777]

Cool. As a data point, the version of Cypress being used by Redash is fairly old. Version 5.3.0, while the current version of Cypress is 12.14.0.

So, if you need to read Cypress docs make sure you read the appropriate ones for the old version.

Btw, there were some major changes around Cypress version 10, so it's not straight forward for us to just switch to a recent version. 😉

It seems that we have a lot of dependencies that need to be gradually upgraded to slightly higher versions. The times are changing, and versions that are too outdated cannot effectively move us forward. A lot of time is wasted on dealing with these trivial matters.

[justinclift]

Yeah. I think that once we have things in a reasonable state, we'll need to look at a better approach to all this.

My feeling is that the dependency management for Redash might be more complicated (at present) than code development. Mainly because some updated dependencies need their own changes in Redash's code, and we seem to only find that out when things break. :frowning:

[Avey777]

@gaecoli How to restart the test? It has been fixed now.

[justinclift]

To restart the test, click on the failed one to open the right page. Then there's a button to rerun the tests.

---

Ahhh, this one was a bit different. It needed a button to be clicked on to "run the workflow". I've just done that, so lets see how this goes... :smile:

[Avey777]

To restart the test, click on the failed one to open the right page. Then there's a button to rerun the tests.

Ahhh, this one was a bit different. It needed a button to be clicked on to "run the workflow". I've just done that, so lets see how this goes... 😄

I have done my best to resolve all the errors that I could understand. However, I do not understand what the problem is with this current issue. @gaecoli

[Avey777]

I didn't see the button to execute the test on the page you mentioned. Could you please take a screenshot and show it to me? @gaecoli

[gaecoli]

I didn't see the button to execute the test on the page you mentioned. Could you please take a screenshot and show it to me? @gaecoli

I started it.

[justinclift]

Seems to be only a single test failure now:

The test it mentions is the one here:

https://github.com/RedashCommunity/redash/blob/62e4f6b52b3e77616700eb641a3ac12cb970b3ae/client/cypress/integration/dashboard/sharing_spec.js#L170-L201

That's showing a "time out" error, which can sometimes be a real error, or can sometimes be a false positive failure.

@gaecoli is running it again now, so hopefully it passes this time around.

[Avey777]

I didn't see the button to execute the test on the page you mentioned. Could you please take a screenshot and show it to me? @gaecoli

I started it.

I'm struggling too much and haven't even touched this part of the code. I currently don't have any solutions in mind.

[Avey777]

I noticed that the 'PublicAccessEnabled' element is not being used in the code. I'm having trouble making changes to this part, could you help me figure out how to modify it? @gaecoli

[justinclift]

Yeah, there's a bunch of stuff to get the hang of. Don't give up yet though. :smile:

For the re-running of a failed job, the button is here:

It's the "Re-run jobs" button on the right hand side. It can either re-run all the tasks, or just the failed ones. For our purposes, they're both similar though it's a bit faster to just do the failed ones.

---

Looking at the latest test run, it's failed in two places:

• The first place is the same spot as last time, so it probably means this is a real failure that we need to look into.
• The 2nd spot didn't fail last time around so that's very likely a false positive that can be ignored.

For fixing that first one, probably the best approach is to:

1. Install the Cypress 5.3.0 GUI and run it
2. Point it at the Redash cypress configuration file in your local git repo directory
3. Then navigate to the failing test and try running it

The Cypress GUI is used here as it'll show you visually what the test is trying to do. So you can either modify the test, or modify your code.

Looking over the Cypress tests that Redash has... they're a bit more advanced than the simple tests I'm more familiar with. I can probably help out a bit, but it'll be in a few hours time as I'm doing some other stuff at the moment.

---

Oh, be aware that running the Cypress tests manually from the GUI will generally require you to have built and started the docker image before hand. It uses that running docker image to do the tests on.

[Avey777]

@justinclift There is no "restart" button on my interface, so I'm planning to try it out within my fork.

[justinclift]

@Avey777 Ahhh, that's right. You didn't accept the group invite, so you don't have permissions to run stuff. Gimme a minute, I'll send it again. :smile:

---

Invite sent. If you accept that, then you should have the "Re-run ..." button show up when you visit that page. :smile:

[Avey777]

@Avey777 Ahhh, that's right. You didn't accept the group invite, so you don't have permissions to run stuff. Gimme a minute, I'll send it again. 😄

Invite sent. If you accept that, then you should have the "Re-run ..." button show up when you visit that page. 😄

Yes, I have accepted the invitation now.

[justinclift]

Cool. The "Rerun ..." button is showing up for you now yeah?

[Avey777]

Cool. The "Rerun ..." button is showing up for you now yeah?

yeah

[Avey777]

I am a bit skeptical about whether the previous code can pass the test normally, and I have not made any modifications to the errors reported

[Avey777]

Perfect， Code has passed the tests. @gaecoli

[Avey777]

@justinclift

Can you merge it?

[Avey777]

I merged it.

[gaecoli]

Sounds great. 🎉 Thank you!

[justinclift]

Awesome, well done! :smile:

[justinclift]

I merged it.

As a tip for the future, when merging a PR there are two main options:

1. Merge the PR and preserve all of the commits in the PR, or
2. Merge the PR, and combine (squash) all of the commits in the PR into a single commit

The 2nd option is generally the one to choose when there are many commits in the PR. Otherwise the commit history for the repo gets messy and a bit harder to work with.

That's only a neatness thing, and doesn't really affect things all that badly. It's something to get the hang of over time though as we work on stuff. :smile: