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

Give client and project index page access to employee #268

Merged
merged 3 commits into from
Apr 18, 2022

Conversation

shalapatil
Copy link
Contributor

@shalapatil shalapatil commented Apr 14, 2022

Notion card

Summary

Added client and project list page access for employees. It was giving forbidden error.

Preview

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Checklist:

  • I have manually tested all workflows
  • I have performed a self-review of my own code
  • I have added automated tests for my code

@vipulnsward vipulnsward temporarily deployed to miru-review-pr-268 April 14, 2022 16:27 Inactive
@github-actions
Copy link

Current Code Coverage Percent of this PR:

91.03 %

Files having coverage below 100%

Impacted Files Coverage
/app/controllers/clients_controller.rb 94.44 %
/app/controllers/invoices_controller.rb 66.67 %
/app/controllers/concerns/timesheet.rb 75.0 %
/app/models/project.rb 90.32 %
/app/controllers/users/registrations_controller.rb 40.0 %
/app/controllers/users/sessions_controller.rb 25.0 %
/app/controllers/users/omniauth_callbacks_controller.rb 25.0 %
/app/controllers/users/passwords_controller.rb 41.67 %
/app/controllers/internal_api/v1/timesheet_entry_controller.rb 40.0 %
/app/controllers/internal_api/v1/projects_controller.rb 75.0 %
/app/controllers/internal_api/v1/timesheet_entry/bulk_action_controller.rb 50.0 %

Copy link
Contributor

@supriya3105 supriya3105 left a comment

Choose a reason for hiding this comment

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

LGTM from functional perspective

Copy link
Contributor

@rohitjoshixyz rohitjoshixyz left a comment

Choose a reason for hiding this comment

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

Fix grammatical errors in spec description and discuss with Gowsik for the failing spec and uncomment it before the merge.
I would suggest using Grammarly and performing a self-review before assigning for a review to avoid such nitpicks

spec/requests/internal_api/v1/projects/show_spec.rb Outdated Show resolved Hide resolved
Comment on lines +17 to +18
# Check why following test is failing with Gowsik and uncomment following
# expect(response).to have_http_status(:redirect)
Copy link
Contributor

Choose a reason for hiding this comment

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

Address this before merging else we tend to never fix it on priority

spec/requests/internal_api/v1/projects/show_spec.rb Outdated Show resolved Hide resolved
spec/requests/internal_api/v1/projects/show_spec.rb Outdated Show resolved Hide resolved
spec/requests/internal_api/v1/projects/show_spec.rb Outdated Show resolved Hide resolved
Co-authored-by: Rohit Joshi <rohit.joshiadvanced@gmail.com>
Copy link
Contributor

@keshavbiswa keshavbiswa left a comment

Choose a reason for hiding this comment

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

LGTM! After you fix Rohit's suggestions and merge conflicts.

@@ -42,12 +42,34 @@
context "when the user is an employee" do
Copy link
Contributor

Choose a reason for hiding this comment

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

top level should describe blocks

@vipulnsward vipulnsward merged commit 5ce4fe2 into develop Apr 18, 2022
@vipulnsward vipulnsward deleted the fix-employee-access-for-client-and-project branch April 18, 2022 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants