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

KB-45700: Update Telemeter Tracking to Utilize Promises #954

Merged
merged 6 commits into from
May 16, 2024

Conversation

usantos-at-wiris
Copy link
Contributor

@usantos-at-wiris usantos-at-wiris commented Apr 24, 2024

Changes

feat: update telemeter tracking to utilize promises. #KB-45700

Description

This PR introduces a significant enhancement to our telemeter tracking functionality by transitioning all tracking actions to utilize promises. The motivation behind this change stems from a recent integration issue encountered with OnlyOffice.

Steps to Reproduce:

  1. Pull the wiris/telemeter repo.
  2. Initiate the telemeter server by executing cargo run and ensure it's operational.
  3. Navigate to packages/devkit/src/Integrationmodel on this branch.
  4. Amend the code from debug: false to debug: true.
  5. Execute yarn && nx build generic && nx start html-generic to build the generic editor.
  6. Verify the connection of the generic editor to the telemeter server.
  7. Interact with the generic editor by inserting formulas, closing, and reopening it.
  8. Observe the telemetry actions being accurately tracked on the telemeter server.

Commands to test generic telemetry

WirisPlugin.GenericIntegration.telemeter.wrsOpenedEditorModal('button','chemistry')

WirisPlugin.GenericIntegration.telemeter.wrsClosedEditorModal('mtct_close','chemistry')

WirisPlugin.GenericIntegration.telemeter.wrsInsertedFormula('«math xmlns=¨http://www.w3.org/1998/Math/MathML¨»«msqrt»«mi»x«/mi»«/msqrt»«/math»','«math xmlns=¨http://www.w3.org/1998/Math/MathML¨»«msqrt»«mi»x«/mi»«/msqrt»«/math»',Date.now(),'chemistry')

Link to Task: #45700

Copy link
Contributor

@jgonzalez-at-wiris jgonzalez-at-wiris left a comment

Choose a reason for hiding this comment

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

To make sure the applied changes really have effect, we´d have to await all the methods we appended the async keyword to... In other words, for example, now that we made insertFormula an async method, we need to check all places where that method is used and await its execution... This could bring us to an infinity spiral of async/await methods, let's check it at least!

@carla-at-wiris
Copy link
Contributor

To make sure the applied changes really have effect, we´d have to await all the methods we appended the async keyword to... In other words, for example, now that we made insertFormula an async method, we need to check all places where that method is used and await its execution... This could bring us to an infinity spiral of async/await methods, let's check it at least!

The code is thought to run in serial mode, so it may not be an issue to add the async/await where needed. However, take into consideration that there are cyclic methods.

Copy link
Contributor

@jgonzalez-at-wiris jgonzalez-at-wiris left a comment

Choose a reason for hiding this comment

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

Left a comment regarding some refactor code that was interesting to be included in this PR.

packages/devkit/src/modal.js Show resolved Hide resolved
@usantos-at-wiris
Copy link
Contributor Author

Sorry for the length of this PR. Fixed a bunch of typos and added a more considerate way to track the closing telemetry.

packages/devkit/src/integrationmodel.js Outdated Show resolved Hide resolved
packages/devkit/src/integrationmodel.js Outdated Show resolved Hide resolved
packages/generic/wirisplugin-generic.src.js Outdated Show resolved Hide resolved
Copy link
Contributor

@carla-at-wiris carla-at-wiris left a comment

Choose a reason for hiding this comment

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

Even though there are interesting changes improving the code, they are removing important telemetry calls and mixing some of them. In the comments, i specified which ones.

packages/devkit/src/modal.js Show resolved Hide resolved
packages/generic/wirisplugin-generic.src.js Outdated Show resolved Hide resolved
packages/devkit/src/modal.js Outdated Show resolved Hide resolved
packages/devkit/src/modal.js Outdated Show resolved Hide resolved
packages/devkit/src/modal.js Outdated Show resolved Hide resolved
usantos-at-wiris and others added 6 commits May 16, 2024 15:27
fix: change debug mode to false

refactor: change the way the promise is handled

revert: Only the close telemeter track is async

fix: change quote types
…eds to and fixed typos

fix: miss to restore debug mode to false

fix: Generic telemetry work and correct some misc errors with methods

fix: Add the trigger capability to a global attribute

docs: Update docs of some functions to explain what it does

fix: remove unwanted files

fix: restore unwanted files

fix: debug mode to false and changed telemeter object
@carla-at-wiris carla-at-wiris merged commit 3e15121 into master May 16, 2024
14 checks passed
@xjiang-at-wiris xjiang-at-wiris deleted the KB-45700 branch May 16, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants