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

#1269 Resolved the bug to show correct {{eventName}} on compose message page #1275

Conversation

ved-asole
Copy link
Contributor

Resolved the bug #1269 to show show correct {{eventName}} on compose message page

@ved-asole
Copy link
Contributor Author

@cbellone - Could you please approve the above bug fix for the bug #1269?

@cbellone
Copy link
Member

Hi @ved-asole thanks for the modification.

I am afraid this does not solve the issue completely. You have to make sure that event.displayName is also used in the actual email being sent (see original bug).

If you're testing locally, you can see the email output on the console and check if the output is the expected one. Feel free to create an (unit) test case for this. It would be highly appreciated

@ved-asole
Copy link
Contributor Author

Hi @cbellone, sure. I will also check the actual email.

@ved-asole
Copy link
Contributor Author

Hi @cbellone, I have updated the code to sent the correct eventName in the actual email also

@ved-asole
Copy link
Contributor Author

Hi @cbellone, Could you please approve the above pull request?

@cbellone
Copy link
Member

I can't approve the pull request because I don't think it solves the issue.
I don't know what you're trying to do here but you shouldn't be supposed to change select statements for solving this issue.

Have you tested the application and verified that your fix is working? Because I'd be very surprised if it did...

@ved-asole
Copy link
Contributor Author

ved-asole commented Sep 19, 2023

Hi @cbellone, so what happened is that at first the angular ui was showing the wrong value for eventName. When I corrected that then that value was sent in api call to the backend and in backend it was checking for that value with the DB for short name whereas it should have checked for displayName. So I updated the select statement to check for display Name also as that method is used by other functions so I can't remove the check for shortName.

Please find below the screenshots where it works for me :

Email_Issue_Resolution

image

@ved-asole
Copy link
Contributor Author

I can't approve the pull request because I don't think it solves the issue. I don't know what you're trying to do here but you shouldn't be supposed to change select statements for solving this issue.

Have you tested the application and verified that your fix is working? Because I'd be very surprised if it did...

If you want, I can created a new method for the same to get display Name rather than changing the select statement because that method is used by around 4 other functions to get the short Name so I can't remove short name also.

@cbellone
Copy link
Member

OK, thanks for your detailed explanation. I now understand what's wrong and what caused the query modification.
You have modified a $scope property which is then sent to the server.

find an event by display_name is not a good idea. Two events can have the same display name (for whatever reason) but they will always have a different short_name.

I'd suggest you to:

  1. revert the modification on the admin-application.js file
  2. revert the modification on the EventRepository
  3. find a better way to display the right value. I.e. adding an additional property in the scope ($scope.eventDisplayName?) with the right value to display
  4. find the right place in the HTML template / JS code where we use $scope.eventName and we should use $scope.eventDisplayName instead
  5. test and see if this solves the issue

@ved-asole
Copy link
Contributor Author

Sure, I'm on it

@ved-asole ved-asole force-pushed the bugfix/1269-wrong-eventName-display-on-custom-message branch from c51d8c1 to 00fb07b Compare September 20, 2023 22:49
@ved-asole
Copy link
Contributor Author

Hi @cbellone, I have updated the code to show the display name on the Angular UI screen and updated the backend code to get the displayName while sending the email. Please approve the PR.

@ved-asole
Copy link
Contributor Author

Hi @cbellone, could you please review the pull request and approve it?

@cbellone cbellone self-requested a review September 22, 2023 15:56
@cbellone cbellone merged commit 78cd747 into alfio-event:2.0-M4-maintenance Sep 22, 2023
4 of 6 checks passed
@cbellone
Copy link
Member

Approved and merged. Thank you!

@ved-asole
Copy link
Contributor Author

Thanks @cbellone for giving me this opportunity to contribute to the project 😊

@ved-asole ved-asole deleted the bugfix/1269-wrong-eventName-display-on-custom-message branch September 22, 2023 18:32
@ved-asole ved-asole restored the bugfix/1269-wrong-eventName-display-on-custom-message branch September 25, 2023 14:07
@ved-asole
Copy link
Contributor Author

Hi @cbellone, just had a small doubt. In the repository contributors section my id/name is not being shown. Is it some glitch or I have not followed the proper procedure to contribute to the repo?

@ved-asole
Copy link
Contributor Author

Hi @cbellone, could you please suggest on the above issue?

@ved-asole
Copy link
Contributor Author

Hi @cbellone , could you please suggest why my name is not coming in collaborators list?

@cbellone
Copy link
Member

Hi, now you should be in the collaborators list.

@ved-asole
Copy link
Contributor Author

Thanks @cbellone, looking forward to on more projects working with you 😄

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.

None yet

2 participants