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

frontend: Fix router event handling in base class #2375

Conversation

gorankokin
Copy link
Contributor

@gorankokin gorankokin commented Jul 10, 2023

  • Fix router event handling in DataJobsBaseGridComponent
    • Distinguish between Browser popped state and changed Team
    • fix will affect only case when there is external Team selector dependency like DataPipelinesConfig['manageConfig'].selectedTeamNameObservable
  • Added new SystemEvent with id "SE_Location_Change" bound to constant SE_LOCATION_CHANGE that is publicly exposed through "@versatiledatakit/shared"
    • with the new core SystemEvent there is provided interface for its payload
    • event is fired whenever method locationToURL in URLStateManager is invoked and condition allows location change
  • Small refactor: introduced interface for SystemEvent payload about event with id "SE_Navigate"

Tested with Supercollider in local dev environment and everything works as expected.
Test scenarios:

  1. In Data Jobs manage there are applied multiple filters in the grid and there is external dependency that team A is selected.
    1. whenever external dependency notify that team was changed to B, content will be reloaded and Data Jobs for team B would be loaded and all filters that are applied in the grid will be preserved and used to load Data Jobs for team B
    2. before the fix, on Team change filters were deleted and default filter by status enabled in manage wasn't preserved
  2. Tests that the newly introduced event could be consumed through SystemEventHandler in supercollider codebase and expected payload was received with attached debugger, and afterward processing is done for the needs of supercollider.
  3. Automation tests are not added, because in public VDK UI there is no concept for external Team dependency (global Team selector) because it lays on Taurus foundation, where there wasn't concept of Teams but rather than that it was built around deployment per Tenant. Adding parametrized public VDK UI just for the test (where exist some abstract Team selector exist) is additional development cost for which we don't find its benefits right now.

@gorankokin gorankokin requested a review from hzhristova as a code owner July 10, 2023 08:44
@gorankokin gorankokin self-assigned this Jul 10, 2023
@gorankokin gorankokin added the area/frontend Related to changes in the folder projects/frontend for details about ui please see readme label Jul 10, 2023
@gorankokin gorankokin force-pushed the person/gkokinovski/fix-navigation-in-data-jobs-for-telemetry-2 branch from c64871d to e8ddd7b Compare July 10, 2023 09:00
@antoniivanov
Copy link
Collaborator

Fix router event handling in DataJobsBaseGridComponent.
Distinguish between Browser popped state and changed Team.

What is the problem here? Please can you explain in the PR description.

So is it when I navigate back to data jobs page (e.g click back) or when I change team in the UI, this causes problem.... Is that correct ? And what problem? Without understanding the actual problem it's hard to review properly.

Tested with Supercollider in local dev environment and everything works as expected.

Though I cannot comment on the testing until I am sure I understand the problem, description is not enough. Please provide information on exact manual steps so that if I have to re-do the the test myself I can do that without help. Also why aren't automated test in the codebase possible ?

Fix router event handling in DataJobsBaseGridComponent.
Distinguish between Browser popped state and changed Team.i
Added new SystemEvent with id 'SE_Location_Change' bound to
constant SE_LOCATION_CHANGE that is publicly exposed
through '@versatiledatakit/shared'.
 - with the new core SystemEvent there is provided
interface for its payload
 - event is fired whenever method locationToURL
in URLStateManager is invoked
and condition allows location change
Small refactor, introduced interface for SystemEvent
with id 'SE_Navigate' payload.

Signed-off-by: Goran Kokinovski <gkokinovski@vmware.com>
@gorankokin gorankokin force-pushed the person/gkokinovski/fix-navigation-in-data-jobs-for-telemetry-2 branch from e8ddd7b to 999d3e0 Compare July 10, 2023 11:50
@gorankokin
Copy link
Contributor Author

Fix router event handling in DataJobsBaseGridComponent.
Distinguish between Browser popped state and changed Team.

What is the problem here? Please can you explain in the PR description.

So is it when I navigate back to data jobs page (e.g click back) or when I change team in the UI, this causes problem.... Is that correct ? And what problem? Without understanding the actual problem it's hard to review properly.

Tested with Supercollider in local dev environment and everything works as expected.

Though I cannot comment on the testing until I am sure I understand the problem, description is not enough. Please provide information on exact manual steps so that if I have to re-do the the test myself I can do that without help. Also why aren't automated test in the codebase possible ?

I added more information in the description

@gorankokin gorankokin merged commit a3616bb into main Jul 10, 2023
@gorankokin gorankokin deleted the person/gkokinovski/fix-navigation-in-data-jobs-for-telemetry-2 branch July 10, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to changes in the folder projects/frontend for details about ui please see readme cla-not-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants