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.

Sign up for GitHub

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

Jump to bottom

Fix #2064: Check catcha url exists in signup form #2470

Merged
merged 3 commits into from
Jul 5, 2023
Merged

Fix #2064: Check catcha url exists in signup form #2470

merged 3 commits into from
Jul 5, 2023

Conversation

m4ra
Copy link
Contributor

@m4ra m4ra commented Jun 19, 2023

This PR fixes the error:

AttributeError at /accounts/signup/

'Settings' object has no attribute 'CAPTCHA_URL'

when no captcha is installed in the project and thus no CAPTCHA_URL in the settings.

#2064

@m4ra m4ra requested a review from fuzzylogic2000 June 19, 2023 12:51
@github-actions
Copy link

github-actions bot commented Jun 19, 2023 •
edited
Loading

Coverage report

Total coverage

Status Category Percentage Covered / Total
🔴 Statements 11.88% 84/707
🔴 Branches 13.61% 49/360
🔴 Functions 9.73% 25/257
🔴 Lines 23.15% 528/2281

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

Report generated by 🧪jest coverage report action from 08e1c4d

fuzzylogic2000
fuzzylogic2000 reviewed Jun 20, 2023
View reviewed changes
Copy link
Contributor

@fuzzylogic2000 fuzzylogic2000 left a comment

Choose a reason for hiding this comment

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

Nice, only one little thing. And maybe it would be nice if it also deleted the field when the setting is just an empty string instead of nonexistent?

apps/contrib/templates/a4_candy_contrib/includes/form_field.html Outdated Show resolved Hide resolved
fuzzylogic2000
fuzzylogic2000 reviewed Jun 23, 2023
View reviewed changes
Copy link
Contributor

@fuzzylogic2000 fuzzylogic2000 left a comment

Choose a reason for hiding this comment

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

@m4ra had two comments to my commit, which are true: db29abc
And I remembered the wagtail forms, where we also use the captcha. That should also be included before merge. (See the end of the landing page of the dev-server and https://github.com/liqd/adhocracy-plus/blob/main/apps/cms/contacts/models.py)
So, I will just leave it for now, you can finish it in July or with the next a+ sprint.

@m4ra
Copy link
Contributor Author

m4ra commented Jul 4, 2023

@Rineee @goapunk @philli-m changes in cms/contacts is added. captcha appears in the form only if exists in the settings.
image

Rineee
Rineee reviewed Jul 5, 2023
View reviewed changes
apps/cms/contacts/models.py Outdated

print("WagtailCaptchaFormBuilder FIELDS", list(fields))
Copy link
Contributor

Choose a reason for hiding this comment

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

this should go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooops, yes, i deleted it now.

@Rineee
Copy link
Contributor

Rineee commented Jul 5, 2023

Looking good! Only one print out left.

We just introduced a CHANGELOG procedure, once you rebase this branch onto main, you should find a changelog folder there with a README. The plan is to add a file to that folder with every PR/commit, so maybe you could already add one in this PR?

goapunk
goapunk reviewed Jul 5, 2023
View reviewed changes
apps/cms/contacts/models.py Outdated
),
)
# Add captcha to formfields property if the URL exists in settings
if settings.CAPTCHA_URL:
Copy link
Contributor

Choose a reason for hiding this comment

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

in the other file you use if not (hasattr(settings, "CAPTCHA_URL") and settings.CAPTCHA_URL): but here only settings.CAPTCHA_URL: is there a reason why we don't need hasattr here ? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is because here the field is added, and in the other form the field is deleted if setting not there. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I started working on this, there was no CAPTCHA_URL as default with an empty string, thus I did the check for the attribute. Katharina added that url as empty string in the base settings afterwards and while I was away. So technically we don't need the hasattr check any longer. But it can also be there as an extra safety. Let me know what you prefer and I will update either check to be the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mara ah, makes sense. I'm fine with both, so pick what you prefer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@goapunk @Rineee ok now they are uniformed, and I kept the extra hasattr as extra check in case a project would like to remove the setting. I rebased and added a changelog.

@Rineee
Copy link
Contributor

Rineee commented Jul 5, 2023 •
edited
Loading

Looking good! Only one print out left.

We just introduced a CHANGELOG procedure, once you rebase this branch onto main, you should find a changelog folder there with a README. The plan is to add a file to that folder with every PR/commit, so maybe you could already add one in this PR?

@mara Not sure if you saw this comment?

@Rineee Rineee self-requested a review July 5, 2023 15:58
Rineee
Rineee approved these changes Jul 5, 2023
View reviewed changes
Copy link
Contributor

@Rineee Rineee left a comment

Choose a reason for hiding this comment

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

Very nice!

@Rineee Rineee merged commit 3544ebd into main Jul 5, 2023
2 checks passed
@Rineee Rineee deleted the m4-fix-captcha-signup branch July 5, 2023 16:05
@m4ra m4ra mentioned this pull request Sep 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goapunk goapunk goapunk left review comments

@fuzzylogic2000 fuzzylogic2000 fuzzylogic2000 left review comments

@Rineee Rineee Rineee approved these changes

@philli-m philli-m Awaiting requested review from philli-m

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@m4ra @Rineee @goapunk @fuzzylogic2000