Skip to content

fix(invite): set accepted for existing users#571

Merged
AVVS merged 5 commits intomasterfrom
fix/set-invite-accepted
Jul 14, 2022
Merged

fix(invite): set accepted for existing users#571
AVVS merged 5 commits intomasterfrom
fix/set-invite-accepted

Conversation

@sky3d
Copy link
Copy Markdown
Collaborator

@sky3d sky3d commented Jul 11, 2022

No description provided.

@sky3d sky3d requested a review from AVVS July 11, 2022 14:44
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 11, 2022

Codecov Report

Merging #571 (9cda6aa) into master (b73e272) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #571   +/-   ##
=======================================
  Coverage   96.38%   96.38%           
=======================================
  Files         192      192           
  Lines       11453    11460    +7     
=======================================
+ Hits        11039    11046    +7     
  Misses        414      414           
Impacted Files Coverage Δ
src/actions/organization/create.js 100.00% <100.00%> (ø)
src/actions/organization/members/add.js 100.00% <100.00%> (ø)
src/utils/organization/add-organization-members.js 98.23% <100.00%> (+0.11%) ⬆️
...ls/organization/get-organization-member-details.js 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 ba0f194...9cda6aa. Read the comment docs.


member.username = member.email;
member.invited = Date.now();
member.accepted = password ? Date.now() : null;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

logic is being changed here, how is the back-compatibility here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I suppose the logic here is not quite correct.

In the distributeUsersByExist getUserId returns only id without password for existing users.
In the registerOrganizationMember method returns the user with password

Then the existing users combines with new ones
const organizationMembers = registeredMembers.concat(createdMembers);
Some items in the merged list have a password but some do not. In the addMember method accepted-logic depends on the password.

As a result accepted flag will never assigned for existing users. Password condition is not valid in this case and I pass the { inviteAccepted: true } flag directly to determine the case when user invitation accepted .

Copy link
Copy Markdown
Collaborator Author

@sky3d sky3d Jul 14, 2022

Choose a reason for hiding this comment

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

back-compatibility

@AVVS Added the separated field for joinedAt and keep the previous logic with accept.

@sky3d sky3d force-pushed the fix/set-invite-accepted branch from a38f763 to 2b43297 Compare July 13, 2022 15:59
@sky3d sky3d requested review from AVVS and BrRenat July 14, 2022 08:56
@AVVS AVVS merged commit 8c454fe into master Jul 14, 2022
@AVVS AVVS deleted the fix/set-invite-accepted branch July 14, 2022 15:51
AVVS pushed a commit that referenced this pull request Jul 15, 2022
## [15.6.6](v15.6.5...v15.6.6) (2022-07-15)

### Bug Fixes

* **invite:** set accepted for existing users ([#571](#571)) ([8c454fe](8c454fe))
* validate signature using raw body ([#567](#567)) ([16142ca](16142ca))
@AVVS
Copy link
Copy Markdown
Member

AVVS commented Jul 15, 2022

🎉 This PR is included in version 15.6.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@AVVS AVVS added the released label Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants