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

Extending stateful URLS with node filters and Expanding/collapsing modular pipelines flag #1799

Merged
merged 48 commits into from
Apr 12, 2024

Conversation

jitu5
Copy link
Contributor

@jitu5 jitu5 commented Mar 11, 2024

Description

Resolves #1736

In this PR the stateful URLs to include additional functionalities such as

  • Node type filtering (type=task,data)
  • Tag filtering (tags=companies,evaluate)
  • Expanding/collapsing all modular pipelines (expandAllPipelines=true)
  • Reset filter button

Below are four scenarios where the possible values for new query parameters could be:

Empty url & New browser Empty url & Old browser Stateful url & New browser Stateful url & Old browser
types task,data (default) From local storage From stateful url If available, then from stateful URL, else from local storage.
tags From local storage From stateful url If available, then from stateful URL, else from local storage.
expandAllPipelines false (default) From local storage From stateful url From stateful url
pipeline_id _default_ (default) From local storage From stateful url From stateful url
reset-button.mov

Development notes

  • On first load query params pipeline_id, expandAllPipelines and types will always be part of url.
  • For path matching URLSearchParams used instead of matchPath.
  • Shortening our existing URL query params:
    'focused_id' to fid
    'selected_id' to sid
    'selected_name' to sn
    'pipeline_id' to pid

QA notes

  • All tests should pass

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

jitu5 and others added 18 commits January 25, 2024 14:52
* refactor router to accept new deployer inputs

Signed-off-by: ravi-kumar-pilla <[email protected]>

* refactor cli for shareableviz deploy

Signed-off-by: ravi-kumar-pilla <[email protected]>

* merge router change

Signed-off-by: ravi-kumar-pilla <[email protected]>

* PR comments fix

Signed-off-by: ravi-kumar-pilla <[email protected]>

---------

Signed-off-by: ravi-kumar-pilla <[email protected]>
* CLI command kedro viz build added

* Lint fix

* lint fix

* Lint fix

* add mypy ignore

* Missing build file added

* Lint error fix

* BaseDeployer class added

* Unused code removed

* Fix lint issue

* azure deploy initial draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* added base_deployer

* add deployer factory

* partial working draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* Test and comments of deployers updated

* test draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix lint

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove circular dependency

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix lint

Signed-off-by: ravi-kumar-pilla <[email protected]>

* revert back consent

Signed-off-by: ravi-kumar-pilla <[email protected]>

* minor updates

Signed-off-by: ravi-kumar-pilla <[email protected]>

* update pytests

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add pytest for azure shareableviz

Signed-off-by: ravi-kumar-pilla <[email protected]>

* refactor and add timeout

Signed-off-by: ravi-kumar-pilla <[email protected]>

* refactor cli

Signed-off-by: ravi-kumar-pilla <[email protected]>

* update pytest

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add release note

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix PR comments and flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* testing flaky c
y test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* resolve conflicts

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix PR comments

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add back cypress flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove cypress flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove duplicate pytest parameter

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove fsspec upper bound

Signed-off-by: ravi-kumar-pilla <[email protected]>

---------

Signed-off-by: ravi-kumar-pilla <[email protected]>
Co-authored-by: Jitendra Gundaniya <[email protected]>
Co-authored-by: rashidakanchwala <[email protected]>
* CLI command kedro viz build added

* Lint fix

* lint fix

* Lint fix

* add mypy ignore

* Missing build file added

* Lint error fix

* BaseDeployer class added

* Unused code removed

* Fix lint issue

* azure deploy initial draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* added base_deployer

* add deployer factory

* partial working draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* Test and comments of deployers updated

* test draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix lint

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove circular dependency

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix lint

Signed-off-by: ravi-kumar-pilla <[email protected]>

* revert back consent

Signed-off-by: ravi-kumar-pilla <[email protected]>

* initial draft

Signed-off-by: ravi-kumar-pilla <[email protected]>

* minor updates

Signed-off-by: ravi-kumar-pilla <[email protected]>

* update pytests

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add pytest for azure shareableviz

Signed-off-by: ravi-kumar-pilla <[email protected]>

* refactor and add timeout

Signed-off-by: ravi-kumar-pilla <[email protected]>

* refactor cli

Signed-off-by: ravi-kumar-pilla <[email protected]>

* update pytest

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add release note

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix PR comments and flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* testing flaky c
y test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* remove flaky test

Signed-off-by: ravi-kumar-pilla <[email protected]>

* add pytest for gcp

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix gcp pytest coverage

Signed-off-by: ravi-kumar-pilla <[email protected]>

* fix lint

Signed-off-by: ravi-kumar-pilla <[email protected]>

* update pytest

Signed-off-by: ravi-kumar-pilla <[email protected]>

* revert file permission change

---------

Signed-off-by: ravi-kumar-pilla <[email protected]>
Co-authored-by: Jitendra Gundaniya <[email protected]>
Co-authored-by: rashidakanchwala <[email protected]>
# Conflicts:
#	package/kedro_viz/api/rest/responses.py
#	package/requirements.txt
#	src/components/metadata/metadata.js
# Conflicts:
#	src/components/shareable-url-modal/shareable-url-modal.js
#	src/components/shareable-url-modal/shareable-url-modal.scss
#	src/components/shareable-url-modal/shareable-url-modal.test.js
#	src/config.js
@jitu5 jitu5 self-assigned this Mar 11, 2024
@jitu5 jitu5 added the Javascript Pull requests that update Javascript code label Mar 11, 2024
@jitu5 jitu5 marked this pull request as ready for review March 15, 2024 10:59
Copy link
Contributor

@rashidakanchwala rashidakanchwala left a comment

Choose a reason for hiding this comment

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

Hi LGTM thank @jitu5 ..awesome work :D

@ravi-kumar-pilla
Copy link
Contributor

Hi @jitu5 , Nice work.

I am trying out the scenario below -
Lets say someone would like to share a URL - http://127.0.0.1:4141/?pid=__default__&expandAllPipelines=false. This does not have nodeTypes (i.e., I want all types including parameters). But if I open this in incognito or new browser, the url gets appended with types=task,data. Is this desired, if so how can someone ever share a url which has all 3 nodeTypes ?

Thank you

@jitu5
Copy link
Contributor Author

jitu5 commented Apr 9, 2024

Hi @jitu5 , Nice work.

I am trying out the scenario below - Lets say someone would like to share a URL - http://127.0.0.1:4141/?pid=__default__&expandAllPipelines=false. This does not have nodeTypes (i.e., I want all types including parameters). But if I open this in incognito or new browser, the url gets appended with types=task,data. Is this desired, if so how can someone ever share a url which has all 3 nodeTypes ?

Thank you

@ravi-kumar-pilla
if you see the table above for nodeType with new browser & empty url(no nodeType params), the default ( types=task,data ) will apply. this is desired. If user wanted to to share with all nodeTypes, all should be included in urls.

@ravi-kumar-pilla
Copy link
Contributor

Hi @jitu5 , Nice work.
I am trying out the scenario below - Lets say someone would like to share a URL - http://127.0.0.1:4141/?pid=__default__&expandAllPipelines=false. This does not have nodeTypes (i.e., I want all types including parameters). But if I open this in incognito or new browser, the url gets appended with types=task,data. Is this desired, if so how can someone ever share a url which has all 3 nodeTypes ?
Thank you

@ravi-kumar-pilla if you see the table above for nodeType with new browser & empty url(no nodeType params), the default ( types=task,data ) will apply. this is desired. If user wanted to to share with all nodeTypes, all should be included in urls.

okay got it. I think it is a bit of design question as well. Should we make allSelected nodeTypes == noneSelected nodeTypes ? That is how the UI behaves at the moment. What do you think ? @stephkaiser @jitu5 @rashidakanchwala

@ravi-kumar-pilla
Copy link
Contributor

Hi @jitu5 , Appreciate your patience 💯

I am testing the below scenario -

  1. I have a tag filter with companies selected
  2. I go to incognito and paste the url - it works fine
  3. I try to remove the tags in the url and refresh the page, the tag re-appears as shown in the gif
  4. I tested the scenario on normal browser and it has the same issue

Please let me know if you need more information. Thank you

stateful_test1

@jitu5
Copy link
Contributor Author

jitu5 commented Apr 9, 2024

Hi @jitu5 , Appreciate your patience 💯

I am testing the below scenario -

  1. I have a tag filter with companies selected
  2. I go to incognito and paste the url - it works fine
  3. I try to remove the tags in the url and refresh the page, the tag re-appears as shown in the gif
  4. I tested the scenario on normal browser and it has the same issue

Please let me know if you need more information. Thank you

@rashidakanchwala
if see below table
Screenshot 2024-04-09 at 5 05 23 p m

  • If tags is present in url it will apply on UI
  • If tags is not part of url and you have new browser(no local-storage) nothing will apply
  • If tags is not part of url and and if your browser's local storage contains any tag (in your case 'companies') it will applies from your browser's local storage.

this is expected behaviour.

@stephkaiser
Copy link

Hi @jitu5 , Nice work.
I am trying out the scenario below - Lets say someone would like to share a URL - http://127.0.0.1:4141/?pid=__default__&expandAllPipelines=false. This does not have nodeTypes (i.e., I want all types including parameters). But if I open this in incognito or new browser, the url gets appended with types=task,data. Is this desired, if so how can someone ever share a url which has all 3 nodeTypes ?
Thank you

@ravi-kumar-pilla if you see the table above for nodeType with new browser & empty url(no nodeType params), the default ( types=task,data ) will apply. this is desired. If user wanted to to share with all nodeTypes, all should be included in urls.

okay got it. I think it is a bit of design question as well. Should we make allSelected nodeTypes == noneSelected nodeTypes ? That is how the UI behaves at the moment. What do you think ? @stephkaiser @jitu5 @rashidakanchwala

Thanks Ravi, do you mind rephrasing or explaining in a less-technical way? Im struggling to follow :) are we discussing if the way all/none node types are displayed in the URL is intuitive to users?

I also had a question regarding the types - what are the task and data types? From the UI i see we have Nodes, Datasets, or Parameters so i'm a bit confused.

Another question - shouldn't all pipelines be expanded by default?

@stephkaiser
Copy link

stephkaiser commented Apr 9, 2024

Hi @jitu5 , Appreciate your patience 💯

I am testing the below scenario -

  1. I have a tag filter with companies selected
  2. I go to incognito and paste the url - it works fine
  3. I try to remove the tags in the url and refresh the page, the tag re-appears as shown in the gif
  4. I tested the scenario on normal browser and it has the same issue

Please let me know if you need more information. Thank you

stateful_test1

it does seem a bit strange that users wont be able to tweak the URL themselves. What happens if the user clicks on the Companies tag to remove it from the UI, will the URL update?

@ravi-kumar-pilla
Copy link
Contributor

Hi @jitu5 , Appreciate your patience 💯
I am testing the below scenario -

  1. I have a tag filter with companies selected
  2. I go to incognito and paste the url - it works fine
  3. I try to remove the tags in the url and refresh the page, the tag re-appears as shown in the gif
  4. I tested the scenario on normal browser and it has the same issue

Please let me know if you need more information. Thank you

@rashidakanchwala if see below table Screenshot 2024-04-09 at 5 05 23 p m

  • If tags is present in url it will apply on UI
  • If tags is not part of url and you have new browser(no local-storage) nothing will apply
  • If tags is not part of url and and if your browser's local storage contains any tag (in your case 'companies') it will applies from your browser's local storage.

this is expected behaviour.

okay I understand that if there is any data in local storage, we try to get that and there is no way to check if the user removed all tags. For nodeTypes we do have some default but for tags we do not have such thing and that makes this test case confusing. Basically if there is local storage, the user can only disable a tag via UI and not via URL. May be we need to think about this approach. It is a bit confusing if I want to change the URL and it keeps appending tags.

@rashidakanchwala
Copy link
Contributor

rashidakanchwala commented Apr 9, 2024

http://127.0.0.1:4141/?pid=__default__&expandAllPipelines=false

Historically, parameters in the flowchart are hidden by default based on user research indicating that most users do not need to see them. This approach has not drawn complaints, suggesting it is generally acceptable. However if we see a lot of users complaining, maybe we can consider revising our approach.
I think, when node types are unspecified, the URL should automatically revert to the default view, which includes both task and data types.

@jitu5
Copy link
Contributor Author

jitu5 commented Apr 9, 2024

Hi @jitu5 , Appreciate your patience 💯
I am testing the below scenario -

  1. I have a tag filter with companies selected
  2. I go to incognito and paste the url - it works fine
  3. I try to remove the tags in the url and refresh the page, the tag re-appears as shown in the gif
  4. I tested the scenario on normal browser and it has the same issue

Please let me know if you need more information. Thank you
stateful_test1

    [
      
    
        ![stateful_test1](https://private-user-images.githubusercontent.com/87735534/320918972-3527cf61-8813-4208-af23-2b3b85543b87.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTI2Nzg4NjIsIm5iZiI6MTcxMjY3ODU2MiwicGF0aCI6Ii84NzczNTUzNC8zMjA5MTg5NzItMzUyN2NmNjEtODgxMy00MjA4LWFmMjMtMmIzYjg1NTQzYjg3LmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA0MDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNDA5VDE2MDI0MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTU1ZmZlZGE5YzAxMmJhNzQxMTY5YjIzYjVlOGI0NWRkNDEyZTQxOGY0Y2Q2MWIzZmM0OTNhZWQ5OGMwZWI0Y2ImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.1EHhJ9fXU64gtnbEkcyVPvKMFtaNldbbPfpckv33jfY)
      ](https://private-user-images.githubusercontent.com/87735534/320918972-3527cf61-8813-4208-af23-2b3b85543b87.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTI2Nzg4NjIsIm5iZiI6MTcxMjY3ODU2MiwicGF0aCI6Ii84NzczNTUzNC8zMjA5MTg5NzItMzUyN2NmNjEtODgxMy00MjA4LWFmMjMtMmIzYjg1NTQzYjg3LmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA0MDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNDA5VDE2MDI0MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTU1ZmZlZGE5YzAxMmJhNzQxMTY5YjIzYjVlOGI0NWRkNDEyZTQxOGY0Y2Q2MWIzZmM0OTNhZWQ5OGMwZWI0Y2ImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.1EHhJ9fXU64gtnbEkcyVPvKMFtaNldbbPfpckv33jfY)
    
    
      
        
          
        
        
          
          
        
      
      [
        
          
        
      ](https://private-user-images.githubusercontent.com/87735534/320918972-3527cf61-8813-4208-af23-2b3b85543b87.gif?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTI2Nzg4NjIsIm5iZiI6MTcxMjY3ODU2MiwicGF0aCI6Ii84NzczNTUzNC8zMjA5MTg5NzItMzUyN2NmNjEtODgxMy00MjA4LWFmMjMtMmIzYjg1NTQzYjg3LmdpZj9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA0MDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNDA5VDE2MDI0MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTU1ZmZlZGE5YzAxMmJhNzQxMTY5YjIzYjVlOGI0NWRkNDEyZTQxOGY0Y2Q2MWIzZmM0OTNhZWQ5OGMwZWI0Y2ImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.1EHhJ9fXU64gtnbEkcyVPvKMFtaNldbbPfpckv33jfY)

it does seem a bit strange that users wont be able to tweak the URL themselves. What happens if the user clicks on the Companies tag to remove it from the UI, will the URL update?

@stephkaiser
User can modify URL themselves. but wherever you select the tag or nodeType it updates the url as well as it update your browser's local storage. So whenever you hit URL without tag it will check for for your browser's local storage if any previously applied tag is present if yes, it applies on UI as well as it update URL

@rashidakanchwala
Copy link
Contributor

rashidakanchwala commented Apr 9, 2024

@jitu5 - I faced an issue related to tags that I got in the beginning but couldn't do it again whilst testing that case Ravi mentioned above.

  • Open the browser
  • Select companies tag. Updates URL to http://localhost:4141/?types=task,data&pid=__default__&expandAllPipelines=false&tags=companies
  • Select evaluate tag. Updates URL to http://localhost:4141/?types=task,data&pid=__default__&expandAllPipelines=false&tags=companies,evaluate
  • Now click on the tag group deselect. Updates URL to http://localhost:4141/?types=task,data&pid=__default__&expandAllPipelines=false&tags=shuttles,train

I think it should revert it back to no tags.

Team, I propose we meet tomorrow after the standup to discuss our approach to tags. Given our current way of saving, reading to local storage and reading from the URL, some of design decisions have been made that were thought most optimal while Steph was away. Let's see what we can release soon and identify areas for future improvement.

@rashidakanchwala
Copy link
Contributor

I also had a question regarding the types - what are the task and data types? From the UI i see we have Nodes, Datasets, or Parameters so i'm a bit confused.

task = node,
but from user perspective, I agree we should keep the language consistent. so we should say type = data,node,parameters.

@stephkaiser
Copy link

I think a meeting to discuss this work would be great, i wasn't involved in this so would be great to get a walkthrough and discuss tags and the default view. Also agree we should always keep language consistent, I would even suggest type = datasets,nodes,parameters.

@astrojuanlu
Copy link
Member

Please sign me up to the meeting!

@jitu5
Copy link
Contributor Author

jitu5 commented Apr 10, 2024

@jitu5 - I faced an issue related to tags that I got in the beginning but couldn't do it again whilst testing that case Ravi mentioned above.

  • Open the browser
  • Select companies tag. Updates URL to http://localhost:4141/?types=task,data&pid=__default__&expandAllPipelines=false&tags=companies
  • Select evaluate tag. Updates URL to http://localhost:4141/?types=task,data&pid=__default__&expandAllPipelines=false&tags=companies,evaluate
  • Now click on the tag group deselect. Updates URL to http://localhost:4141/?types=task,data&pid=__default__&expandAllPipelines=false&tags=shuttles,train

I think it should revert it back to no tags.

@rashidakanchwala I was able to reproduce this issue with group tag filter, yesterday I fix for this and now you can test it again.

@jitu5
Copy link
Contributor Author

jitu5 commented Apr 10, 2024

I think a meeting to discuss this work would be great, i wasn't involved in this so would be great to get a walkthrough and discuss tags and the default view. Also agree we should always keep language consistent, I would even suggest type = datasets,nodes,parameters.

@stephkaiser, Now types in URL matches with UI labels type=datasets,nodes,parameters thanks.

Copy link
Contributor

@ravi-kumar-pilla ravi-kumar-pilla left a comment

Choose a reason for hiding this comment

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

Great work @jitu5 !! Looks good to me

@astrojuanlu
Copy link
Member

I'm not qualified to comment on the code and also unfortunately don't have a lot of time to do manual QA of this in the face of #1764. Please don't wait on my review 🙏🏼 Thanks @jitu5 !

@jitu5 jitu5 merged commit 9405827 into main Apr 12, 2024
5 checks passed
@jitu5 jitu5 deleted the feature/stateful_url branch April 12, 2024 10:32
@jitu5 jitu5 mentioned this pull request Apr 16, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extending stateful URLS
6 participants