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

Undo ControlPanel: Creates an @transactions endpoint by fetching transactions from the database #1455

Merged
merged 70 commits into from
Sep 28, 2022

Conversation

MdSahil-oss
Copy link
Contributor

No description provided.

@mister-roboto
Copy link

@MdSahil-oss 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!

@netlify
Copy link

netlify bot commented Jul 4, 2022

Deploy Preview for plone-restapi ready!

Name Link
🔨 Latest commit 3d0be21
🔍 Latest deploy log https://app.netlify.com/sites/plone-restapi/deploys/6333d5fa76d694000b199e13
😎 Deploy Preview https://deploy-preview-1455--plone-restapi.netlify.app/endpoints/transactions.html
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@jensens
Copy link
Sponsor Member

jensens commented Jul 4, 2022

Was this tested with a history free RelStorage? (In this case the whole endpoint is pointless)

Copy link
Member

@robgietema robgietema left a comment

Choose a reason for hiding this comment

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

What is missing are the tests for the calls. I also added some minor change requests, mostly naming and language.

docs/source/transactions.md Outdated Show resolved Hide resolved
docs/source/transactions.md Outdated Show resolved Hide resolved
docs/source/transactions.md Outdated Show resolved Hide resolved
docs/source/transactions.md Outdated Show resolved Hide resolved
@MdSahil-oss
Copy link
Contributor Author

@jensens Yes! This was tested for the RelStorage.
Once the database is created a few transactions are stored in RelStorage like initial Database creation, Created initial User, etc so I think there is no Error message is generated by the Database for Empty Transactions on calling @Transactions endpoint because transactions Array in not null ever.

Here I'm showing A list of few transactions that automatically created by Zope Once database is created.

Screenshot_from_2022-08-10_16-43-12

@jensens
Copy link
Sponsor Member

jensens commented Sep 14, 2022

This is an excerpt from my working configuration using a Postgresql:

...

[instance]
eggs =
    Plone
    RelStorage[postgresql]

rel-storage =
    type postgresql
    dsn dbname='${db:name}' host='${db:host}' user='${db:user}' password='${db:password}'
    keep-history false
    shared-blob-dir true
    blob-dir ${db:blob-dir}
    
[db]
name = plone-test
host = 127.0.0.1
blob-dir = ${buildout:directory}/var/blobstorage
user = zope
password = secret

...

@mauritsvanrees
Copy link
Sponsor Member

We just had a Plone 6 coordination meeting. I heard that the new Undo control panel may be included in the Volto version that comes with Plone 6.0 final.
With my Plone Release Manager hat on that makes me nervous, introducing a new feature while we are in beta phase. Biggest worry for me is if this needs changes in all kinds of backend packages. It looks like this is only in plone.restapi, and no changes in other packages. Is that correct? Then it should be fine. The changes in this package are quite small.
Can someone confirm that for backend it is only this package?

@MdSahil-oss
Copy link
Contributor Author

We just had a Plone 6 coordination meeting. I heard that the new Undo control panel may be included in the Volto version that comes with Plone 6.0 final. With my Plone Release Manager hat on that makes me nervous, introducing a new feature while we are in beta phase. Biggest worry for me is if this needs changes in all kinds of backend packages. It looks like this is only in plone.restapi, and no changes in other packages. Is that correct? Then it should be fine. The changes in this package are quite small. Can someone confirm that for backend it is only this package?

@mauritsvanrees Yes! this is the only backend repository which is having changes in order to implement Undo Controlpanel and the rest of the changes is present in plone's frontend volto repository.

@sneridagh
Copy link
Member

LGTM!
@robgietema @MdSahil-oss Is this ready to merge?

@mauritsvanrees are you ok then with having this for Plone 6?

Maybe @stevepiercy wants to take a look at the docs first.

Thanks to everybody who helped/reviewed this!

@MdSahil-oss
Copy link
Contributor Author

@sneridagh Yes! This is ready to merge.

@stevepiercy
Copy link
Contributor

I am reviewing the docs now. Please give me the opportunity to complete that.

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.

We reorganized the docs after this PR was started. Sorry about that. Please let me know if you need further direction with that, and let's check the rendering of the docs in Netlify, too.

CHANGES.rst Outdated Show resolved Hide resolved
docs/source/transactions.md Outdated Show resolved Hide resolved
docs/source/transactions.md Outdated Show resolved Hide resolved
docs/source/transactions.md Outdated Show resolved Hide resolved
docs/source/transactions.md Outdated Show resolved Hide resolved
docs/source/transactions.md Outdated Show resolved Hide resolved
docs/source/transactions.md Outdated Show resolved Hide resolved
docs/source/transactions.md Outdated Show resolved Hide resolved
@mauritsvanrees
Copy link
Sponsor Member

@mauritsvanrees are you ok then with having this for Plone 6?

Yes, fine to have this extra endpoint in Plone 6.
@MdSahil-oss Thanks for creating this!

@sneridagh
Copy link
Member

@MdSahil-oss could you please take care of @stevepiercy comments? Then let's make it happen. Thanks!

@MdSahil-oss
Copy link
Contributor Author

@MdSahil-oss could you please take care of @stevepiercy comments? Then let's make it happen. Thanks!

@sneridagh I will be doing it soon.

MdSahil-oss and others added 9 commits September 27, 2022 17:07
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
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.

One more that I missed on my first pass, then this can be merged!

docs/source/endpoints/transactions.md Outdated Show resolved Hide resolved
Co-authored-by: Steve Piercy <web@stevepiercy.com>
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.

Thank you!

@sneridagh
Copy link
Member

@jenkins-plone-org please run jobs

@sneridagh sneridagh merged commit 8a17801 into master Sep 28, 2022
@sneridagh sneridagh deleted the undo-control-panel branch September 28, 2022 13:31
sneridagh added a commit that referenced this pull request Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants