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

[Acknowledgements] Fix for selecting all roles #9209

Merged

Conversation

skarya22
Copy link
Contributor

Brief summary of changes

  • Fixed the main issue by changing from insert to insertIgnore. With all the roles the string was just a tad too long for the regular insert.
  • I also added a fix for not including "undefined" as a Role, Affiliation or Degree when the blank spot is selected

image

Testing instructions (if applicable)

  1. Try adding an acknowledgement with different values for degrees, roles, and other fields
  2. Confirm it adds as expected

Link(s) to related issue(s)

@laemtl laemtl self-requested a review April 18, 2024 12:26
Copy link
Contributor

@laemtl laemtl left a comment

Choose a reason for hiding this comment

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

Tested and main fix works, all roles are saved. The second fix to remove the undefined value not work though, undefined is still present:
Screenshot 2024-04-18 at 8 42 54 AM
I wonder if the cleanup should be done on the frontend before it passes undefined.

I'm also curious about the reason insertIgnore vs insert fix the issue. @ridz1208, idea?

@laemtl laemtl self-requested a review April 18, 2024 12:28
Copy link
Contributor

@laemtl laemtl left a comment

Choose a reason for hiding this comment

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

See my comment above

'affiliations' => $affiliations,
'degrees' => $degrees,
'roles' => $roles,
'affiliations' => implode(',', array_filter(explode(',', $affils))),
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not do the array_filter before the implode on line 96/99/102?

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 think since the fields are strings and not arrays, it skips lines 96, 99, and 102. I didn't want to get rid of that part though as I don't know if there was some other reason for it. By doing it later it makes sure to do it for all cases without duplicating the codes on 96 and 97, 99 and 100, and 102 and 103
image

@skarya22
Copy link
Contributor Author

Looks like this was caused by all the roles selected = character count > 258, and the sql column can only handle up till 255

@skarya22
Copy link
Contributor Author

@laemtl turns out it worked as insertIgnore was ignoring the fact that it couldn't insert a value > than the sql table can handle. Definitely not the correct fix so I am re-doing it

@skarya22 skarya22 requested review from laemtl and driusan April 18, 2024 15:06
@skarya22 skarya22 removed their assignment Apr 18, 2024
skarya22

This comment was marked as off-topic.

@skarya22 skarya22 requested a review from laemtl April 19, 2024 12:21
@driusan driusan merged commit 2167094 into aces:main Apr 19, 2024
28 checks passed
@ridz1208 ridz1208 added this to the 26.0.0 milestone Jun 6, 2024
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.

[acknowledgements] 500 Error when all roles are selected when adding an entry
4 participants