feat(excel): support excel spreadsheets on sharepoint sites #6

trieloff · 2020-03-25T14:57:34Z

Please ensure your pull request adheres to the following guidelines:

make sure to link the related issues in this description
when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

Related Issues

Thanks for contributing!

tripodsan

better to use the graph api.

test/excel.test.js

tripodsan · 2020-03-26T01:08:10Z

src/matchers/excel.js

+    const driveItem = await drive.getDriveItemFromShareLink(url);
+    const xlsxbuffer = await drive.downloadDriveItem(driveItem);
+
+    const workbook = XLSX.read(xlsxbuffer, { type: 'buffer' });


probably better to use the excel api:
https://docs.microsoft.com/en-us/graph/api/resources/excel?view=graph-rest-1.0

eg: https://docs.microsoft.com/en-us/graph/api/worksheet-get?view=graph-rest-1.0&tabs=http#example

I tried, but ran into this error: https://stackoverflow.com/questions/50216300/could-not-obtain-wac-access-token

but here it works:
https://github.com/adobe/helix-theblog-scanner/blob/master/src/handlers/ExcelHandler.js

src/matchers/excel.js

test/excel.test.js

package.json

tripodsan · 2020-03-26T01:15:42Z

test/excel.test.js

+    const result = await main({
+      __ow_path: '/https://adobe.sharepoint.com/sites/TheBlog/_layouts/15/guestaccess.aspx',
+      share: 'ESR1N29Z7HpCh1Zfs_0YS_gB4gVSuKyWRut-kNcHVSvkew',
+      email: 'helix%40adobe.com',


what is email used for?

It's not needed, but part of the share link as you copy it from the UI.

as I said, better provide the entire sharelink as param.

I don't get it. The entire share link is https://adobe.sharepoint.com/sites/TheBlog/_layouts/15/guestaccess.aspx?share=ESR1N29Z7HpCh1Zfs_0YS_gB4gVSuKyWRut-kNcHVSvkew&email=helix%40adobe.com&e=rcfCT3

This gets appended to the service URL, so that the path is /https://adobe.sharepoint.com/sites/TheBlog/_layouts/15/guestaccess.aspx and the URL parameters are passed as individual parameters.

If I pass the remaining parameters as __ow_query="share=ESR1N29Z7HpCh1Zfs_0YS_gB4gVSuKyWRut-kNcHVSvkew&email=helix%40adobe.com&e=rcfCT3" I'm not gaining anything, because I now have to parse the query string – something that only occurs in testing, never in production.

my problem is, that your sharelink argument is distributed as path and query params. this is inconsistent and pollutes the action params. I would do:

...data-embed@v1?share=https://adobe.sharepoint.com/sites/TheBlog/_layouts/15/guestaccess.aspx%21share=ESR1N29Z7HpCh1Zfs_0YS_gB4gVSuKyWRut-kNcHVSvkew%22email=helix%40adobe.com%22e=rcfCT

I'd rather keep it the way it is, so that helix-embed and helix-data-embed have the same API (just append the full URL)

I don' think this is good. if the url would be url-encoded, i would be marginally better.
you already see from your code, that you need to reconstruct the share link based on the path and some params...and you always need to rely on the exact format of the link.

what if MS or google decides to add a new query param to the sharelink?

test/excel.test.js

Co-Authored-By: Tobias Bocanegra <tripodsan@users.noreply.github.com>

github-actions · 2020-03-26T07:08:56Z

This PR will trigger a minor release when merged.

github-actions · 2020-03-26T07:09:04Z

This PR will trigger a minor release when merged.

Co-Authored-By: Tobias Bocanegra <tripodsan@users.noreply.github.com>

github-actions · 2020-03-26T07:16:17Z

This PR will trigger a minor release when merged.

trieloff · 2020-03-26T07:35:08Z

@tripodsan I'll add Polly and suggest we merge then, as we can iterate on the minor points later.

…o excel

github-actions · 2020-03-26T09:38:01Z

This PR will trigger a minor release when merged.

codecov · 2020-03-26T09:39:24Z

Codecov Report

Merging #6 into master will not change coverage by %.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master        #6   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         4    +1     
  Lines           40        56   +16     
=========================================
+ Hits            40        56   +16

Impacted Files	Coverage Δ
src/embed.js	`100.00% <100.00%> (ø)`
src/matchers/excel.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bec2715...83ee1cb. Read the comment docs.

tripodsan · 2020-03-26T09:41:21Z

@tripodsan I'll add Polly and suggest we merge then, as we can iterate on the minor points later.

I would really try to use the excel API....
and pass the share link as param

those are not minor points....

tripodsan

don't abuse __ow_path as document link
use excel graph api

trieloff · 2020-03-26T09:47:29Z

don't abuse __ow_path as document link

I don't think it's abuse and I don't want to break compatibility with helix-pipeline or helix-embed. In fact, the question whether to use __ow_path or not is completely orthogonal to this PR.

use excel graph API

I really don't see the benefit of using an API that has proven to be brittle when we have a working alternative. In addition, using the graph API would require more extensive mocking that is just adding dead testing code instead of verifying the functionality.

github-actions · 2020-03-26T09:55:13Z

This PR will trigger a minor release when merged.

tripodsan · 2020-03-26T09:55:25Z

I don't think it's abuse and I don't want to break compatibility with helix-pipeline or helix-embed. In fact, the question whether to use __ow_path or not is completely orthogonal to this PR.

then, I suggest to do it better here and then fix the helix-embed model.
as you see here:

https://github.com/adobe/helix-cli/blob/706a80fb30d16cedb81944cab8eac0f801b4ecf6/test/fixtures/recordings/Integration-test-for-up-command_3614339512/up-command-delivers-correct-response-_668345128/recording.har#L40

it causes already problems. you have to admit, that using the path doesn't add benefits and only complicates the parsing.

tripodsan · 2020-03-26T09:56:46Z

API that has proven to be brittle

why brittle? donwloading the file has drackbacks

if the excel file is large, it is slow to download
adding the extra dependency to parse the xls adds overhead

github-actions · 2020-03-26T09:59:42Z

This PR will trigger a minor release when merged.

trieloff · 2020-03-26T10:00:52Z

The advantage of the __ow_path model is that you can simply concatenate the URLs. The occasional lack of a / does not negate that.

github-actions · 2020-03-26T10:04:36Z

This PR will trigger a minor release when merged.

github-actions · 2020-03-26T10:10:29Z

This PR will trigger a minor release when merged.

trieloff · 2020-03-26T10:12:07Z

By the way, I find your insistence on using both the Excel Graph API and then mocking it until it doesn't expose the real-world behavior (and issues) anymore quite hard to reconcile.

I would (grudgingly) try to port it to the Graph API, but not when it means that the tests that I have are completely useless during development and won't help me troubeshoot issues with the underlying APIs.

github-actions · 2020-03-26T10:14:53Z

This PR will trigger a minor release when merged.

see #6 (review)

tripodsan · 2020-03-27T01:15:19Z

By the way, I find your insistence on using both the Excel Graph API and then mocking it until it doesn't expose the real-world behavior (and issues) anymore quite hard to reconcile.

IMO, the unit tests should work w/o external infrastructure, especially when it relies on resources on personal shares or repositories, or requires authentication with such.

I agree, that mocking the API bears its dangers of not catching changes in that API. the mocking the API tests your code; integration tests test their code.

I would (grudgingly) try to port it to the Graph API, but not when it means that the tests that I have are completely useless during development and won't help me troubeshoot issues with the underlying APIs.

I agree. So maybe the pollyjs approach is the good inbetween solution? you can disable playback during development and record it then for faster testing?

sidenote:
Maybe it's not so relevant for this project - but for larger ones, like helix-cli or helix-pipeline, working remote with a slow (or none) internet connection is very difficult.

feat(excel): support excel spreadsheets on sharepoint sites

9c02828

tripodsan reviewed Mar 26, 2020

View reviewed changes

trieloff and others added 2 commits March 26, 2020 08:08

chore(deps): pin dependency version

61fbc8e

Co-Authored-By: Tobias Bocanegra <tripodsan@users.noreply.github.com>

chore(deps): pin dependency version

16a6f97

Co-Authored-By: Tobias Bocanegra <tripodsan@users.noreply.github.com>

chore(deps): pin dependency versions

2f3f1ab

Co-Authored-By: Tobias Bocanegra <tripodsan@users.noreply.github.com>

trieloff added 4 commits March 26, 2020 08:05

build(deps): add fs-extra

21ca3ce

Merge branch 'excel' of https://github.com/adobe/helix-data-embed int…

922ae1a

…o excel

build(deps): pin dependency versions

89078e0

test(excel): run unit tests offline

cbedd91

trieloff marked this pull request as ready for review March 26, 2020 09:37

tripodsan suggested changes Mar 26, 2020

View reviewed changes

test(excel): add post-deploy test

0cca7e5

test(embed): fix array in post-deploy test

b639e87

test(excel): another post-deploy test tweak

bcea748

test(excel): use different URL for post-deploy test

91f9ae1

test(excel): use deep matching in post-deploy tests

83ee1cb

trieloff added a commit that referenced this pull request Mar 26, 2020

refactor(excel): use Excel Graph API instead of downloading Excel

26d947d

see #6 (review)

trieloff mentioned this pull request Mar 26, 2020

Excel Support (Using Graph API) #10

Merged

trieloff closed this Mar 26, 2020

This was referenced Mar 27, 2020

escape or don't use __ow_path to pass external resource url #11

Closed

escape or don't use __ow_path to pass external resource url adobe/helix-embed#187

Closed

tripodsan deleted the excel branch July 20, 2020 05:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(excel): support excel spreadsheets on sharepoint sites #6

feat(excel): support excel spreadsheets on sharepoint sites #6

trieloff commented Mar 25, 2020

tripodsan left a comment

tripodsan Mar 26, 2020

trieloff Mar 26, 2020

tripodsan Mar 26, 2020

tripodsan Mar 26, 2020

trieloff Mar 26, 2020

tripodsan Mar 26, 2020

trieloff Mar 26, 2020

tripodsan Mar 26, 2020 •

edited

trieloff Mar 26, 2020

tripodsan Mar 26, 2020

github-actions bot commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

trieloff commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

codecov bot commented Mar 26, 2020 •

edited

tripodsan commented Mar 26, 2020

tripodsan left a comment

trieloff commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

tripodsan commented Mar 26, 2020

tripodsan commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

trieloff commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

trieloff commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

tripodsan commented Mar 27, 2020

feat(excel): support excel spreadsheets on sharepoint sites #6

feat(excel): support excel spreadsheets on sharepoint sites #6

Conversation

trieloff commented Mar 25, 2020

Related Issues

tripodsan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tripodsan Mar 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

trieloff commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

codecov bot commented Mar 26, 2020 • edited

Codecov Report

tripodsan commented Mar 26, 2020

tripodsan left a comment

Choose a reason for hiding this comment

trieloff commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

tripodsan commented Mar 26, 2020

tripodsan commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

trieloff commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

trieloff commented Mar 26, 2020

github-actions bot commented Mar 26, 2020

tripodsan commented Mar 27, 2020

tripodsan Mar 26, 2020 •

edited

codecov bot commented Mar 26, 2020 •

edited