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

feat(api-v2): Add support for text file upload (DSP-44) #1664

Merged
merged 16 commits into from Sep 29, 2020

Conversation

@benjamingeer
Copy link
Collaborator

@benjamingeer benjamingeer commented Jun 29, 2020

https://dasch.myjetbrains.com/youtrack/issue/DSP-44

@benjamingeer benjamingeer self-assigned this Jun 30, 2020
@benjamingeer benjamingeer marked this pull request as draft Jun 30, 2020
@subotic subotic changed the title feat(api-v2): Add support for text file upload feat(api-v2): Add support for text file upload (DSP-44) Sep 23, 2020
- Refactor Sipi bug workaround.
@benjamingeer benjamingeer marked this pull request as ready for review Sep 28, 2020
@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Sep 28, 2020

Skipping ClientApiRouteE2ESpec because it keeps failing with this error, despite everything we've tried:

Failed to get test data: {"knora-api:error":"akka.pattern.AskTimeoutException: Ask timed out on
[Actor[akka://E2ETest/user/applicationManager#-1049986081]] after [5000 ms]. Message of type
[org.knora.webapi.messages.admin.responder.usersmessages.UserGetADM] was sent by
[Actor[akka://E2ETest/user/applicationManager#-1049986081]].
@benjamingeer benjamingeer requested a review from SepidehAlassi Sep 28, 2020
@subotic
Copy link
Collaborator

@subotic subotic commented Sep 28, 2020

@benjamingeer @SepidehAlassi could you please make sure to fix this test either in this PR or as the first next thing? If you decide to ignore it in this PR, please open an issue on YouTrack. This test is also failing on develop.

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Sep 29, 2020

@benjamingeer @SepidehAlassi could you please make sure to fix this test either in this PR or as the first next thing? If you decide to ignore it in this PR, please open an issue on YouTrack. This test is also failing on develop.

We're planning to redesign it in the next sprint, using your idea of collecting the test data as a side effect of running the E2E tests.

@subotic
Copy link
Collaborator

@subotic subotic commented Sep 29, 2020

cool, great 👍

Copy link
Contributor

@SepidehAlassi SepidehAlassi left a comment

Thanks for this, please see my comment regarding work around for DSP-711

test_data/test_route/files/spam.csv Outdated Show resolved Hide resolved
sipiResponseStr <- doSipiRequest(sipiRequest)
sipiResponse: SipiKnoraJsonResponse = sipiResponseStr.parseJson.convertTo[SipiKnoraJsonResponse]

// Workaround for https://dasch.myjetbrains.com/youtrack/issue/DSP-711

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Sep 29, 2020
Contributor

@benjamingeer it seems like Lukas has already fixed the issue of DSP-711, see his response should this workaround be removed in this PR or later?

Copy link
Contributor

@SepidehAlassi SepidehAlassi left a comment

@benjamingeer thanks for your work. As agreed, please make a new PR with the new version of SIPI.

@subotic
Copy link
Collaborator

@subotic subotic commented Sep 29, 2020

PR for updating Sipi: #1721

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Sep 29, 2020

@SepidehAlassi Thanks for reviewing!

@benjamingeer benjamingeer merged commit a88d20d into develop Sep 29, 2020
8 checks passed
8 checks passed
Build Everything
Details
API Unit Tests
Details
API E2E Tests
Details
API Integration Tests
Details
Upgrade Integration Tests
Details
Docs Build Test
Details
Update next release draft
Details
Publish (on release only)
Details
@benjamingeer benjamingeer deleted the wip/DSP-44-text-files branch Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants