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

Add more lines for blogs and rss feeds. Fixes: 1409 #1410

Merged
merged 13 commits into from
Jun 21, 2024

Conversation

phsmoura
Copy link
Contributor

Just changing here at noggin seems free-ipa and fasjson doesnt need to be updated, but let me know if it does or if this PR needs adjustments

related issue: #1409

@abompard
Copy link
Member

abompard commented Jun 6, 2024

OK I made quite a few commits, one of them is adjusting to the fact that website_url is not set as multi-valued in FAS (see fedora-infra/freeipa-fas#151). You may want to fix this one before going through with this one.

@nirik
Copy link
Member

nirik commented Jun 6, 2024

Would it be possible to enforce https here too?

@phsmoura
Copy link
Contributor Author

phsmoura commented Jun 6, 2024

Would it be possible to enforce https here too?

That would help users because planet drops HTTP links.
@abompard : Do you know how we could block HTTP inputs?

@abompard
Copy link
Member

abompard commented Jun 6, 2024

Sure, WTForms supports validators, there may be one for HTTPS urls in the URLFields, and if not there's always the regex validator

@phsmoura
Copy link
Contributor Author

phsmoura commented Jun 7, 2024

Sure, WTForms supports validators, there may be one for HTTPS urls in the URLFields, and if not there's always the regex validator

Seems WTForms doesnt have a validator to check HTTPS, so I created a new class for it and wrote a few unit tests.
I wasnt able to see it working tho, had trouble with the VMs using vagrant. Will try again on Monday.

Let me know if it needs more changes, maybe this PR is still a WIP

@phsmoura
Copy link
Contributor Author

@abompard / @nirik : What if we keep this issue to make these fields multivalued and open a new one requesting to enforce HTTPS in these fields? That way, we could keep the tasks within the scope of the issue.

@phsmoura phsmoura marked this pull request as draft June 14, 2024 21:34
@phsmoura
Copy link
Contributor Author

It's now blocking any link that doesn't start with HTTPS, but I'm still having trouble adding more lines to the website and RSS fields. I'll keep trying to do this, but any help is very appreciated.

@phsmoura
Copy link
Contributor Author

It works now. Its possible to add multiple lines for Blog and RSS fields and it only allows HTTPS, but still need to add unit tests for https_required. Ill do it tomorrow

@abompard abompard force-pushed the dev branch 2 times, most recently from 1a222f2 to 1095b59 Compare June 18, 2024 12:31
@phsmoura phsmoura marked this pull request as ready for review June 18, 2024 21:13
@@ -80,6 +81,10 @@ def _validate(form, field):


class UserSettingsProfileForm(BaseForm):
def _https_required(form, field):
Copy link
Member

Choose a reason for hiding this comment

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

Why make it a member of the class? This construct is a bit unusual, it should either be:

  1. outside the class, as before
  2. a method, but then it should have self as a first argument and called on the instance, which is not what we want for a validator
  3. a class method, but then it should be decorated with @classmethod and take cls as the first argument
  4. a static method, but then it should be decorated with @staticmethod

I think keeping it outside (option 1) is best here, so it can possibly be reused by another form later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I thought it'd be used only for that case, but if it can possibly be reused I agree with option 1. Just did the changes.

Copy link
Member

@abompard abompard left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@abompard abompard merged commit 1c3abbb into fedora-infra:dev Jun 21, 2024
16 of 18 checks passed
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.

3 participants