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: articles crud #134

Merged
merged 13 commits into from
Sep 26, 2023
Merged

feat: articles crud #134

merged 13 commits into from
Sep 26, 2023

Conversation

alfonsobries
Copy link
Contributor

Summary

A basic crud, also add permissions and model policy

Notes:

  1. You need tun run a fresh migration so new permissions are added
  2. Added a Editor role
  3. See how I defined the permissions to see if make sense (at least for the scope of this pr), for example I defined that if user can create a article can also delete it (if owns) stuff like that

Closes https://app.clickup.com/t/8678v0a1f

Checklist

  • I checked my UI changes against the design and there are no notable differences
  • I checked my UI changes for any responsiveness issues
  • I checked my (code) changes for obvious issues, debug statements and commented code
  • I opened a corresponding card on Clickup for any remaining TODOs in my code
  • I added a short description on how to test this PR (if necessary)
  • I added a storybook entry for the component that was added (if necessary)
  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged


public static function shouldSkipAuthorization(): bool
{
return app()->isLocal();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

notice that You need to remove this to test the permissions

@ItsANameToo ItsANameToo added this to the 0.8.0 milestone Sep 25, 2023
Copy link
Contributor

@ItsANameToo ItsANameToo left a comment

Choose a reason for hiding this comment

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

tiny remarks, otherwise looks good

app/Policies/ArticlePolicy.php Outdated Show resolved Hide resolved
@ItsANameToo ItsANameToo marked this pull request as draft September 25, 2023 14:20
@ItsANameToo
Copy link
Contributor

note that later on we will add permission handling to the branch, so we don't need a migration at this time for demo/prod

@alfonsobries alfonsobries marked this pull request as ready for review September 25, 2023 17:40
@alfonsobries
Copy link
Contributor Author

@ItsANameToo updated with permissions to restore and force delete and assigned to admins

@ItsANameToo
Copy link
Contributor

did you push the changes @alfonsobries ? 🤔

@ItsANameToo ItsANameToo marked this pull request as draft September 26, 2023 07:55
@ItsANameToo ItsANameToo removed this from the 0.8.0 milestone Sep 26, 2023
@ItsANameToo ItsANameToo added this to the TBD milestone Sep 26, 2023
@alfonsobries
Copy link
Contributor Author

alfonsobries commented Sep 26, 2023

@ItsANameToo yesterday wasnt my sharpest day😅, should be ready now

@alfonsobries alfonsobries marked this pull request as ready for review September 26, 2023 15:46
@ItsANameToo ItsANameToo merged commit ecd3f75 into feat/articles Sep 26, 2023
12 checks passed
@ItsANameToo ItsANameToo deleted the feat/articles-crud branch September 26, 2023 17:18
@ItsANameToo ItsANameToo modified the milestones: TBD, 0.10.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants