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.

Sign up for GitHub

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

Jump to bottom

API-16: Support for FS style upload workflow #34

Merged
merged 12 commits into from Dec 29, 2020
Merged

API-16: Support for FS style upload workflow #34

merged 12 commits into from Dec 29, 2020

Conversation

Arpit-Sharma-USC
Copy link
Contributor

  • logic to support new FS api based file upload
  • prune amazon s3 logic

JIRA

@Arpit-Sharma-USC
new FS upload
e973963 
- new fs upload workflow
- remove old aws s3 dependencies
- tests
@Arpit-Sharma-USC
tests, docs, readme fix
9d4ce2a
@Arpit-Sharma-USC Arpit-Sharma-USC marked this pull request as draft December 10, 2020 16:44
@Arpit-Sharma-USC Arpit-Sharma-USC marked this pull request as ready for review December 14, 2020 12:55
githuib
githuib reviewed Dec 14, 2020
View reviewed changes
src/Bynder/Api/Impl/AbstractRequestHandler.php Outdated Show resolved Hide resolved
src/Bynder/Api/Impl/AbstractRequestHandler.php Outdated Show resolved Hide resolved
src/Bynder/Api/Impl/AbstractRequestHandler.php Outdated Show resolved Hide resolved
src/Bynder/Api/Impl/OAuth2/RequestHandler.php Outdated Show resolved Hide resolved
src/Bynder/Api/Impl/PermanentTokens/RequestHandler.php Outdated Show resolved Hide resolved
src/Bynder/Api/Impl/PermanentTokens/RequestHandler.php Outdated Show resolved Hide resolved
src/Bynder/Api/Impl/OAuth2/RequestHandler.php Outdated Show resolved Hide resolved
@Arpit-Sharma-USC
suggested changes and formatting
3b1cc71 
-get ride of state variables
- cleanup RequestHandler
- format tests
githuib
githuib reviewed Dec 23, 2020
View reviewed changes
src/Bynder/Api/Impl/AbstractRequestHandler.php Outdated Show resolved Hide resolved
src/Bynder/Api/Impl/AbstractRequestHandler.php Outdated Show resolved Hide resolved
src/Bynder/Api/Impl/OAuth2/RequestHandler.php Outdated Show resolved Hide resolved
@Arpit-Sharma-USC
suggested changes- part 1
9e3e896 
- remove fileID and correlationId return from uploadFile
- clean up doc comments
- clean up fileSha256 usage
-revert AbstractRequestHandler (no raw response needed anymore)
- add brandId non-null check
- test for invalid brandId
@Arpit-Sharma-USC
suggested changes - part 2
fb6105d 
- clean the header updation a little
@Arpit-Sharma-USC
resolve merge conflicts
8f946c2
githuib
githuib reviewed Dec 28, 2020
View reviewed changes
Copy link

@githuib githuib left a comment

Choose a reason for hiding this comment

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

Nice work! Have some comments still mainly regarding the save media calls.

src/Bynder/Api/Impl/Upload/FileUploader.php Outdated Show resolved Hide resolved
tests/AssetBank/FileUploaderTest.php Show resolved Hide resolved
tests/AssetBank/FileUploaderTest.php Outdated Show resolved Hide resolved
tests/AssetBank/FileUploaderTest.php Outdated Show resolved Hide resolved
src/Bynder/Api/Impl/Upload/FileUploader.php Outdated Show resolved Hide resolved
@Arpit-Sharma-USC
suggested changes - 1
2468979 
- change filePath from a request param to a method param
- recycle code in tests
- fix saveMediaAsync for mediaId and brandId
- fix tests per the changes
- update sample and readme with new uploadFileAsync usage
@Arpit-Sharma-USC
comments to save media for understanding
07719e7
@Arpit-Sharma-USC
Merge branch 'master' into API-16
089d338
githuib
githuib reviewed Dec 29, 2020
View reviewed changes
Copy link

@githuib githuib left a comment

Choose a reason for hiding this comment

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

Looks good! Just some comments on things that can or should be left out.

src/Bynder/Api/Impl/Upload/FileUploader.php Outdated Show resolved Hide resolved
src/Bynder/Api/Impl/Upload/FileUploader.php Outdated Show resolved Hide resolved
@Arpit-Sharma-USC
Copy link
Contributor Author

Looks good! Just some comments on things that can or should be left out.

Awesome thanks for the review 😄

@Arpit-Sharma-USC
suggested changes - 2
a1e3321 
- prune upload intent for sdks
- remove server-ignored form_params
@Arpit-Sharma-USC Arpit-Sharma-USC changed the base branch from master to FS_upload_dev December 29, 2020 10:44
@Arpit-Sharma-USC Arpit-Sharma-USC merged commit cc24f66 into FS_upload_dev Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@githuib githuib githuib approved these changes

@betacar betacar Awaiting requested review from betacar

@WouterToering WouterToering Awaiting requested review from WouterToering

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Arpit-Sharma-USC @WouterToering @githuib