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.

Sign up for GitHub

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

Jump to bottom

pkp/pkp-lib#9658 user access table #10576

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

pkp/pkp-lib#9658 user access table #10576

wants to merge 4 commits into from
+119 −5

Conversation

ipula
Copy link
Contributor

@ipula ipula commented Nov 1, 2024 •
edited
Loading

@ipula ipula mentioned this pull request Nov 1, 2024
Open
@ipula ipula force-pushed the ipula/9658-user-table branch from 28a1316 to cd6e544 Compare November 18, 2024 11:02
@asmecher
Copy link
Member

@defstat, could you review this one?

@defstat
Copy link
Contributor

defstat commented Nov 19, 2024

@ipula is this accompanied with a ui-library PR?

@ipula
Copy link
Contributor Author

ipula commented Nov 19, 2024

@ipula is this accompanied with a ui-library PR?
Yes pkp/ui-library#437

@withanage withanage mentioned this pull request Nov 26, 2024
Closed
@withanage withanage requested a review from defstat November 28, 2024 16:05
defstat
defstat requested changes Dec 18, 2024
View reviewed changes
Copy link
Contributor

@defstat defstat left a comment

Choose a reason for hiding this comment

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

@ipula thanks, just one question on this, otherwise I think it is ok

pages/invitation/InvitationHandler.php
public function editUser($args, $request): void
{
$invitation = null;
$invitationPayload =[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if you can use the already existing UserRoleAssignmentInvitePayload object here, in order to avoid the many key strings.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked but some values are different. For example I need orcid but in UserRoleAssignmentInvitePayload has userOrcid also same with the country.

Copy link
Member

Choose a reason for hiding this comment

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

@defstat , if you are ok with @ipula s explanation, I think we can mark it as ready for merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

@withanage, @ipula if we can't change those variable names in the front-end, we could go forward with merging it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@defstat Issue like we are using same user invitation components. I have to change the code that already QA and merged

@ipula ipula force-pushed the ipula/9658-user-table branch from 95e0772 to 485b32b Compare January 7, 2025 13:59
@ipula ipula force-pushed the ipula/9658-user-table branch 2 times, most recently from 56c6645 to 320171b Compare January 9, 2025 15:33
@ipula ipula force-pushed the ipula/9658-user-table branch from 320171b to 4f50264 Compare January 13, 2025 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@withanage withanage withanage left review comments

@defstat defstat defstat requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ipula @asmecher @defstat @withanage