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

refactor(router_core): move hardcoded value to constant for max invitations (fix #5938) #6151

Closed
wants to merge 0 commits into from

Conversation

ramkumargs
Copy link

@ramkumargs ramkumargs commented Sep 28, 2024

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Motivation and Context

How did you test it?

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code
  • I added unit tests for my changes where possible

@ramkumargs ramkumargs requested a review from a team as a code owner September 28, 2024 03:52
Copy link

semanticdiff-com bot commented Sep 28, 2024

Review changes with SemanticDiff.

@@ -498,7 +498,7 @@ pub async fn invite_multiple_user(
req_state: ReqState,
auth_id: Option<String>,
) -> UserResponse<Vec<InviteMultipleUserResponse>> {
if requests.len() > 10 {
if requests.len() > consts::user::MAX_INVITE_REQUESTS {
return Err(report!(UserErrors::MaxInvitationsError))
.attach_printable("Number of invite requests must not exceed 10");
Copy link
Contributor

@ThisIsMani ThisIsMani Oct 1, 2024

Choose a reason for hiding this comment

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

Change this attach_printable as well.

Log the const value and also try to log the number of requests as well in this.

Copy link
Author

Choose a reason for hiding this comment

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

Changes are done and committed the code.

@ramkumargs
Copy link
Author

@ThisIsMani, Changes are done and committed the code.

@ThisIsMani
Copy link
Contributor

@ramkumargs, were you able to test this?

@ThisIsMani
Copy link
Contributor

ThisIsMani commented Oct 7, 2024

@ramkumargs, were you able to test this?

Let me know if you need instructions on this.

@ThisIsMani ThisIsMani linked an issue Oct 8, 2024 that may be closed by this pull request
1 task
@ramkumargs
Copy link
Author

ramkumargs commented Oct 9, 2024

@ramkumargs, were you able to test this?

Let me know if you need instructions on this.

@ThisIsMani please give me the steps for testing.
Cargo test -- Is throwing error: Package common_utils v0.1.0 (/root/hyperswitch/crates/common_utils) does not have feature router_env. (Trying to look into it) (RHEL env.)

@ThisIsMani
Copy link
Contributor

Hey @ramkumargs, sorry for the late reply.

Steps for testing:

  1. Create a user
    curl --location 'http://localhost:8080/user/signup' \
    --header 'Content-Type: application/json' \
    --header 'api-key: test_admin' \
    --data '{
        "email": "email",
        "password": "password"
    }'
    
  2. The above API will give a token in the response.
    curl --location 'http://localhost:8080/user/2fa/terminate?skip_two_factor_auth=true' \
    --header 'Authorization: Bearer {token from signup}'
    
  3. The above API will give one more token. With that you can hit invite API.
    curl --location 'http://localhost:8080/user/user/invite_multiple' \
    --header 'Content-Type: application/json' \
    --header 'Authorization: Bearer {token from 2FA API}' \
    --data-raw '[
        {
            "email": "email",
            "name": "name",
            "role_id": "merchant_view_only"
        },
        {
            "email": "email2",
            "name": "name2",
            "role_id": "merchant_view_only"
        }
    ]'
    
    • If the number of elements in the requests are more than what you configured, it should throw error.

Let me know if you face any issues. If you were able to check, please attach a screenshot in the description.

@gorakhnathy7 gorakhnathy7 added the hacktoberfest Issues that are up for grabs for Hacktoberfest participants label Oct 15, 2024
@ramkumargs
Copy link
Author

Hey @ramkumargs, sorry for the late reply.

Steps for testing:

  1. Create a user

    curl --location 'http://localhost:8080/user/signup' \
    --header 'Content-Type: application/json' \
    --header 'api-key: test_admin' \
    --data '{
        "email": "email",
        "password": "password"
    }'
    
  2. The above API will give a token in the response.

    curl --location 'http://localhost:8080/user/2fa/terminate?skip_two_factor_auth=true' \
    --header 'Authorization: Bearer {token from signup}'
    
  3. The above API will give one more token. With that you can hit invite API.

    curl --location 'http://localhost:8080/user/user/invite_multiple' \
    --header 'Content-Type: application/json' \
    --header 'Authorization: Bearer {token from 2FA API}' \
    --data-raw '[
        {
            "email": "email",
            "name": "name",
            "role_id": "merchant_view_only"
        },
        {
            "email": "email2",
            "name": "name2",
            "role_id": "merchant_view_only"
        }
    ]'
    
    • If the number of elements in the requests are more than what you configured, it should throw error.

Let me know if you face any issues. If you were able to check, please attach a screenshot in the description.

@ThisIsMani , I will test and update you today

@gorakhnathy7
Copy link
Collaborator

Hey @ramkumargs Any update here?

@ramkumargs
Copy link
Author

Hi @gorakhnathy7 sorry for late reply. I was struck on my some personal issue.
I have started the testing now.

@ramkumargs
Copy link
Author

Hi,
When running below command, hanging at => [hyperswitch-web 5/5] RUN npm install
docker compose up -d (Expecting to run this command before executing curl commands)
image
Am I proceeding in the right way for testing?

@ThisIsMani
Copy link
Contributor

Hey @ramkumargs, did the hyperswitch-server container start without any issues?

@ramkumargs
Copy link
Author

ramkumargs commented Oct 25, 2024

Not started. Failing at hyperswitch-hyperswitch-server-1, after long build time
image

@ThisIsMani
Copy link
Contributor

Can you use cargo run instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Issues that are up for grabs for Hacktoberfest participants
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REFACTOR] Move Hardcoded Value to Constant for Max Invitations
4 participants