search

collective / collective.logbook

Advanced exception logging for Plone
5 stars 2 forks source link

When you say in 0.8 release "Initial port to Plone 5", does it mean you're dropping support for 4.x? #7

Closed idgserpro closed 7 years ago

idgserpro commented 7 years ago

Do you have plans of Plone 4 support? If not, shouldn't the version be bumped to 1.0?

idgserpro commented 7 years ago

For @glemignot and @tkimnguyen since you're the last ones commiting. :smiley:

glemignot commented 7 years ago

The way to hook with Plone Control Panel completely changed between Plone 4 and Plone 5, so having a version that works on both will be a bit tricky, it seems easier to me to have two branches, one for Plone 4 (only for major bug fixes) and one for Plone 5 where main dev is done.

But I'm a mere contributor, not the maintainer, so not my decision ;)

idgserpro commented 7 years ago

The way to hook with Plone Control Panel completely changed between Plone 4 and Plone 5, so having a version that works on both will be a bit tricky

No problem. So, < 0.8 is the Plone 4 branch, and > 0.8 the Plone 5 branch? If I need to create a 0.7.1 release to work in Plone 4, is it possible? Should I use https://github.com/collective/collective.logbook/tree/collective.logbook-0.7? Is https://github.com/collective/collective.logbook/tree/collective.logbook-0.8 still needed, or should be deleted and master is now Plone 5?

Who is the maintainer? In pypi it says @jfroche and @gotcha but I'm not sure.

ramonski commented 7 years ago

To reduce confusion, I would keep the master branch for Plone 5 and current changes.

So master would represent the current 0.8 branch, which can be safely removed (only the HISTORY file changed, but unimportant as I see).

@idgserpro: please send your Plone 4 changes via a PR to the 0.7 branch, which should be released as 0.7.1 on PyPI. If you prepared your changes with the Changlog & Version info, I can do the release if you like.

Thank you all for contribution.

idgserpro commented 7 years ago

If you prepared your changes with the Changlog & Version info, I can do the release if you like.

We still don't have the PR since we're discussing with a client if this package would be used or not, but we needed to know if Plone 4 support would still exist. Now that we know this is the case, we can suggest using it. Thanks for answering.

I don't know if you have time for this, but it would be nice to have the following change in documentation (check if you agree):

I think after this the issue can be closed :laughing:

idgserpro commented 7 years ago

The way to hook with Plone Control Panel completely changed between Plone 4 and Plone 5

@glemignot I decided to read more carefully this statement and I have a question to ask: is this the only reason to have two branches? I just tested master in latest Plone 4.3.11, and both the controlpanel and @@logbook view works with the test error (but the e-mail is not sent).

How to test:

git clone https://github.com/collective/collective.logbook.git
python bootstrap.py
bin/buildout

Configure e-mail in http://localhost:8080/Plone/@@mail-controlpanel, configure your settings in http://localhost:8080/Plone/@@logbook-controlpanel and simulate an error in http://localhost:8080/Plone/@@error-test: the e-mail is not sent but was correctly added to http://localhost:8080/Plone/@@logbook:

selecao_013

Shouldn't we just try to see why the e-mail wasn't sent, fix it and have just master for both Plone 4 and Plone 5?

ramonski commented 7 years ago

+1 to keep only master if possible.

glemignot commented 7 years ago

Ah, to be fair I didn't try to run the Plone 5 version on Plone 4. If it does work with minor fix, that would indeed be much better !

idgserpro commented 7 years ago

We can't do a PR, but we can give you some light about this problem. If you could fix we would appreciate!

You can have this package compatible in Plone 4 and Plone 5 adding and if here https://github.com/collective/collective.logbook/commit/6266e36893f0e76b099fedbbf41394198977ee73#diff-527befdbb56ab9788b61e26213ef32b0L87 checking for both Plone versions instead of removing the code. I recommend using plone.api for checking the Plone version.

In Plone 4, the email_from_name and email_from_address comes from portal_properties, and, in Plone 5, from portal_registry. Installing from master in Plone 4 your utils.py model tries to get from registry but it's in portal_properties, thus you get a 'None' and can't send the email to 'None'. So you need the if.

Some examples of different code condition when you have different Plone versions for reference:

https://github.com/collective/collective.cover/blob/16804ecbe1a3f7c197d5bc8f7885c7a9dd53e8b8/src/collective/cover/tests/test_robot.py#L31

https://github.com/collective/collective.cover/blob/16804ecbe1a3f7c197d5bc8f7885c7a9dd53e8b8/src/collective/cover/config.py#L31

We suggest as well:

This package isn't so complicated, it shouldn't have two different branches for different Plone versions. If collective.cover that is a HUGE plugin can do it, almost everyone can! :laughing:

ramonski commented 7 years ago

@idgserpro: Thanks for your comment, which is quite helpful. I just checked out the master branch and will see what I can do. You're right, the code isn't that complex to be not usable in Plone 4 and 5. Stay tuned, I'll do a PR soon for you all to review.

ramonski commented 7 years ago

Please check out my WIP branch for Plone 4/5 compatibility:

https://github.com/collective/collective.logbook/tree/v0.8

Didn't test it yet in Plone 4, so feedback is welcome.

ramonski commented 7 years ago

Ok, I'm quite satisfied with the result and merged now the changes into master, which is now version 0.8 and should run nicely under Plone 4 and Plone 5.

Thanks @idgserpro and @glemignot for your suggestions and support and of course, feel free to add PR's against the current master, as I will stop now working on collective.logbook until we consider to release this version.

Unfortunately I think your pending PR's are now obsolete and you have to reconsider if you need some of the changes in the new codebase – but the master branch should be now much more cleaner and easier to understand.

idgserpro commented 7 years ago

Did you add the package to travis CI? https://travis-ci.org/collective/collective.logbook I still couldn't test the package against 4.3!

ramonski commented 7 years ago

I prepared everything for Travis CI, but the repository itself has to be hooked up I guess (actually I forgot how to do it...). Could you support here?

Did you run the tests locally with the current codebase and ./bin/test?

idgserpro commented 7 years ago

IIRC, you can follow this tutorial here https://github.com/mbonaci/mbo-storm/wiki/Integrate-Travis-CI-with-your-GitHub-repo.

No, I didn't run bin/test since it's only for 5.0. Since you already created a travis infrastructure, I would like to know what would happen for Plone 4.3.

ramonski commented 7 years ago

Unfortunately if your repository is hosted on collective, you have to ask someone to activate it:) But I found out where again and opened a ticket: https://github.com/collective/collective.github.com/issues/951

ramonski commented 7 years ago

https://travis-ci.org/collective/collective.logbook all tests passing 👍

tkimnguyen commented 7 years ago

@idgserpro nice... I didn't need to get dragged into this when my commit was just to update the pypi classifier and tweak the docs :)