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

fix: transferusers.php #1084

Merged
merged 3 commits into from
Jan 3, 2025
Merged

Conversation

RaphaelCapone
Copy link
Contributor

@RaphaelCapone RaphaelCapone commented Dec 24, 2024


💡 Description

Updating transferusers.php for the new database changes. Now tranfered users by this script are listed properly in the admin panel and can use services. see issue at: https://discord.com/channels/787829714483019826/850551820018384907/1320901027649683506


🛠️ Type of Change

Breaking change


  • Change tested *

Updating transferusers.php for the new database changes.
Now tranfered users by this script are listed properly in the admin panel and can use services.
see issue at: https://discord.com/channels/787829714483019826/850551820018384907/1320901027649683506
@S0ly
Copy link
Member

S0ly commented Dec 25, 2024

why you say it contain breaking changes ? your only modifying a third party script

@S0ly S0ly requested a review from 1day2die December 25, 2024 00:37
@S0ly
Copy link
Member

S0ly commented Dec 25, 2024

also checking this script / code it contains many things wrong, in relation to the fix or not

@MrWeez
Copy link
Collaborator

MrWeez commented Dec 25, 2024

@RaphaelCapone Thanks for the PR, but could you please make these changes in the development branch instead of the main? Also, you added a php closing tag, which is not correct according to the norms of writing code in php.

@MrWeez
Copy link
Collaborator

MrWeez commented Dec 25, 2024

Otherwise, everything looks correct, good job

@RaphaelCapone RaphaelCapone changed the base branch from main to development December 25, 2024 00:46
@S0ly
Copy link
Member

S0ly commented Dec 25, 2024

Correct $CPPPG_PASSWORD to $CPGG_PASSWORD.
also why the roles are selected by ID ? is it not like the worst things to do ???
I mean I understand that he is only fixing one problem but this script is full of problems

@RaphaelCapone RaphaelCapone marked this pull request as draft December 25, 2024 00:47
@MrWeez
Copy link
Collaborator

MrWeez commented Dec 25, 2024

also why the roles are selected by ID ? is it not like the worst things to do ???

No, because the default role IDs are not changeable. They are the same for all instances

@S0ly
Copy link
Member

S0ly commented Dec 25, 2024

yes but you still should never use IDs 99% of the time. its just something that wait to break here

@S0ly
Copy link
Member

S0ly commented Dec 25, 2024

anyway @RaphaelCapone you can only do your fix, if you dont want to fully fix this code I may refactor it in the future, thanks you

@RaphaelCapone RaphaelCapone marked this pull request as ready for review December 25, 2024 00:52
@MrWeez
Copy link
Collaborator

MrWeez commented Dec 25, 2024

yes but you still should never use IDs 99% of the time. its just something that wait to break here

In that case, how do you see the implementation of this? This is how the permission system works, and to fix it you need to change the backend of the CtrlPanel itself

@S0ly
Copy link
Member

S0ly commented Dec 25, 2024

idk how the backend is made

but if you have a mysql row saying, role name = this string

you can search by name and not by ID, IDs are normally automatically generated by mysql and are unpredictable

anyway

@MrWeez MrWeez self-requested a review December 25, 2024 00:59
Copy link
Collaborator

@MrWeez MrWeez left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll wait for the review from @1day2die , he's more familiar with his script after all 😅

@S0ly
Copy link
Member

S0ly commented Dec 25, 2024

I think we should accept this fix and I will further refactor and fix this script, also this script have nothing to do here, it should be in the app/Console/Commands directory for laravel

@S0ly S0ly changed the title Fixing transferusers.php fix: transferusers.php Dec 25, 2024
@S0ly S0ly added Bug / Fix Something isn't working and may need a fix Medium Priority Needs attention, not urgent labels Dec 25, 2024
@RaphaelCapone
Copy link
Contributor Author

RaphaelCapone commented Dec 25, 2024

yes but you still should never use IDs 99% of the time. its just something that wait to break here

I know fields are kinda confusing, but the thing is that, in my change, after the new users are imported it just gets their ID(not setting it manually or anything) in order to set the right ID in the model_id field.
So, for example if ctrl panel have this user table(maybe some users got deleted):
id name
1 user1
4 user2 ----> (the new imported user)

The model_has_roles table will look like that [the id in this case could be for example if the user used the outdated script, deleted users by loggin manually in their account and then updating the script and start again importing the ptero users, so its specially made for not messing up things]
id role_id model_type model_id
1 1 App\Models\User 1
2 3 App\Models\User 4 --> the Localy ID of the imported ptero panel user
and 1(root admin) and 3(client) are just the IDs of the roles from roles table.
Hope my explanation will clarify things up

@1day2die 1day2die merged commit 56037ea into Ctrlpanel-gg:development Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug / Fix Something isn't working and may need a fix Medium Priority Needs attention, not urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants