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

Update admin paths and change organization_id to organization_name #4010

Merged
merged 18 commits into from
Mar 29, 2024

Conversation

dorner
Copy link
Collaborator

@dorner dorner commented Jan 5, 2024

Resolved #3991 .

This changes the organization_id parameter to organization_name, and removes the need for organization_name in admin controllers.

This is a pretty big change - we probably should do some manual testing on staging.

@dorner dorner marked this pull request as ready for review January 7, 2024 01:48
@cielf
Copy link
Collaborator

cielf commented Jan 15, 2024

Agreed. If I read this correctly, pretty much every thing you can do as an admin is affected.

@cielf
Copy link
Collaborator

cielf commented Mar 9, 2024

@dorner. If I read the tests right, there are failures that are related to this change.

@dorner
Copy link
Collaborator Author

dorner commented Mar 10, 2024

Should be fixed now! I'd like to try to get this merged sooner rather than later... I don't like having such a big PR getting stale.

@cielf
Copy link
Collaborator

cielf commented Mar 10, 2024

On the other hand, I'm a little leery about having such a big PR in a release along with a host of other things. g. Let's see if we can get @awwaiid to take a look from a technical pov soonest.

@cielf cielf requested a review from awwaiid March 10, 2024 21:35
@cielf
Copy link
Collaborator

cielf commented Mar 10, 2024

This should only affect admin things, though, right?

@dorner
Copy link
Collaborator Author

dorner commented Mar 11, 2024

I use the magical software engineering word should, as in it "should" only affect admin things... 😁

@cielf
Copy link
Collaborator

cielf commented Mar 11, 2024

(nods) That makes me more at ease about it going in with a bunch of other stuff.

@cielf
Copy link
Collaborator

cielf commented Mar 12, 2024

Hey @dorner,

In the context that this PR 'removes the need for organization name in admin controllers", seeing this path when I logged as the superadmin surprised me. The organization name part is also different than what is on staging, which has the organization_name as admin.

Screenshot 2024-03-12 at 1 09 54 PM

# Conflicts:
#	spec/requests/items_requests_spec.rb
@dorner
Copy link
Collaborator Author

dorner commented Mar 22, 2024

@cielf I can't reproduce that. This is what I get
image

Do you have reproduction instructions?

@cielf
Copy link
Collaborator

cielf commented Mar 23, 2024

Of course, it appears to be working now... which puts this back on my list to "kick" -- hopefully I'll find out how I got there.

@cielf
Copy link
Collaborator

cielf commented Mar 23, 2024

I haven't found it for the dashboard, but this one, which is where you get to after you perform an action on a user in the organization view... seems like it has some redundant info in the path -- yes? (Example with a new test organization, new_org)
http://localhost:3000/admin/organizations/3?organization_name=new_org

Edit: Quickest path to this with a new seed: sign in as superadmin, Organizations | All Organizations View an organization, scroll down to find the user who is not and admin and make them an admin.

@dorner
Copy link
Collaborator Author

dorner commented Mar 28, 2024

@cielf that was pretty much just there. 😄 I've pushed a fix for it.

@dorner
Copy link
Collaborator Author

dorner commented Mar 28, 2024

...scratch that. The issue is that the controller for that action is not under the admin namespace. Put in a different fix, hoping this works.

@cielf
Copy link
Collaborator

cielf commented Mar 28, 2024

I'm happy with it from a functional pov now. Would like @awwaiid to take a look from a technical pov.

@@ -27,7 +27,7 @@
get admin_dashboard_path
expect(response).to be_successful

edit_user_path_pattern = %r{admin/users/#{user_with_name.id}/edit\?organization_id=\w+}
edit_user_path_pattern = %r{admin/users/#{user_with_name.id}/edit}
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah yes

Copy link
Collaborator

@awwaiid awwaiid left a comment

Choose a reason for hiding this comment

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

Looks great. I made two small non-functional changes -- fixed two comments and removed a few stray organization_id: path parameters.

@awwaiid awwaiid merged commit 2d3683c into main Mar 29, 2024
20 checks passed
@awwaiid awwaiid deleted the 3991-admin-paths branch March 29, 2024 18:07
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.

Cleanup on admin_ paths
3 participants