-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Person attributes #3161
base: master
Are you sure you want to change the base?
Person attributes #3161
Conversation
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
We'll rebase / review again after next changes for item import of @objecttothis are integrated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't do a full review. Just the one change that I noted.
$failCodes[] = $i; | ||
$failed_row = $i+1; | ||
$failCodes[] = $failed_row; | ||
log_message("ERROR","CSV Item import failed on line ". $failed_row .". This customer was not imported."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line needs to change. I think it probably needs to read something like log_message("ERROR","CSV Customer import failed on line ". $failed_row .". This customer was not imported.");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if you aren't using string interpolation you need to use ' not ". " calls the parser unnecessarily when there are no variables to parse. That said, this line would be better written with string interpolation:
log_message("ERROR","CSV Customer import failed on line $failed_row. This customer was not imported.");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DamianoSilverhand still motivated to drive this one home?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jekkos if you can get the Person Attributes to show in the table view he might get interested in it again. LOL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have cloned and rebased the branch and pushed it to the organisation's repo now.. let's see what changes are in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, we need to fix that first so this change can be implemented correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jekkos I downloaded this version but Now I am unable to update an employee. The Dev server does the same. Unable to update Employee. I can create a new Employee. The Demo server also does not allow the Employee to be updated and I am getting no errors with errors set to 1 . I narrowed it down to the following commit. Check username before employee creation (#3239)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks for testing, I'll have look later, I probably missed something there. It might have been better to implement the check as a jQuery validation instead.
fddedb8
to
6d106d6
Compare
32cd824
to
cce2c1c
Compare
2772920
to
0f3175b
Compare
This PR adds person attributes on Customers, Suppliers and Employees. Also includes person attributes in customer import using csv.