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

removed quickposts and changed app routes #889

Open
wants to merge 9 commits into
base: staging
Choose a base branch
from

Conversation

lodewiges
Copy link
Contributor

@lodewiges lodewiges commented Oct 26, 2024

Solves #890 and #886

remove all files regarding quick posts and changed some application routes to authentication routes because the should only be accessible when logged in.

Copy link

codecov bot commented Oct 26, 2024

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 13.79%. Comparing base (ff8ac7f) to head (60fcdb5).

Files with missing lines Patch % Lines
app/routes/books.js 0.00% 1 Missing ⚠️
app/routes/debit/collections.js 0.00% 1 Missing ⚠️
app/routes/debit/mandates.js 0.00% 1 Missing ⚠️
app/routes/debit/transactions.js 0.00% 1 Missing ⚠️
app/routes/forum.js 0.00% 1 Missing ⚠️
app/routes/forum/categories/category/index.js 0.00% 1 Missing ⚠️
.../forum/categories/category/threads/thread/index.js 0.00% 1 Missing ⚠️
app/routes/groups.js 0.00% 1 Missing ⚠️
app/routes/polls.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           staging     #889      +/-   ##
===========================================
+ Coverage    13.39%   13.79%   +0.39%     
===========================================
  Files          455      450       -5     
  Lines         3180     3067     -113     
===========================================
- Hits           426      423       -3     
+ Misses        2754     2644     -110     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lodewiges lodewiges marked this pull request as ready for review October 26, 2024 21:58
@lodewiges lodewiges changed the title removed quickposts and changed authenticaiton routes removed quickposts and changed authentication routes Oct 26, 2024
@lodewiges lodewiges changed the title removed quickposts and changed authentication routes removed quickposts and changed app routes Oct 26, 2024
app/router.js Outdated Show resolved Hide resolved
Copy link
Contributor

@DrumsnChocolate DrumsnChocolate left a comment

Choose a reason for hiding this comment

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

hold up, we need to provide canAccess on AuthenticatedRoutes. Did you test the pages you changed the route classes for?

@lodewiges
Copy link
Contributor Author

hold up, we need to provide canAccess on AuthenticatedRoutes. Did you test the pages you changed the route classes for?

I checked a couple of pages, I did not do I did every single one because the changes were the same between pages.

@DrumsnChocolate
Copy link
Contributor

DrumsnChocolate commented Nov 3, 2024

the pages did break; without canAccess the AuthenticatedRoute does not want to function properly.

Let's look at application.js and see the implementation of the AuthenticatedRoute: this class is a bit confusing, in that it fulfils two responsibilities: it does not only check for authentication but also verifies authorization. I find these are very similar terms, so let's be clear:

  • Authentication is the process of verifying authenticity. -> Login
  • Authorization is the process of verifying authority. -> Permissions

These are two different concepts. I think that we may benefit from separating these responsibilities, because there are definitely cases where we can use a logged-in check but do not need a permissions check.
I propose the following classes:
ApplicationRoute as it is now
AuthenticatedRoute:

  • extends ApplicationRoute
  • verifies authentication

AuthorizedRoute,

  • extends AuthenticatedRoute
  • verifies authority

EDIT:
alternatively to splitting up the classes, we could rename AuthenticatedRoute to AuthorizedRoute. That would at least improve the reader's association when seeing this term.

@DrumsnChocolate
Copy link
Contributor

In the meantime I'll add a canAccess to all the relevant routes.
We also don't need to introduce new classes in this PR, we should probably make an issue for my comment above

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.

2 participants