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

Add helpers to export all loaded annotations #5642

Merged
merged 1 commit into from
Jul 24, 2023
Merged

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Jul 20, 2023

This PR is part of #5640

Adding logic to build an annotations json export file, and "download" it.

Out of the scope of this PR

Usage

This PR introduces a new service and a helper function, intended to be used like this:

import { downloadJSONFile } from 'shared/download-json-file';
import type { AnnotationsExporter } from 'sidebar/services/annotations-exporter';

const annotationsExporter: AnnotationsExporter = ...;

const exportContent = annotationsExporter.buildExportContent();
downloadJSONFile(exportContent, 'annotations-export.json');

Testing steps

There are many ways to test this, which involve binding the new helper function to some UI component.

One possible option is editing src/sidebar/components/SelectionTabs.tsx, adding the next changes:

diff --git a/src/sidebar/components/SelectionTabs.tsx b/src/sidebar/components/SelectionTabs.tsx
index 5429a086e..932a20097 100644
--- a/src/sidebar/components/SelectionTabs.tsx
+++ b/src/sidebar/components/SelectionTabs.tsx
@@ -9,11 +9,13 @@ import {
 import classnames from 'classnames';
 import type { ComponentChildren } from 'preact';
 
+import { downloadJSONFile } from '../../shared/download-json-file';
 import type { SidebarSettings } from '../../types/config';
 import type { TabName } from '../../types/sidebar';
 import { applyTheme } from '../helpers/theme';
 import { withServices } from '../service-context';
 import type { AnnotationsService } from '../services/annotations';
+import type { AnnotationsExporter } from '../services/annotations-exporter';
 import { useSidebarStore } from '../store';
 
 type TabProps = {
@@ -89,6 +91,7 @@ export type SelectionTabProps = {
   // injected
   settings: SidebarSettings;
   annotationsService: AnnotationsService;
+  annotationsExporter: AnnotationsExporter;
 };
 
 /**
@@ -96,6 +99,7 @@ export type SelectionTabProps = {
  */
 function SelectionTabs({
   annotationsService,
+  annotationsExporter,
   isLoading,
   settings,
 }: SelectionTabProps) {
@@ -126,6 +130,14 @@ function SelectionTabs({
       )}
     >
       <div className="flex gap-x-6 theme-clean:ml-[15px]" role="tablist">
+        <button
+          onClick={() => {
+            const content = annotationsExporter.buildExportContent();
+            downloadJSONFile(content, 'annotations-export.json');
+          }}
+        >
+          Export
+        </button>
         <Tab
           count={annotationCount}
           isWaitingToAnchor={isWaitingToAnchorAnnotations}
@@ -199,4 +211,8 @@ function SelectionTabs({
   );
 }
 
-export default withServices(SelectionTabs, ['annotationsService', 'settings']);
+export default withServices(SelectionTabs, [
+  'annotationsService',
+  'annotationsExporter',
+  'settings',
+]);

This will generate a new "Export" button in the sidebar, which should download the export file when clicked, including all the existing annotations on it.

@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #5642 (8b11e3c) into main (6f82c0e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 8b11e3c differs from pull request most recent head 2a1ae76. Consider uploading reports for the commit 2a1ae76 to get more accurate results

@@           Coverage Diff           @@
##             main    #5642   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files         240      242    +2     
  Lines        9435     9455   +20     
  Branches     2237     2239    +2     
=======================================
+ Hits         9382     9402   +20     
  Misses         53       53           
Impacted Files Coverage Δ
src/shared/download-json-file.ts 100.00% <100.00%> (ø)
src/sidebar/services/annotations-exporter.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@acelaya acelaya marked this pull request as ready for review July 20, 2023 09:38
Copy link
Contributor

@lyzadanger lyzadanger left a comment

Choose a reason for hiding this comment

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

Just a couple of passing notes from a quick peek:

I think it might be nice to separate the concerns of generating and downloading a file from the details of annotations. So you might have a util module function that takes data and options (options, e.g. filename) and creates the blob and the hidden anchor tag, etc. But a separate helper module function that generates an array of properly-formatted annotation objects and container metadata.

Second, I don't think what you're calling ExportAnnotationOptions are really options — it's data. I'd expect options to be more like filename. Anyway, see what you think about maybe splitting this up?

@acelaya
Copy link
Contributor Author

acelaya commented Jul 20, 2023

I think it might be nice to separate the concerns of generating and downloading a file from the details of annotations. So you might have a util module function that takes data and options (options, e.g. filename) and creates the blob and the hidden anchor tag, etc. But a separate helper module function that generates an array of properly-formatted annotation objects and container metadata.

Yeah, I like it. I'll do the changes and then document here and/or the issue how the functions are then supposed to be used.

@acelaya acelaya force-pushed the export-annotations-logic branch 2 times, most recently from 37b5141 to 2057cf2 Compare July 20, 2023 12:43
_document = document
): string {
const link = _document.createElement('a');
const fileContent = JSON.stringify(exportData, null, 2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point I'm not taking into consideration possible non-serializable content on provided data, like functions and such. I don't think we need to worry about that for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to "deal" with non-serializable values, as such, but do you think perhaps we could handle the fact that this function can throw? It could be the responsibility of the caller to handle exceptions (e.g. maybe we show a toast error if this export-file function encounters an error/throws), but perhaps we could at least document that the function can throw?

Copy link
Contributor

@lyzadanger lyzadanger left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this! I think we're progressing well here. I had a couple of questions here for you to think about. We can talk with voices if you'd like, to move this forward faster!

_document = document
): string {
const link = _document.createElement('a');
const fileContent = JSON.stringify(exportData, null, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to "deal" with non-serializable values, as such, but do you think perhaps we could handle the fact that this function can throw? It could be the responsibility of the caller to handle exceptions (e.g. maybe we show a toast error if this export-file function encounters an error/throws), but perhaps we could at least document that the function can throw?

* @param _document - Test seam
* @return The contents of the downloaded file
*/
export function exportJSONFile(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if downloadJSONFile might be more descriptive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It definitely is.

Comment on lines 3 to 14
export type ExportAnnotationsData = {
annotations: Annotation[];
clientVersion: string;
userId: string;
};

export type ExportContent = {
export_date: string;
export_userid: string;
client_version: string;
annotations: Annotation[];
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now, both the caller and this helper need to be aware of the "container" metadata for the export file, here typed in ExportAnnotationsData. I wonder if it might be possible instead to make this helper take just annotations and do the container-property building itself. It could simplify the API and typing, because right now ExportAnnotationsData and ExportContent pertain to overlapping properties (clientVersion => client_version, e.g.) and this could be confusing over time, especially if the amount of container properties gets long.

To accomplish this, it's possible this might have to be a service instead of a helper, but I think its worth considering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That actually makes a lot of sense.

@acelaya acelaya changed the title Add helper function to export all loaded annotations Add helpers to export all loaded annotations Jul 21, 2023
@acelaya acelaya requested a review from lyzadanger July 21, 2023 15:23
buildExportContent(now = new Date()): ExportContent {
const profile = this._store.profile();
const annotations = this._store.allAnnotations();
const versionData = new VersionData(profile, []);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't found a better way to properly get the client version, other than creating a new VersionData instance.

The main problem is that this class wraps more information and logic, but at the same time, it is the single place where the __VERSION__ placeholder is defined, ensuring it will be replaced at build time with the actual version.

Another option would be to make this service define its own property with __VERSION__ value.

Copy link
Member

Choose a reason for hiding this comment

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

__VERSION__ is a placeholder that gets replaced by Rollup. It can appear in any file. See references to __VERSION__ in rollup.config.mjs.

Copy link
Contributor Author

@acelaya acelaya Jul 21, 2023

Choose a reason for hiding this comment

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

Yep, that's why I mentioned this.

Another option would be to make this service define its own property with __VERSION__ value.

What I don't know if it would be desirable or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like the smell here is VersionData's dependency on profile. Seems reasonable to make VersionData a source of information about, well, version data, but that module could use some cleanup and detangle. For now I think this is OK, especially since you need profile in this scope anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's exactly my thinking.

In fact, I passed profile there just because I had it for free, but the truth is that anything would still work as we are just reading versionData.version here.

Copy link
Contributor

@lyzadanger lyzadanger left a comment

Choose a reason for hiding this comment

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

I think this is at a reasonable place to land for this stage of the project. Thank you!

There are some things I'd like us to think about:

  • Unit-testing the download util and the exporter is not really sufficient to test the feature. I don't have a proposed solution for this.
  • Being really diligent about browser-testing this feature

buildExportContent(now = new Date()): ExportContent {
const profile = this._store.profile();
const annotations = this._store.allAnnotations();
const versionData = new VersionData(profile, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like the smell here is VersionData's dependency on profile. Seems reasonable to make VersionData a source of information about, well, version data, but that module could use some cleanup and detangle. For now I think this is OK, especially since you need profile in this scope anyway.

src/shared/download-json-file.ts Outdated Show resolved Hide resolved
src/shared/download-json-file.ts Show resolved Hide resolved
Comment on lines +26 to +28
_document.body.appendChild(link);
link.click();
_document.body.removeChild(link);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking of reaching out for some QA help (i.e. outside of our team) to aggressively test this download in a lot of browsers as this feature starts to roll out behind a feature flag. This is an industry-standard trick here as far as I can ascertain, but we should be diligent!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I have used this approach in multiple projects in the past, and never experienced issues, but it definitely feels like a hack every time I see it.


it('generates export content with provided annotations', () => {
const now = new Date();
const annotations = [{}, {}];
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we're about to build on this work to define what fields do get delivered, this is OK for now; we should use fixtures at some point as this feature gets more defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is a bit improved in #5644, where the property cleanup implementation is introduced.

I'll make sure I do something even closer to reality there.

@acelaya acelaya merged commit 683fde6 into main Jul 24, 2023
2 checks passed
@acelaya acelaya deleted the export-annotations-logic branch July 24, 2023 13:26
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.

3 participants