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.

Sign up for GitHub

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

Jump to bottom

Asset : Added Test For AssetInSameState #429

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Asset : Added Test For AssetInSameState #429

wants to merge 10 commits into from

Conversation

zeel991
Copy link
Contributor

@zeel991 zeel991 commented Apr 12, 2024 •
edited

@zeel991
Copy link
Contributor Author

zeel991 commented Apr 13, 2024

Hey @amarts @vatsa287 , the PR has passed all the three tests

vatsa287
vatsa287 requested changes Apr 15, 2024
View reviewed changes
Copy link
Member

@vatsa287 vatsa287 left a comment

Choose a reason for hiding this comment

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

Please check for both the instances

  • When instance-id is provided and asset is in same state,
  • When instance-id is not provided, then change status of asset itself, but the given new status is of same status of existing asset.

Above must be done for both status_change & vc_status_change.
So totally 4 places to check the error-code AssetInSameState.

@zeel991
Copy link
Contributor Author

zeel991 commented Apr 15, 2024

Hey @vatsa287 , I hope I have completed your required changes
Please review the PR

vatsa287
vatsa287 requested changes Apr 18, 2024
View reviewed changes
Copy link
Member

@vatsa287 vatsa287 left a comment

Choose a reason for hiding this comment

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

Few minor comments

pallets/asset/src/tests.rs Outdated Show resolved Hide resolved
pallets/asset/src/tests.rs Outdated Show resolved Hide resolved
pallets/asset/src/tests.rs Outdated Show resolved Hide resolved
pallets/asset/src/tests.rs Outdated Show resolved Hide resolved
@zeel991
Copy link
Contributor Author

zeel991 commented Apr 18, 2024

Hey @vatsa287 , I have made the changes , Please review the PR

vatsa287
vatsa287 approved these changes Apr 22, 2024
View reviewed changes
Copy link
Member

@vatsa287 vatsa287 left a comment •
edited

Choose a reason for hiding this comment

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

LGTM.
Hoping you have gone through this discussion, please start contributing on next level issues other than good-first-issues.

@vatsa287 vatsa287 requested a review from amarts April 22, 2024 06:16
@zeel991
Copy link
Contributor Author

zeel991 commented Apr 30, 2024

Hello @amarts , can you please review the PR

@smohan-dw
Copy link
Member

LGTM. @zeel991 need to resolve the merge conflicts.

@zeel991
Copy link
Contributor Author

zeel991 commented May 3, 2024

LGTM. @zeel991 need to resolve the merge conflicts.

Done !

@zeel991
Copy link
Contributor Author

zeel991 commented May 7, 2024

Hello @amarts sir ,PTAL

@vatsa287 vatsa287 requested review from amarts and removed request for amarts May 23, 2024 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vatsa287 vatsa287 vatsa287 approved these changes

@amarts amarts Awaiting requested review from amarts

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C4GT] Asset: Add tests for AssetInSameState
3 participants
@zeel991 @smohan-dw @vatsa287