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

[TEST ONLY]Invitation table view #382

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[TEST ONLY]Invitation table view #382

wants to merge 1 commit into from

Conversation

ipula
Copy link
Contributor

@ipula ipula commented Jul 15, 2024

No description provided.

@ipula ipula changed the title [TEST ONLY]Invitation landing page [TEST ONLY]Invitation table view Jul 19, 2024
@ipula ipula force-pushed the invitation-landing-page branch 4 times, most recently from 10f08af to 5af9570 Compare July 26, 2024 15:51
@ipula ipula force-pushed the invitation-landing-page branch 7 times, most recently from 50d2e79 to e2b7aab Compare August 12, 2024 21:46
@ipula ipula force-pushed the invitation-landing-page branch 7 times, most recently from 2b83095 to 23647b7 Compare August 20, 2024 12:58
@ipula ipula force-pushed the invitation-landing-page branch 2 times, most recently from 6ff6df5 to 8b7aa7a Compare September 3, 2024 18:20
@withanage
Copy link
Member

withanage commented Sep 19, 2024 •
edited
Loading

@ipula may be try to fix the tests also here.

You may need to rebase ain fully. i would do that after going through the review change requests

@ipula ipula force-pushed the invitation-landing-page branch 7 times, most recently from 6f8fe87 to 2cd30b0 Compare September 25, 2024 16:38
jardakotesovec
jardakotesovec requested changes Sep 25, 2024
View reviewed changes
Copy link
Collaborator

@jardakotesovec jardakotesovec left a comment

Choose a reason for hiding this comment

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

Hi Ipula,
I did review of InvitationManager. The others are still for me to pick up next days.

Thanks!

src/managers/InvitationManager/InvitationManager.vue Outdated Show resolved Hide resolved
src/managers/InvitationManager/InvitationManager.vue Outdated Show resolved Hide resolved
src/managers/InvitationManager/InvitationManager.vue Outdated Show resolved Hide resolved
src/managers/InvitationManager/InvitationManager.vue Outdated Show resolved Hide resolved
src/managers/InvitationManager/InvitationManager.vue Outdated Show resolved Hide resolved
src/managers/InvitationManager/InvitationManagerStore.js Outdated Show resolved Hide resolved
src/managers/InvitationManager/InvitationManagerStore.js Outdated Show resolved Hide resolved
src/managers/InvitationManager/InvitationManagerStore.js Outdated Show resolved Hide resolved
src/managers/InvitationManager/InvitationManagerStore.js Outdated Show resolved Hide resolved
src/managers/InvitationManager/InvitationManager.vue Outdated Show resolved Hide resolved
@ipula ipula force-pushed the invitation-landing-page branch 4 times, most recently from be1ec67 to 8852037 Compare September 26, 2024 10:53
@withanage withanage mentioned this pull request Sep 27, 2024
Closed
2 tasks
@ipula ipula force-pushed the invitation-landing-page branch 8 times, most recently from 0b8df10 to ce7ddc4 Compare October 2, 2024 17:13
jardakotesovec
jardakotesovec requested changes Oct 3, 2024
View reviewed changes
Copy link
Collaborator

@jardakotesovec jardakotesovec left a comment

Choose a reason for hiding this comment

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

I went through the UserInvitationManager and UserInvitationPage

There is good progress - lets keep going.

I still have AcceptInvitation to go through - but there is probably enough feedback for this week.

src/managers/UserInvitationManager/UserInvitationManager.vue Outdated Show resolved Hide resolved
src/managers/UserInvitationManager/UserInvitationManager.vue Outdated Show resolved Hide resolved
src/managers/UserInvitationManager/UserInvitationManagerStore.js Show resolved Hide resolved
src/pages/userInvitation/UserInvitationUserGroupsTable.vue Show resolved Hide resolved
src/managers/UserInvitationManager/UserInvitationManagerStore.js Show resolved Hide resolved
src/pages/userInvitation/UserInvitationSearchFormStep.vue
);
if (!user) {
store.updatePayload('inviteeEmail', fields.value.email);
store.userSearch.message = t('userInvitation.search.userNotFound');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me check with Devika - maybe we could indicate this directly on UserInvitationDetailsFormStep based on whether userId exist - I think that would be cleaner.

src/pages/userInvitation/UserInvitationSearchFormStep.vue Show resolved Hide resolved
src/pages/userInvitation/UserInvitationDetailsFormStep.vue
function updateUserForm(a, form, c, d) {
set(a, form, c, d);
if (form.fields) {
form.fields.forEach((field) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please explain (probably via comments) what this logic is aiming to achieve? I would hope there is opportunity to simplify it.

src/pages/userInvitation/UserInvitationDetailsFormStep.vue
if (!store.invitationPayload.userId) {
userForm.value.fields.forEach((field) => {
if (field.isMultilingual) {
store.updatePayload(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly here - it would deserve comments and check whether there is opportunity for simplification.

src/pages/userInvitation/UserInvitationDetailsFormStep.vue Outdated
errors[element] = {
...errors[element],
[data.key]: store.errors[element + '.' + data.key],
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please just create general function which converts from dot notation to nested object.

@withanage withanage mentioned this pull request Oct 3, 2024
Open
8 tasks
jardakotesovec
jardakotesovec reviewed Oct 3, 2024
View reviewed changes
src/pages/acceptInvitation/AcceptInvitationUserAccountDetails.vue
@@ -3,7 +3,7 @@
<div class="py-1">
<FormDisplayItemBasic
heading-element="h4"
:heading="t('user.emailAddress')"
:heading="t('user.email')"
:value="store.email"
></FormDisplayItemBasic>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Below I see multiple FieldText - would be possible to have these configured through the form as usual, rather than just individual Fields here?

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 will check this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asmecher asmecher asmecher left review comments

@jardakotesovec jardakotesovec jardakotesovec requested changes

@withanage withanage withanage 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 @withanage @asmecher @jardakotesovec