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

feat: adding collaborator via invitation link #2675

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

vedant-jain03
Copy link
Member

Fixes #2307

Describe the changes you have made in this PR -

  • Migration file to add new column collaboration_token, index, and token_expires_at in Project table
  • Functionality to add members to the group via link
  • Different conditions like if the collaborator is already present, URL not valid, token expired.
  • Expiry date of token is 12 days.

Screenshots of the changes (If any) -

CircuitVerse.-.Untitled.-.Google.Chrome.2021-12-06.18-03-31.mp4

Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.

</div>
</div>

Copy link

Choose a reason for hiding this comment

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

Extra whitespace detected at end of line.

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

<span><%= t("projects.invite_link_btn") %></span>
<% end %>
<div class="collapse" id="multiCollapseExample2">
<textarea id="embedInviteLink" class="collaborator-invite-link-text" rows="4" cols="35"></textarea>
Copy link

Choose a reason for hiding this comment

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

Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.

Copy link
Member

Choose a reason for hiding this comment

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

@vedant-jain03 Why this is closed ?

Copy link
Member Author

@vedant-jain03 vedant-jain03 Jan 25, 2022

Choose a reason for hiding this comment

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

@tachyons I thought it is just readonly textarea as we are only rendering the link for the invitation, there should not be any modification from the user end to the invitation link.
WDYT should I add a label for it sir @tachyons ? I will add readonly="readonly" attribute for the element.

@vedant-jain03
Copy link
Member Author

vedant-jain03 commented Dec 6, 2021

@tachyons @pavanjoshi914 @Shivansh2407 @nitin10s Kindly Review.

app/controllers/projects_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects_controller.rb Outdated Show resolved Hide resolved
app/models/project.rb Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Dec 6, 2021

Coverage Status

Coverage decreased (-0.9%) to 80.792% when pulling b06392e on vedant-jain03:issue2307 into d6d4f71 on CircuitVerse:master.

@tachyons
Copy link
Member

tachyons commented Dec 6, 2021

You can follow the rest conventions by tweaking the URL.

Token generation should be

POST /projects/:id/collaboration

@vedant-jain03
Copy link
Member Author

vedant-jain03 commented Dec 12, 2021

@tachyons @pavanjoshi914 Kindly review

I have added a new inherited class collaboration controller and added create method under which token generation will execute

app/controllers/projects/collaboration_token_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects/collaboration_token_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects_controller.rb Outdated Show resolved Hide resolved
app/models/project.rb Show resolved Hide resolved
app/models/project.rb Show resolved Hide resolved
@vedant-jain03
Copy link
Member Author

vedant-jain03 commented Dec 12, 2021

@tachyons @pavanjoshi914 @satu0king @gr455 I have a suggestion, what if we can also change the design of showing collaborators just like showing group members. As it is space-optimized.
@nitin10s

@vedant-jain03 vedant-jain03 changed the title feat: adding collaborator via invitation link feat: added collaborator via invitation link Dec 12, 2021
@vedant-jain03 vedant-jain03 changed the title feat: added collaborator via invitation link feat: adding collaborator via invitation link Dec 12, 2021
@nitin10s
Copy link
Member

Let's leave it as it is, for now, we'll revisit certain sections like these and design them appropriately.

@vedant-jain03
Copy link
Member Author

Let's leave it as it is, for now, we'll revisit certain sections like these and design them appropriately.

Sure

@vedant-jain03
Copy link
Member Author

@tachyons @gr455 Any feedback sir?

Copy link
Member

@gr455 gr455 left a comment

Choose a reason for hiding this comment

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

This doesn't work when the Project access type is Private, although I can add collaborators in Private projects by email

@vedant-jain03
Copy link
Member Author

vedant-jain03 commented Jan 6, 2022

This doesn't work when the Project access type is Private, although I can add collaborators in Private projects by email

I think this is working, have a look:

bandicam.2022-01-06.13-09-34-943.mp4

@gr455
Copy link
Member

gr455 commented Jan 7, 2022

This doesn't work when the Project access type is Private, although I can add collaborators in Private projects by email

I think this is working, have a look:

bandicam.2022-01-06.13-09-34-943.mp4

Oh, I had pasted the link in an unauthenticated session (incognito). Instead of directing me to login, it just showed me the unauthorized message

@vedant-jain03
Copy link
Member Author

vedant-jain03 commented Jan 7, 2022

This doesn't work when the Project access type is Private, although I can add collaborators in Private projects by email

I think this is working, have a look:
bandicam.2022-01-06.13-09-34-943.mp4

Oh, I had pasted the link in an unauthenticated session (incognito). Instead of directing me to login, it just showed me the unauthorized message

Oh 😅,so WDYT?

@vedant-jain03
Copy link
Member Author

vedant-jain03 commented Jan 20, 2022

@tachyons Kindly provide me feedback for this PR sir.

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

@@ -1,6 +1,6 @@
<script src="https://cdn.jsdelivr.net/bootstrap.tagsinput/0.8.0/bootstrap-tagsinput.min.js"></script>

<div id="collaboratorModal" class="modal fade" role="dialog">
<div id="collaboratorModal" class="modal fade" role="dialog" data-controller="projectCollaboration">
Copy link

Choose a reason for hiding this comment

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

Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.

<span><%= t("projects.invite_link_btn") %></span>
<% end %>
<div class="collapse" id="multiCollapseExample2">
<textarea readonly="readonly" id="embedInviteLink" class="collaborator-invite-link-text" rows="4" cols="35"><%= user_project_url %>/invite/<%= @project.collaboration_token %></textarea>
Copy link

Choose a reason for hiding this comment

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

Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.

app/controllers/projects/invitations_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects/invitations_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects/invitations_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects/invitations_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects/invitations_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects/invitations_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects/invitations_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects/invitations_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects/invitations_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects/invitations_controller.rb Outdated Show resolved Hide resolved
@vedant-jain03
Copy link
Member Author

@tachyons

  • I have added integration with the stimulus controller.
  • Move project_invite to project/invitation#create.
  • Added check if the collaborator is the author.
  • Added readonly property to textarea.

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

@codeclimate
Copy link

codeclimate bot commented Jan 25, 2022

Code Climate has analyzed commit b06392e and detected 11 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 3
Style 7
Bug Risk 1

View more on Code Climate.

@vedant-jain03
Copy link
Member Author

@gr455 Any views for this PR?

@vedant-jain03
Copy link
Member Author

@tachyons Any feedback for this, sir?

Copy link
Member

@tachyons tachyons left a comment

Choose a reason for hiding this comment

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

codeclimate warnings are closed without addressing them

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.

Adding collaborator via invitation link
5 participants