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. |
objecttothis
left a comment
objecttothis
left a comment
There was a problem hiding this comment.
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.
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.
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.
@DamianoSilverhand still motivated to drive this one home?
There was a problem hiding this comment.
@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.
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.
you're right, we need to fix that first so this change can be implemented correctly
There was a problem hiding this comment.
@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.
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.
jekkos
force-pushed
the
master
branch
4 times, most recently
from
fddedb8 to
6d106d6
Compare
jekkos
force-pushed
the
master
branch
7 times, most recently
from
32cd824 to
cce2c1c
Compare
jekkos
force-pushed
the
master
branch
3 times, most recently
from
2772920 to
0f3175b
Compare
|
@DamianoSilverhand now that CI4 has been merged into the master can you rebase your changes so we can proceed? |
4 participants
This PR adds person attributes on Customers, Suppliers and Employees. Also includes person attributes in customer import using csv.