Skip to content

Conversation

@yoution
Copy link
Contributor

@yoution yoution commented Mar 3, 2021

No description provided.

Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

@yoution I've tested it with pshah_copilot user. And for all the challenges it says that copilot cannot delete them even if I created it. Actually, I can delete it.

You may check it with pshah_copilot in this project http://localhost:3000/projects/9132/challenges.

  1. Create a new challenge
  2. Go to challenge list and click Delete
  3. It would say You don't have permission to delete this challenge. But if you start editing this challenge you can edit it, and also you can delete this challenge from the challenge edit page.

See demo video https://monosnap.com/file/3sb33DNOHjZwJWql2j6RAQHCRysU8W

Also, one thing regarding the code. Can we implement this method without using Redux? It feels that it doesn't bring value to put this into the Redux. If we put to Redux we have to pass isCheckChalengePermission, hasEditChallengePermission, checkEditPermission in many places which is not comfortable.

I guess we can do something like this:

  • remove this method from Redux, remove dispatch
  • method checkChallengeEditPermission should return a Promise with value true or false if user has permission or no
  • inside ChallengeCard/index.js call this method and put isCheckChalengePermission, hasEditChallengePermission in the local state
  • I think this would simplify the code. Would this work?

Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

@yoution all works great now.

The only thing, a lot of code that is not changed has changed formatting now. Could you please don't make unnecessary changes to formating where we don't need it. Because we work in parallel on multiple tasks, such changes could lead to merge conflicts and it would be hard to merge everything together after.

image

Copy link
Contributor

@maxceem maxceem left a comment

Choose a reason for hiding this comment

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

Perfect now. Thank you for all the changes @yoution.

@maxceem maxceem merged commit efd01df into topcoder-platform:develop Mar 4, 2021
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.

2 participants