Fix: ensure redirect to user group view includes required group path … #3359
Conversation
|
Was there a reason this was closed? |
MoralCode
left a comment
MoralCode
left a comment
There was a problem hiding this comment.
Nice job fixing this issue! just had a small fix and this looks like it should be good to go!
kabir2123
force-pushed
the
fix/user_remove_repo-redirect
branch
from
11e7f4b to
42a7d90
Compare
I didnt sign my commit off so had to close it |
|
ah ok. this will rewrite those commits (giving them new hashes) and include a signoff in the commit message. if you prefer not to do this force push, you can also create a new branch at your rewritten commit with |
|
Great I have done the same. Can you please merge the branch now? |
There's still some unaddressed feedback I left on this PR regarding your redirect back to the settings page when there is an error (which prevents users from seeing the flash messages). I have marked this as accepted for the purposes of hacktoberfest though in case you are participating |
kabir2123
force-pushed
the
fix/user_remove_repo-redirect
branch
from
42a7d90 to
ade813b
Compare
I have made the required changes. Lmk if I need to make any more ammends. I'll be happy to do so. Also I was eyeing on Google Summer of Code and not hacktober fest but Thanks!!! |
| try: | ||
| repo_id = int(repo) | ||
| except (TypeError, ValueError): | ||
| flash("Invalid repo id provided") |
There was a problem hiding this comment.
we should probably also print a log message here with the actual exception so the admin of augur knows what specifically happened.
kabir2123
force-pushed
the
fix/user_remove_repo-redirect
branch
2 times, most recently
from
40f3c90 to
be7bade
Compare
…param and validate inputs Signed-off-by: Kabir Panda <kabirpanda@Kabirs-MacBook-Air.local>
kabir2123
force-pushed
the
fix/user_remove_repo-redirect
branch
from
be7bade to
e445b86
Compare
|
Because this is a fairly narrowly targeted fix for a confirmed crashing scenario, im going to try and get this merged for this release |
I am contributing for the first time. Hope I was actually able to solve some problem |
Welcome! This contribution definitely is helpful - you were able to identify and fix a crash you found in the UI - kudos! If you plan to stick around or continue using Augur, feel free to check out the getting started page and join the #wg-augur-8knot channel on slack if you'd like to chat with other users and/or the team! |
| ) | ||
|
|
||
| from ..server import app | ||
| from .utils import * |
There was a problem hiding this comment.
Was it your editor that reformatted a lot of these imports? I dont see anything crazy wrong just wanted to check where this came from
There was a problem hiding this comment.
The pylint check was failing coz of the order of imports. So had to make some tweaks
MoralCode
left a comment
MoralCode
left a comment
There was a problem hiding this comment.
Works on my machine and the flash messages (at least in the success case) seem to work
MoralCode
added
the
ready
3 participants
Fix: ensure redirect to user group view includes required group path param and validate inputs
Description
/account/repo/removeand ensureurl_for('user_group_view', group=group)is used so Flask can build the required path parameter.This PR fixes #3347
Notes for Reviewers
user_remove_repohandler now:repo_idandgroup_nameare present and returns the user to account settings with a flash message if notrepo_idto int and handles conversion errorsurl_for("user_group_view", group=group)for the redirect, matching the route/user/group/<group>defined inaugur/api/view/routes.pySigned-off-by: Kabir Panda kabirpa57@gmail.com
Signed commits