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

server Simple support to sync users from SG over to Ayon #76

Merged
merged 9 commits into from
Dec 2, 2024

Conversation

fabiaserra
Copy link
Contributor

Quick implementation to sync the user database from SG to Ayon.

This also adds a change to the existing populate tables functionality to filter out all the projects that we wouldn't be able to sync (because they have whitespaces or not a code name) so the table isn't that long and it's easier to navigate through the projects... although ideally the whole server UI needs a revamp

@jakubjezek001
Copy link
Member

This also adds a change to the existing populate tables functionality to filter out all the projects that we wouldn't be able to sync

Before, we had the opportunity to visually identify the underlying issues within those specific projects and proactively address them. Now, without this visibility, valuable time may be spent attempting to pinpoint the root cause of any unexpected occurrences

server/frontend/dist/index.html Outdated Show resolved Hide resolved
server/frontend/dist/shotgrid-addon.js Outdated Show resolved Hide resolved
@fabiaserra
Copy link
Contributor Author

This also adds a change to the existing populate tables functionality to filter out all the projects that we wouldn't be able to sync

Before, we had the opportunity to visually identify the underlying issues within those specific projects and proactively address them. Now, without this visibility, valuable time may be spent attempting to pinpoint the root cause of any unexpected occurrences

Fair, in my case I prefer it this way as otherwise there's lots of noise of template projects and other legacy things that I know for sure I won't ever sync and a table widget is already clunky enough for this workflow that the more filtering the better. Also the documentation that shows in the same page clearly states both requirements for the sync to work...
image

I think I´d rather steal the UI from another of the addons that might have spend more time trying to make this friendlier. Can you show a screenshot of the addon-ftrack and addon-kitsu to compare how they handle it?

@jakubjezek001
Copy link
Member

You you have a chance @fabiaserra please resolve conflicts and lets merge it ;)

@fabiaserra
Copy link
Contributor Author

You you have a chance @fabiaserra please resolve conflicts and lets merge it ;)

Done, please respond to https://github.com/ynput/ayon-shotgrid/pull/86/files/25d1b7fdf244e229a21bb404ddbe48bea0192f13#r1565979186

@jakubjezek001 jakubjezek001 added the type: feature Adding something new and exciting to the product label Apr 24, 2024
@BigRoy
Copy link
Contributor

BigRoy commented Jul 17, 2024

@jakubjezek001 the ball is in your court - can we merge this?

@jakubjezek001
Copy link
Member

Also @fabiaserra would you mind to merge latest develop into your branch? Thank you.

@fabiaserra
Copy link
Contributor Author

Also @fabiaserra would you mind to merge latest develop into your branch? Thank you.

You did it no?

@jakubjezek001
Copy link
Member

Also @fabiaserra would you mind to merge latest develop into your branch? Thank you.

You did it no?

Yeah I have done it - didn't know I can do it myself ;)

frontend/dist/shotgrid-addon.js Outdated Show resolved Hide resolved
Comment on lines 240 to 258
const createNewUserInAyon = async (login, email, name) => {
call_result_paragraph = document.getElementById("call-result");

response = await ayonAPI
.put("/api/users/" + login, {
"active": true,
"attrib": {
"fullName": name,
"email": email,
},
"password": login,
})
.then((result) => result)
.catch((error) => {
console.log("Unable to create user in AYON!")
console.log(error)
call_result_paragraph.innerHTML = `Unable to create user in AYON! ${error}`
});
}
Copy link
Member

Choose a reason for hiding this comment

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

it seems this api call is obsolete or I am not sure why is the console erroring it

image

@martastain can you have a look into this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm yeah I'm not sure, my syncs still work fine, I just synced a few more differences I had on our fork

Copy link
Member

Choose a reason for hiding this comment

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

So some changes needed to be done so we could merge this. Please review following PR on your fork @fabiaserra fabiaserra#1. This should probably interfere with your user base since we are only normalizing users which are having email in login. Also I had removed the password and active flag since it is active by default. The original SG login was added into user.data.sg_login.

@fabiaserra fabiaserra requested a review from jakubjezek001 July 30, 2024 14:54
@BigRoy BigRoy added the community Issues and PRs coming from the community members label Jul 30, 2024
@fabiaserra
Copy link
Contributor Author

FYI I recently just finished adding accessGroups synchronization in our fork of ayon-shotgrid: https://github.com/fabiaserra/ayon-shotgrid/blob/release/alkemyx/frontend/dist/shotgrid-addon.js#L181 it has some assumptions that only would work in our studio at the moment such as the Flow groups being admin, management, executive, lead and artist but perhaps you can get inspiration on abstracting that away on the settings

@jakubjezek001 jakubjezek001 mentioned this pull request Oct 3, 2024
7 tasks
@fabiaserra
Copy link
Contributor Author

FYI I recently just finished adding accessGroups synchronization in our fork of ayon-shotgrid: https://github.com/fabiaserra/ayon-shotgrid/blob/release/alkemyx/frontend/dist/shotgrid-addon.js#L181 it has some assumptions that only would work in our studio at the moment such as the Flow groups being admin, management, executive, lead and artist but perhaps you can get inspiration on abstracting that away on the settings

As I commented on this issue #130 (comment) I have expanded the sync users functionality so it's now driven by the Shotgrid events when there's a user assigned to a project, making the Sync Users not that important anymore, although still useful to have it to force an update and create the users even if they are not assigned to any project

@kalisp kalisp self-requested a review October 4, 2024 11:38
@kalisp
Copy link
Member

kalisp commented Oct 7, 2024

I am wondering if we need this PR or we should just move to syncing users via regular event system. Having separate javascript logic just for that seems weird.

@fabiaserra
Copy link
Contributor Author

I am wondering if we need this PR or we should just move to syncing users via regular event system. Having separate javascript logic just for that seems weird.

yeah as I said it still has a small use case for forcing the sync of users without the need for an event, in the case where users don't have a project assigned yet. It's very few cases where that could happen but it also helps for debugging purposes so it doesn't hurt to have it. On any case, I have both workflows implemented on my fork already

@jakubjezek001 jakubjezek001 self-assigned this Nov 29, 2024
- Added user ID to the new user creation function.
- Enhanced login validation by sanitizing input and ensuring it meets specific patterns.
- Updated API call to use validated login format.
@jakubjezek001
Copy link
Member

This PR needs to be merged before we proceed into #113 . Some enhancements needed to be added so we could merge it. Validation of login string for example and storing SG user ID number in User entity data key as sg_user_id. This way we should be able to be able to sync statuses based on SG user id rather then on login or email.

Copy link
Member

@kalisp kalisp left a comment

Choose a reason for hiding this comment

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

Users are created. There is no real logging/notification they got synced, but I guess for ad-hoc syncing it should be enough.

@jakubjezek001 jakubjezek001 merged commit bc21435 into ynput:develop Dec 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Issues and PRs coming from the community members type: feature Adding something new and exciting to the product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants