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

[57] Edit participant component (fixes#57) #353

Merged

Conversation

stanislawK
Copy link
Contributor

Story / Bug id:

57

Description:

Edit participant form added

Migrations:

N/A

New imports / dependencies:

N/A

What tests do I need to run to validate this change:

Check both form's functionalities: create and edit participant.

@OtisRed OtisRed self-requested a review February 3, 2021 18:09
@OtisRed
Copy link
Contributor

OtisRed commented Feb 24, 2021

Ok, so far I only clicked it out and regarding its main function it seems to work

I've found few issues with messages though:

  1. Surname field when left (either by tab or mouse click) there is this notification "This last name already exists. Please verify if a person is actually a new paricipant" - which does not make much sense in the context of editing details.
    Zrzut ekranu z 2021-02-24 22-36-52

  2. The same goes for slack field - but with respective message.

  3. The bahavior described in point 2 happens in the slack field even when its empty - it reads empty string and given that other participants also have empty field it returns the message: "This slack nick already exists. Please suggest change of the nick to avoid confusion on Slack" - maybe we should use .find differently not to capture empty strings as repeated value?
    Zrzut ekranu z 2021-02-24 22-36-40

  4. The message about existing field also happens when you leave phone field.
    Zrzut ekranu z 2021-02-24 22-37-11

So to sum up points 1-4
we show messages that don't make sense in the context of editing participant but I don't have a good solution what to do with it, because in some cases it might be useful e.g. I decided to edit some details because I think that I confused two people and then if there is already somebody with the same last name, slack or phone then I might realize that I actually didn't confused anybody and they are indeed two different people. However it took me a while to figure out a use case for this, so maybe they are marginal?

And few more with the pre-population:
5.When I choose a participant and touch any of the fields both in edit and create, then I get an alert if "changes should be discarded" - even when I didn't change anything. So its enough to touch to get this msg even when you don't change anything which could be confusing.
Zrzut ekranu z 2021-02-24 22-43-35

  1. When I choose participant in edit and don't touch any of the fields
    Zrzut ekranu z 2021-02-24 22-43-12

and then switch to create, the data of chosen participant in edit mode stay and prepopulate the create form:
Zrzut ekranu z 2021-02-24 22-43-15

  1. When I choose a participant in edit and touch any of the fields:
    Zrzut ekranu z 2021-02-24 22-44-03

Then move to create tab, confirm "discard changes" and then go back again to edit form the field "choose participant" keeps chosen record and shows that participant, but without filling any data
Zrzut ekranu z 2021-02-24 22-44-16

So to sum up
There are two kinds of problems - one with messages and one with populating data in different use cases. We can discuss it via videocall. We can also skip them for now, pull the branch and create tickets to deal with these issues later on (this is especially viable option if the problem is at the beginner level and we can make tasks for juniors out of them).

@OtisRed
Copy link
Contributor

OtisRed commented Mar 10, 2021

Ok, after manual check only point 5 seems to be still in power. I mean that still if I choose any participant to edit, don't touch any of the fields and then try to move to the "create" tab it still asks if I want to discard changes even though I didn't make any. I think we can live with that but if it's not much effort then it may be worth to pursue it. Unless it's not then let's ignore it for the moment.

Just a manual remark before I go to the code.

@OtisRed
Copy link
Contributor

OtisRed commented Mar 10, 2021

One more thing - very low priority.
If you edit participant github, while having open hacknight list with that participant the github nick doesn't change - you need to pick the same hacknight again and only then details get reloaded. I don't believe its must have - just due dilligance on my part but we can absolutely ignore this.

Copy link
Contributor

@OtisRed OtisRed left a comment

Choose a reason for hiding this comment

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

I tested PR manually - two comments above.

Besides that I have only one comment. I understand what happens here but at some points this code is beyond my level so I just trust it based on the fact that it works.

@stanislawK please merge when you're satisfied with your work - my comments are minor so you can try to work with them or ignore them to push this PR through.

If you feel that any comment is relevant but you don't feel like tackling them at the moment please merge PR and create tickets so we have some minor tasks for beginners :)

Comment on lines +303 to +306
formIsNotEmpty() {
return !Object.values(this.$v.form.$model).every(
x => x === null || x === '' || x === undefined
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we adjust this method with some kind condition that will execute this function only if any field is actually changed somehow? Right now we prompt "discard changes" even if no changes were made and I think this is the place where it happens

frontend/src/components/Participants.vue Outdated Show resolved Hide resolved
@KSodowska
Copy link

KSodowska commented Mar 22, 2021

Hi, I've been doing some 'testing' and for start I've got some minor suggestions about participant data:

  • should be presented in alphabetical order – saves time spent on searching,
  • you can add a participant, but you can’t delete them after they‘re already on a list (maybe it should be like this).

participants

@KSodowska
Copy link

KSodowska commented Mar 22, 2021

  • Adding a participant with a name longer than 12 characters cuts-off ‘x’ button, so it's very hard or impossible to remove a participant.
    (Firefox browser)

x+button

@KSodowska
Copy link

Clicking on ‘Edit participant’ cleans his name and all of his data (not permanently, it's more of a usability issue).

  1. Click on ‘Edit existing participant’.
  2. Choose a participant.
  3. His personal data are already in ‘edit -mode’ and you are able to make changes (you don’t need to edit them by clicking ‘Edit participant’).
  4. Click on the button ‘Edit participant’ and all of the data is gone and field ‘Select participant’ is empty. And there is no reason for that, or maybe I don't understand this feature :)

Suggestion: Maybe we should add a „Save changes” button instead of „Edit participant” button. When you insert some changes in a participant data, clicking ‘Edit participant’ is a bit confusing. Also pop-up text after saving changes would need to be something like „Participant data have been successfully changed” instead of „Participant has been successfully edited”.

editing_participant

Copy link

@KSodowska KSodowska left a comment

Choose a reason for hiding this comment

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

Hi, sorting works in a participants form (I understand that sorting in a hacknight participants list is up for separate commit) :)

And I can see that the button 'Save changes' has been added, it's clear now when you save changes 👍

obraz

@KSodowska
Copy link

Hi, one more thing.

Pop-up error message when wanting to add a non-existing participant - changing it to be more clear.

  1. I suggest we change it for ex. "This participant does not exist in our database. You can create his account using "Create new participant" form."
  2. Also this pop-up does not disappear when you engage in other action, you have to click on 'x' button. But it's not that important.

Error message_none_existing_partc

@stanislawK
Copy link
Contributor Author

Hi, one more thing.

Pop-up error message when wanting to add a non-existing participant - changing it to be more clear.

1. I suggest we change it for ex. "This participant does not exist in our database. You can create his account using "Create new participant" form."

2. Also this pop-up does not disappear when you engage in other action, you have to click on 'x' button. But it's not that important.

Error message_none_existing_partc

That is valid point, but issue itself is broader, and we had ticket for that. Basically error messages from backend could have slightly different format and nestings, Therefore we are not able to extract those information in unified way at frontend.

@OtisRed OtisRed merged commit 5b03fb7 into CodeForPoznan:master Apr 14, 2021
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.

3 participants