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 create and fetch aliases using csv. #1812

Closed
wants to merge 3 commits into from

Conversation

Faakhir30
Copy link
Contributor

@Faakhir30 Faakhir30 commented Sep 14, 2024

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
  • Deal filters in download functionality in controlpannel of Products.CMFPlone, filters not being applied yet.

📚 Documentation preview 📚: https://plonerestapi--1812.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 Faakhir30 force-pushed the allias_by_csv branch 3 times, most recently from 1bc3436 to f190308 Compare September 14, 2024 23:06
@Faakhir30
Copy link
Contributor Author

@jenkins-plone-org please run jobs

@stevepiercy
Copy link
Contributor

@Faakhir30 please do not run Jenkins until after all GitHub Workflow actions pass. See https://6.docs.plone.org/contributing/core/continuous-integration.html#run-jenkins.

For the failure, see the full error messages at https://github.com/plone/plone.restapi/actions/runs/10866166462/job/30153407901?pr=1812#step:11:14, ignoring the noise from the repetitive tput: No value for $TERM and no -T specified statements.

Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

I know this is in progress. You'll need to write tests and documentation. See https://6.docs.plone.org/plone.restapi/docs/source/contributing/index.html

src/plone/restapi/services/aliases/add.py Outdated Show resolved Hide resolved
src/plone/restapi/services/aliases/add.py Outdated Show resolved Hide resolved
src/plone/restapi/services/aliases/add.py Outdated Show resolved Hide resolved
news/1812.feature Outdated Show resolved Hide resolved
@Faakhir30 Faakhir30 force-pushed the allias_by_csv branch 3 times, most recently from d4f23a5 to f0bc4e5 Compare September 15, 2024 18:55
@Faakhir30
Copy link
Contributor Author

@jenkins-plone-org please run jobs

@stevepiercy
Copy link
Contributor

Jenkins fails with something I don't recognize. Can someone give an assist?

https://jenkins.plone.org/job/pull-request-6.0-3.8/3376/console

+ bin/rfbrowser init
/tmp/jenkins1642391313986560648.sh: line 29: bin/rfbrowser: No such file or directory
+ bin/test --all --xml . -t '!ONLYROBOT'
/tmp/jenkins1642391313986560648.sh: line 31: bin/test: No such file or directory
+ return_code=127
+ bin/test --all --xml . -t ONLYROBOT
/tmp/jenkins1642391313986560648.sh: line 33: bin/test: No such file or directory
+ return_code=127
+ '[' 127 = all_right ']'
+ python templates/pr-update-status-py3.py
+ exit 127
Build step 'Execute shell' marked build as failure
Recording test results
ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?
Robot results publisher started...
INFO: Checking test criticality is deprecated and will be dropped in a future release!
-Parsing output xml:
ERROR: Build step failed with exception

@Faakhir30
Copy link
Contributor Author

Jenkins fails with something I don't recognize. Can someone give an assist?

https://jenkins.plone.org/job/pull-request-6.0-3.8/3376/console

+ bin/rfbrowser init
/tmp/jenkins1642391313986560648.sh: line 29: bin/rfbrowser: No such file or directory
+ bin/test --all --xml . -t '!ONLYROBOT'
/tmp/jenkins1642391313986560648.sh: line 31: bin/test: No such file or directory
+ return_code=127
+ bin/test --all --xml . -t ONLYROBOT
/tmp/jenkins1642391313986560648.sh: line 33: bin/test: No such file or directory
+ return_code=127
+ '[' 127 = all_right ']'
+ python templates/pr-update-status-py3.py
+ exit 127
Build step 'Execute shell' marked build as failure
Recording test results
ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?
Robot results publisher started...
INFO: Checking test criticality is deprecated and will be dropped in a future release!
-Parsing output xml:
ERROR: Build step failed with exception

yeah, that's same issue I pointed out earlier in my other PRs, different fork name from upstream repo name issue. Ive changed my fork name to make it work for now, an issue to resolve this is open at plone/jenkins.plone.org#368.

@stevepiercy
Copy link
Contributor

For narrative documentation, I would suggest duplicating the following two sections.

Then modify each heading to be explicit, in other words, "Adding URL aliases in bulk via JSON" and "Adding URL aliases in bulk via CSV", and the same for Listing. Finally update each section's text and examples.

@Faakhir30
Copy link
Contributor Author

@jenkins-plone-org please run jobs

@stevepiercy
Copy link
Contributor

@Faakhir30 for now, you will need to create your PR in a branch in this repository, and we will need to grant trusted contributors access to the Plone Developers GitHub Team, which you currently have, in order for Jenkins to run tests.

I brought this issue up with the CI Team. Their response was not to allow untrusted members of the Contributors Team to execute arbitrary code on Jenkins.

@Faakhir30
Copy link
Contributor Author

@Faakhir30 for now, you will need to create your PR in a branch in this repository, and we will need to grant trusted contributors access to the Plone Developers GitHub Team, which you currently have, in order for Jenkins to run tests.

I brought this issue up with the CI Team. Their response was not to allow untrusted members of the Contributors Team to execute arbitrary code on Jenkins.

For now, as I've fixed it by renaming my fork, can I follow this "branch from same repo" technique from next PR ? please :)

@stevepiercy
Copy link
Contributor

It's up to the @plone/ci-team. One of them may respond. It doesn't make sense to run Jenkins when you know it will fail because of that issue. If renaming your fork works, then I guess that is OK ¯\_(ツ)_/¯.

@Faakhir30
Copy link
Contributor Author

@jenkins-plone-org please run jobs

@Faakhir30 Faakhir30 requested a review from davisagli September 17, 2024 09:05
@Faakhir30
Copy link
Contributor Author

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
  • Deal filters in download functionality in controlpannel of Products.CMFPlone; filters have not been applied yet.

@stevepiercy @davisagli I think this PR is already quite large right now, will add missing filters to CMFPlone, plone.restapi, tests, and documentation of aliases in a separate PR. It would be nice to have a review and merge this PR.

@stevepiercy
Copy link
Contributor

I just found another reason to use a branch in this repo instead of a fork. The pull request preview of docs does not work from external forks. I have to instead add a new git remote, pull down your branch, then build the docs. It's more work for reviewers.

@Faakhir30
Copy link
Contributor Author

I just found another reason to use a branch in this repo instead of a fork. The pull request preview of docs does not work from external forks. I have to instead add a new git remote, pull down your branch, then build the docs. It's more work for reviewers.

Sure, will create, no problem

@stevepiercy
Copy link
Contributor

Here's screenshots. I think it looks OK, but I'm not certain about the technical content.

Screenshot 2024-09-17 at 3 45 22 AM

Screenshot 2024-09-17 at 3 45 52 AM

@Faakhir30
Copy link
Contributor Author

PR continued at #1813

@Faakhir30 Faakhir30 closed this Sep 17, 2024
Copy link
Contributor

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Can you carry over these changes to the new PR, so I don't have to repeat my review there? Thank you!

docs/source/endpoints/aliases.md Show resolved Hide resolved
docs/source/endpoints/aliases.md Show resolved Hide resolved
docs/source/endpoints/aliases.md Show resolved Hide resolved
@Faakhir30
Copy link
Contributor Author

@stevepiercy sure, will do these changes in new PR, no need to repeat review

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