-
Notifications
You must be signed in to change notification settings - Fork 8
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
Protect Routes #292
Protect Routes #292
Conversation
|> assign(:page_title, gettext("Edit Activity")) | ||
|> assign(:activity, activity)} | ||
else | ||
raise AtomicWeb.MismatchError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you raising an exception on an handle_params
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what he is doing is raising an exception making the current request fail with 404 status, which automatically shows the 404 page. Smart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see. Why not wrap this logic inside a function and call it on a Plug implemented at the route level? That way, we could also use that function when deciding to display (or not) the "Edit" button on the show of an activity. Agreed? 🙏
Edit: This was supposed to be a reply to the already created thread 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is a good idea. Will clean up the code quite a bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|> assign(:page_title, gettext("Edit Activity")) | ||
|> assign(:activity, activity)} | ||
else | ||
raise AtomicWeb.MismatchError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what he is doing is raising an exception making the current request fail with 404 status, which automatically shows the 404 page. Smart
|> assign(:page_title, gettext("Edit Activity")) | ||
|> assign(:activity, activity)} | ||
else | ||
raise AtomicWeb.MismatchError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see. Why not wrap this logic inside a function and call it on a Plug implemented at the route level? That way, we could also use that function when deciding to display (or not) the "Edit" button on the show of an activity. Agreed? 🙏
Edit: This was supposed to be a reply to the already created thread 😅
43a7b2c
to
0825507
Compare
d5ddbb8
to
290bcbe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6ea76ee
to
f27aafb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
This PR belongs to @joaodiaslobo , in order to merge this quicker i fixed some tests and open it because @joaodiaslobo will be out of town for a week and we can't delay this feature much more.