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

fix profile double type and links bugs, tidy profile code #970

Merged
merged 7 commits into from
Jun 9, 2020

Conversation

chrismclarke
Copy link
Member

PR Type

  • Bug fix (non-breaking change which fixes an issue)

PR Checklist

  • - Latest master branch merged
  • [x - PR title descriptive (can be used in release notes)
  • - Passes Tests

Description

Git Issues

Closes #818
Closes #968

@chrismclarke chrismclarke requested a review from BenGamma June 2, 2020 02:34
@cypress
Copy link

cypress bot commented Jun 2, 2020



Test summary

60 0 0 0


Run details

Project onearmy-community-platform
Status Passed
Commit 798632e ℹ️
Started Jun 9, 2020 6:30 AM
Ended Jun 9, 2020 6:35 AM
Duration 04:34 💡
OS Linux Ubuntu Linux - 16.04
Browser Multiple

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Contributor

@BenGamma BenGamma left a comment

Choose a reason for hiding this comment

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

Yep that fix the issue with the links that doesn't have http:// protocol.
I was mentioning in my comment that we had a addProtocol mutator to add the protocol on the form validation when the user is saving his infos. But for some reason the mutator passed here is not going down to his children Link.field anymore.

So actually at the moment you can save whatever text you want from the links inputs in the user settings, there is no custom validation anymore (customOnBlur method not being called). Maybe we missed that during the previous refactoring of the settings form ?

I tried to add the customBlurMethod on the Link.field but I didn't manage to triggered the mutator from there, I'm probably missing something in the way react final form pass down props.
Do you think you can have a quick look at this before merging ?

@chrismclarke
Copy link
Member Author

Sure thing, can take a quick look. Yeah I had remembered there being a better URL check before, so will be good to get that back in alongside the fix for existing links

@chrismclarke
Copy link
Member Author

chrismclarke commented Jun 3, 2020

OK, yeah I see the problem. It was part of the refactor, specifically the way the mutator was used previously isn't really compatible with array mutator used for form links.

I've fixed it so that first the form now properly checks the validation methods (a state variable required wasn't being updated), and added a standalone format method to use directly in the input format function instead of form validators for the links. I also checked the only other place the protocol validator was used (events page) and that is still working fine without change.

So should be good to go assuming the latest commits passes tests. Will check back in tomorrow

@chrismclarke chrismclarke requested a review from BenGamma June 3, 2020 05:23
Copy link
Contributor

@BenGamma BenGamma left a comment

Choose a reason for hiding this comment

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

Cool thanks for the fix Chris. I added 3 small commits, nothing critical. See commit message for description.

I noticed a warning in the console when the input is empty :

A component is changing a controlled input of type undefined to be uncontrolled. Input elements should not switch from controlled to uncontrolled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component. More info: https://fb.me/react-controlled-components

I guess it's because we use a custom method to validate the field onBlur that is now in conflict with react-final-form validation. Since it's just a warning and we know why it appends, it's fine to me. I'll approve and feel free to merge :)

@chrismclarke
Copy link
Member Author

Great, thank for that. Yeah I jumped back and forth on whether directing to http or https site would be better, I still think it would be better to by default assume the site as https for a couple reasons:

  1. Not all sites are setup to redirect from http to https if they have certificates installed. E.g. visiting http://apache.org/ will give the insecure version of https://apache.org/ in most browsers

  2. More than half of the world runs on https and that includes all major social media outlets (likely where people will be linking to).

Let me know if there's another reason you had thought of though that I have missed, otherwise we can revert that one small change and merge.

Happy with the other additions though (I understand the warning given, ideally we should be make those form alterations at a higher level and passing through state but I tried and it was a bit messy and tricky to get working correctly...so I suggest just leaving for now).

@BenGamma
Copy link
Contributor

BenGamma commented Jun 8, 2020

I agree on your points but I removed it because :

  • Even if we consider https better & more secure we shouldn't change the user input (more than we already do).
  • For social media they will be redirected to https anyway but you can also add your own website which is more likely not to be under https (small associations and non technical ppl don't have the reflex to activate https on their hosting like we do)

@chrismclarke
Copy link
Member Author

Ah, I understand the confusion now - yes we definitely don't want to be changing things a user has put (especially if it can't be changed back by them), but the protocol change only happens if there is nothing there - i.e. if they want their HTTP website, they can still type http://... and it will stay the same.

Actually even for personal websites HTTP is super rare these days, as most people like to use 3rd party web providers and all the largest ones (e.g. heroku, google firebase, netlify, zeit etc.) all provide free https as a default - so it's only really for sites running on their own personal web servers. And again, this is still much less that people who will just be putting social media links. So I've reverted back to https and will merge

@chrismclarke chrismclarke merged commit cb0c541 into master Jun 9, 2020
@chrismclarke chrismclarke deleted the fix/818-profile-double-type branch June 9, 2020 06:29
@BenGamma BenGamma restored the fix/818-profile-double-type branch June 9, 2020 10:22
@BenGamma
Copy link
Contributor

BenGamma commented Jun 9, 2020

Well I REVERTED BACK TO HTTP (that's a joke) 😁

Sorry I'm reopening this branch because I've spotted a weird behavior in the link validation while doing a last check. See a screencast here
It happend in the case you have an invalid url first, then the validation removes the last char of the input. But I don't get it because we are not striping any character, we are just adding from the existing value 🤔

I've added a commit also to remove the url check on discord type, which are not links but discord id (mine for example is benj#2480)

I'm always surprised of the ability of this form errors to always end up in headbanging. But we're almost there 👊

@BenGamma BenGamma mentioned this pull request Jun 9, 2020
5 tasks
@BenGamma
Copy link
Contributor

BenGamma commented Jun 9, 2020

I moved the discussion to #979

@chrismclarke
Copy link
Member Author

hahahahaha! I wouldn't have been surprised. Whaat? that bug is super weird. Oh and good catch on the discord - I'm guessing we should actually link to the discord user page but that can be a job for another day if someone else views it as important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants