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

Clear up settings #25

Merged
merged 3 commits into from
Mar 21, 2022
Merged

Clear up settings #25

merged 3 commits into from
Mar 21, 2022

Conversation

wesleybl
Copy link
Member

@wesleybl wesleybl commented Dec 9, 2021

Remove old stuff from product settings.

@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2021

Codecov Report

Merging #25 (5ab973f) into master (cfe5072) will decrease coverage by 6.60%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage   66.37%   59.77%   -6.61%     
==========================================
  Files           4        4              
  Lines         113       87      -26     
==========================================
- Hits           75       52      -23     
+ Misses         38       35       -3     
Impacted Files Coverage Δ
src/collective/recaptcha/settings.py 75.86% <100.00%> (-5.96%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfe5072...5ab973f. Read the comment docs.

Copy link
Member

@ale-rt ale-rt left a comment

Choose a reason for hiding this comment

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

This looks really good :)
Thanks!
It would be great to have an upgrade step that:

  • creates the proper registry records if missing
  • moves the configuration from the annotations to the registry if the registry is unconfigured
  • removes the old annotations

Also that TRY_REGISTRY should be cleaned up: the package is declared to work only on Plone 5.2, where the registry is always present.

@ale-rt ale-rt mentioned this pull request Dec 13, 2021
@wesleybl
Copy link
Member Author

Also that TRY_REGISTRY should be cleaned up: the package is declared to work only on Plone 5.2, where the registry is always present.

This looks to me like an integration with the plone.formwidget.recaptcha product. To see:

from plone.formwidget.recaptcha.interfaces import IReCaptchaSettings

I wanted to keep this integration. Perhaps the name of the constant should be changed?

@wesleybl
Copy link
Member Author

  • moves the configuration from the annotations to the registry if the registry is unconfigured
  • removes the old annotations

I delete the class from the annotation:

https://github.com/collective/collective.recaptcha/pull/25/files#diff-1796e3d755aa16dca04040d049b1f8cd0724527d2ddfeb4326967798a7da212fL44-L52

How to access data in this situation?

@wesleybl
Copy link
Member Author

@ale-rt any tips here? At some point we will break this. Maybe we'll implement an upgrade that no one will use. These annotations seem to be quite old.

@ale-rt
Copy link
Member

ale-rt commented Feb 25, 2022

@wesleybl
Copy link
Member Author

@ale-rt Well, inserting an alias is one more thing we will have to remove in the future. So I prefer to restore the class.

But imagine the situation:

  • we released release 3.0.0 with class and with step upgrade.
  • then we release release 4.0.0 without the class.
  • whoever used the class and upgraded from a version < 3.0.0 to 4.0.0 will have the problem.
  • most likely, it will migrate keys "by hand", before discovering it had an upgrade step in version 3.0.0.

Do you still think it's worth the upgrade step?

Copy link
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

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

This package reads the settings from the registry since PR #2, which is in version 1.1.5 from 2014. Ideally, an upgrade step would have been written at that time.
I am very much in favour of having a proper upgrade path, but it seems too late to write one now.

@mauritsvanrees
Copy link
Member

About TRY_REGISTRY:

I wanted to keep this integration. Perhaps the name of the constant should be changed?

This was introduced in commit 826c7e7. Its intention was for integration with plone.formwidget.recaptcha. The name of the constant is strange now. I suggest renaming it to TRY_FORMWIDGET.

@wesleybl
Copy link
Member Author

The name of the constant is strange now. I suggest renaming it to TRY_FORMWIDGET.

Done: e4ecd8e

@mauritsvanrees
Copy link
Member

black fails. Is probably fixed when you merge master into this branch, or rebase.

This appears to be a pre-registry thing. The configuration is now read
only from the registry.
This constant is used to check if we can use the
plone.formwidget.recaptcha settings, if they are present. So it makes
more sense to be TRY_FORMWIDGET
@wesleybl
Copy link
Member Author

@mauritsvanrees I did the rebase.

@mauritsvanrees
Copy link
Member

@ale-rt Are you okay with merging now, without upgrade step?
See also my comment about it.

Copy link
Member

@ale-rt ale-rt left a comment

Choose a reason for hiding this comment

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

LGTM

@mauritsvanrees mauritsvanrees merged commit 684f0b8 into master Mar 21, 2022
@mauritsvanrees mauritsvanrees deleted the settings branch March 21, 2022 11:21
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

5 participants