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

added upload and download aliases in csv format. #1813

Merged
merged 4 commits into from
Sep 30, 2024
Merged

Conversation

Faakhir30
Copy link
Contributor

@Faakhir30 Faakhir30 commented Sep 17, 2024

Refers #1812

Closes #1793

Using content negotiation for PUT, GET aliases service.

  • send the "Accept" HTTP header "text/csv" for get request to download aliases as csv. Then REST API would return the CSV file.
  • send the "Content-Type" HTTP header "text/csv" for post request to add aliases from csv.

Tasks breakdown:

  • Handle text/csv content-type to upload aliases in bulk.
  • Handle text/csv content-type to download aliases.
  • Add new http-examples
  • Add aliases unit/integration tests to plone.restapi.
  • Add/update documentation

📚 Documentation preview 📚: https://plonerestapi--1813.org.readthedocs.build/

@mister-roboto
Copy link

@Faakhir30 thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

@Faakhir30
Copy link
Contributor Author

@jenkins-plone-org please run jobs

@stevepiercy
Copy link
Contributor

Hmm... the RTD preview build is still broken. I'll fiddle with its settings.

@Faakhir30
Copy link
Contributor Author

@jenkins-plone-org please run jobs

@stevepiercy
Copy link
Contributor

RTD preview looks good.

https://plonerestapi--1813.org.readthedocs.build/en/1813/endpoints/aliases.html#adding-url-aliases-in-bulk-via-csv

This is ready for technical review.

I think the Jenkins failures are flaky tests. I started a retry for the two failures.

Copy link
Contributor

@Petchesi-Iulian Petchesi-Iulian left a comment

Choose a reason for hiding this comment

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

LGTM codewise, looks like some test is failing

@Faakhir30
Copy link
Contributor Author

Faakhir30 commented Sep 29, 2024

The test failing plone.app.contenttypes.tests.test_robot.RobotTestCase.Scenario Test Relative Location Criterion
It still looks unstable/flaky to me, read from plone/plone.app.contenttypes#362.

Also, I'm unable to understand its relation with this aliases service.
Maybe someone with experience with Robot Tests @mauritsvanrees should chime in. @stevepiercy
I am facing similar timeout failures on Robot Tests in my other PRs also, see #1818 (comment)

@stevepiercy
Copy link
Contributor

Previously two Jenkins builds failed, now it's down to one. I started one more retry.

https://jenkins.plone.org/job/pull-request-6.1-3.12/639/

Those failures are totally unrelated to this PR, so I'm OK with ignoring them.

@Faakhir30 Faakhir30 merged commit a653349 into main Sep 30, 2024
25 checks passed
@Faakhir30 Faakhir30 deleted the allias_by_csv branch September 30, 2024 12:47
Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

@Faakhir30 In general I'd like to have a chance to look at new features before they are merged, and I hadn't had a chance to look at this one yet. But it looks like good work. Thank you!

src/plone/restapi/services/aliases/add.py Show resolved Hide resolved
src/plone/restapi/services/aliases/add.py Show resolved Hide resolved
@Faakhir30 Faakhir30 restored the allias_by_csv branch September 30, 2024 16:22
@Faakhir30
Copy link
Contributor Author

@davisagli understood, will make sure of your review on feature PRs from now on :)
Will create a patch PR for these changes Requested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants