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

Allow registrations #394

Merged
merged 2 commits into from
Aug 4, 2021
Merged

Allow registrations #394

merged 2 commits into from
Aug 4, 2021

Conversation

p1gp1g
Copy link

@p1gp1g p1gp1g commented Feb 21, 2021

This commit allow registrations for users. It might be useful to add a config parameter to enable or disable it.

@p1gp1g p1gp1g requested a review from a team as a code owner February 21, 2021 23:18
jmattheis
jmattheis reviewed Feb 22, 2021
View reviewed changes
Copy link
Member

@jmattheis jmattheis left a comment

Choose a reason for hiding this comment

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

Hey, thanks for your contribution, could you first open an issue, to get some feedback from other users, if that is a needed feature? I rather not add stuff that is not really used even if it is behind a feature flag.

@p1gp1g p1gp1g mentioned this pull request Feb 22, 2021
Closed
jmattheis
jmattheis requested changes Jun 13, 2021
View reviewed changes
Copy link
Member

@jmattheis jmattheis left a comment

Choose a reason for hiding this comment

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

Hey, thanks for your contribution, remarks in subcomments.

config.example.yml Outdated Show resolved Hide resolved
api/user.go Outdated Show resolved Hide resolved
model/version.go Outdated Show resolved Hide resolved
ui/src/user/Login.tsx Outdated Show resolved Hide resolved
ui/src/user/Login.tsx Outdated Show resolved Hide resolved
@JuniorJPDJ
Copy link

bump?

@karmanyaahm
Copy link
Contributor

bump?

I think the commits from the sub PRs 2 and 3 fix all issues from the review? I should've explicitly mentioned that here after both sub-PRs were merged, sorry.

@jmattheis
Copy link
Member

jmattheis commented Aug 2, 2021 •
edited
Loading

@karmanyaahm I've rebased the branch (you may have to update your ui dependencies with yarn install) and added some fixes to this PR, could you review them and test that it works well? And can I squash our commits into one, this way @p1gp1g will be the author of all changes related to registration? This is for a clean git history as in this PR the same files got changed multiple times.

@codecov
Copy link

codecov bot commented Aug 2, 2021 •
edited
Loading

Codecov Report

Merging #394 (1c0547b) into master (7e261be) will decrease coverage by 0.30%.
The diff coverage is 80.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
- Coverage   88.92%   88.62%   -0.31%     
==========================================
  Files          44       44              
  Lines        1436     1477      +41     
==========================================
+ Hits         1277     1309      +32     
- Misses         85       92       +7     
- Partials       74       76       +2
Impacted Files Coverage Δ
config/config.go 90.90% <ø> (ø)
test/testdb/database.go 96.87% <0.00%> (-3.13%) ⬇️
auth/authentication.go 67.53% <77.77%> (+3.12%) ⬆️
api/user.go 77.87% <82.35%> (+0.79%) ⬆️
auth/util.go 100.00% <100.00%> (ø)
router/router.go 100.00% <100.00%> (ø)

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 7e261be...1c0547b. Read the comment docs.

@karmanyaahm
Copy link
Contributor

I've rebased the branch (you may have to update your ui dependencies with yarn install) and added some fixes to this PR, could you review them and test that it works well?

I read through all the commits and tested Register/CreateUser related things and everything seems to work very nicely. (some of the finishing touches you put in really help with user experience)

And can I squash our commits into one

👍

pigpig and others added 2 commits August 4, 2021 19:25
@karmanyaahm @jmattheis
Add registration
db864e0 
Can be enabled via the registration config flag. (disabled per default)

Fixes gotify#395

Co-authored-by: pigpig <pigpig@pig.pig>
Co-authored-by: Karmanyaah Malhotra <32671690+karmanyaahm@users.noreply.github.com>
Co-authored-by: Jannis Mattheis <contact@jmattheis.de>
@jmattheis
Inject register & version information into index.html
1c0547b 
The registration form will always be shown inside the dev mode,
because there is no api that transmits if registration is enabled.
@jmattheis jmattheis merged commit 36eb8d8 into gotify:master Aug 4, 2021
@JuniorJPDJ
Copy link

is it possible to enable this feature through env var?

@jmattheis
Copy link
Member

@JuniorJPDJ Yes, it will be (it is not released yet).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karmanyaahm karmanyaahm karmanyaahm left review comments

@jmattheis jmattheis jmattheis approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@p1gp1g @JuniorJPDJ @karmanyaahm @jmattheis