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 extra fields for registration and fix exception when email address exists #148

Closed
wants to merge 25 commits into from

Conversation

Peterede
Copy link

I have added some extra fields to the registration form to collect more information about the user, these can be turned on/off in the admin settings. I also fixed an exception that occurs if the email address is already in the database and gets added twice.

Copy link
Collaborator

@pellaeon pellaeon left a comment

Choose a reason for hiding this comment

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

Please also add unit tests for PHP code as well. (currently this project has no front-end testing code setup)

<option value="SE">Sweden</option>
<option value="CH">Switzerland</option>
<option value="SY">Syrian Arab Republic</option>
<option value="TW">Taiwan, Province of China</option>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Taiwan is not a province of China.

service/registrationservice.php Show resolved Hide resolved
@pellaeon
Copy link
Collaborator

Also, please fix the broken existing tests because of your changes.

@Peterede
Copy link
Author

When I try and run the tests I get the following error:
~/work/registration$ vendor/phpunit/phpunit/phpunit -c tests/phpunit.xml
PHP Warning: require_once(/home/petere/work/registration/tests/../../../tests/bootstrap.php): failed to open stream: No such file or directory in /home/petere/work/registration/tests/autoloader.php on line 12
PHP Fatal error: require_once(): Failed opening required '/home/petere/work/registration/tests/../../../tests/bootstrap.php' (include_path='.:/usr/share/php') in /home/petere/work/registration/tests/autoloader.php on line 12

@Peterede
Copy link
Author

I have fixed another issue where the validation of the email fails because the user is already registered, the login link shows the html as text instead of a link to go to the login page.

@pellaeon
Copy link
Collaborator

pellaeon commented Sep 30, 2018

@Peterede Seems that you don't have a full Nextcloud instance for testing (currently it is required). In order to run the test, you need to run phpunit from the registration directory that resides in a working Nextcloud instance's apps/ directory.
The tests will modify the database but will revert any changes in the clean up phase, so I suggest you still use a test (non-production) Nextcloud instance.

@pellaeon
Copy link
Collaborator

Could you also fix the styles of the create account form so that it looks the same as before? I would prefer the fields are not separated.
Before:
2018-09-30 3 57 31

After:
2018-09-30 3 58 06
2018-09-30 3 58 35

  • Continuous input fields
  • Icons are shifted (the person and eye icon)
  • Add icons for new fields so that the design looks consistent

@Peterede
Copy link
Author

Peterede commented Oct 9, 2018

I have fixed the issues except for the icons on the new fields, I will work on this

@Peterede
Copy link
Author

All changes have been made now

@pellaeon
Copy link
Collaborator

pellaeon commented Oct 10, 2018

Sorry, but there are a few more problems that I found out:

  1. please use meaningful icons for each field, instead just putting contacts-dark.svg everywhere
  2. contacts-dark.svg doesn't exist in Nextcloud's master, this will cause error when rendering the form, please choose the icons that exist in both latest Nextcloud and ownCloud
  3. the timezone selection is empty on Nextcloud master
  4. please move the fields that have a title (country, language, timezone) to the bottom, because they cannot be combined with other fields. (keep all combined fields together at the top)
    2018-10-10 12 07 54

…ul icons to some fields and added moment-timezone-with-data.js
@Peterede
Copy link
Author

  1. For some fields there are no meaningful icons available, the nextcloud personal settings uses this icon for multiple fields so I used that as reference.
  2. contacts-dark.svg is in /core/img/places/contacts-dark.svg on nextcloud master
  3. I have added the moment-timezone-with-data.js file to the app
  4. I have moved the lists to the bottom so that the input fields are grouped

@pellaeon
Copy link
Collaborator

pellaeon commented Oct 10, 2018

  1. I'm sorry, but I can't find it there: https://github.com/nextcloud/server/tree/master/core/img/places
    Maybe it's related to theming, do you have theming enabled? I don't. There seems to be an issue: NC14: core/img/places/contacts-dark.svg missing server#10443

@pellaeon
Copy link
Collaborator

Also, moment-timezone-with-data.js is a 190KB file, could you make it load only when the timezone field is enabled?

@Peterede
Copy link
Author

In my checkout of master the file is located in core/img/places but if I download the source from github or view it in the web browser the file is not there. The latest commit shows the file was deleted nextcloud/server@29ff7ef
I replaced contacts-dark.svg with social.svg which is the same icon

controller/registercontroller.php Outdated Show resolved Hide resolved
@thoka
Copy link

thoka commented Jan 25, 2020

I would find this patch very usefull. What has to be done to merge it ?

@nickvergessen
Copy link
Member

I would find this patch very usefull. What has to be done to merge it ?

Due to the number of conflicts I guess it has to be redone

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.

4 participants