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

Improve organization invitation with registration/login flow #387

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

noniagriconomie
Copy link
Contributor

Hello

Here is a PR that tries to improve the organization invitation process flow
fixes #381

Friendly ping @akondas

Thank you,

Copy link
Member

@akondas akondas left a comment

Choose a reason for hiding this comment

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

Although I like this change, what do you say to leave one button and do it like this:

The route "organization_accept_invitation" could be opened (available without logging in). In its action, it could be detected if the user is logged in, if not a redirection with 'invitationtoken to login (and then the same on the login action, you would have to render the button for registration withinvitation` token)

src/Controller/IndexController.php Outdated Show resolved Hide resolved
src/Controller/SecurityController.php Outdated Show resolved Hide resolved
@noniagriconomie
Copy link
Contributor Author

@akondas thanks for your review
agreed, I will refactor like this, will try to do it this week

@noniagriconomie noniagriconomie force-pushed the feature-381 branch 5 times, most recently from 5bb1c92 to 99877a2 Compare January 28, 2021 15:53
@noniagriconomie
Copy link
Contributor Author

@akondas code is rewritten, I am testing it now
new round of review appreciated

Copy link
Member

@akondas akondas left a comment

Choose a reason for hiding this comment

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

One change and we are ready to go 😉 👍

@noniagriconomie noniagriconomie force-pushed the feature-381 branch 2 times, most recently from 2e6c771 to 59eabe3 Compare January 29, 2021 08:28
Copy link
Member

@akondas akondas left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@akondas
Copy link
Member

akondas commented Jan 29, 2021

@noniagriconomie Also let me know if you will be able to do a rebase or can I help with it?

@codecov
Copy link

codecov bot commented Jan 29, 2021

Codecov Report

Merging #387 (ef7c255) into master (d8397fe) will decrease coverage by 0.16%.
The diff coverage is 61.53%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #387      +/-   ##
============================================
- Coverage     99.48%   99.31%   -0.17%     
- Complexity     1850     1853       +3     
============================================
  Files           297      297              
  Lines          5828     5846      +18     
============================================
+ Hits           5798     5806       +8     
- Misses           30       40      +10     
Impacted Files Coverage Δ Complexity Δ
src/Controller/IndexController.php 75.00% <50.00%> (-25.00%) 3.00 <1.00> (+1.00) ⬇️
src/Controller/RegistrationController.php 90.90% <50.00%> (-9.10%) 11.00 <1.00> (+1.00) ⬇️
src/Controller/Organization/MembersController.php 95.83% <57.14%> (-4.17%) 15.00 <1.00> (+1.00) ⬇️
src/Controller/SecurityController.php 100.00% <100.00%> (ø) 10.00 <3.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 d8397fe...ef7c255. Read the comment docs.

@noniagriconomie
Copy link
Contributor Author

@akondas PR rebased and comment addressed for the security access control

@akondas akondas merged commit 950e44b into repman-io:master Feb 1, 2021
@akondas
Copy link
Member

akondas commented Feb 1, 2021

Thanks @noniagriconomie 🙏

@noniagriconomie noniagriconomie deleted the feature-381 branch February 1, 2021 08:22
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.

Onboarding new user/organization
2 participants