• Hi All

    Please note that at the Chandoo.org Forums there is Zero Tolerance to Spam

    Post Spam and you Will Be Deleted as a User

    Hui...

  • When starting a new post, to receive a quicker and more targeted answer, Please include a sample file in the initial post.

VBA userform not updating to worksheet when save using new code

Y

Yodelayheewho

Member
I'm new to VBA. The following code works. When I click on the command button, it saves my entries to the "Master" worksheet.
Code: 
'Update and Save Button'
Private Sub cmbUpdate_Click()
On Error Resume Next

Dim Shop_Order_Number As String
Shop_Order_Number = Trim(txtShopOrdNum)
lastrow = Worksheets("Master").Cells(Rows.Count, 1).End(xlUp).Row
For i = 2 To lastrow
'CDbl(txtText.Value) is needed for currency, does not work for percent

If Worksheets("Master").Cells(i, 4).Value = Shop_Order_Number Then
Worksheets("Master").Cells(i, 1).Value = txtPrefix
Worksheets("Master").Cells(i, 2).Value = cboStatus
Worksheets("Master").Cells(i, 3).Value = txtSuffix
Worksheets("Master").Cells(i, 4).Value = txtShopOrdNum
Worksheets("Master").Cells(i, 5).Value = txtEmailSubLine
Worksheets("Master").Cells(i, 6).Value = txtNotes
Worksheets("Master").Cells(i, 7).Value = cboStage
Worksheets("Master").Cells(i, 8).Value = CDate(Me.txtStartDate.Value)
Worksheets("Master").Cells(i, 9).Value = CDate(Me.txtStageDue.Value)
Worksheets("Master").Cells(i, 10).Value = CDate(Me.txtEndDate.Value)
Worksheets("Master").Cells(i, 11).Value = txtDays
Worksheets("Master").Cells(i, 12).Value = cboProcess
Worksheets("Master").Cells(i, 13).Value = txtPropNum
Worksheets("Master").Cells(i, 14).Value = cboSalesPer
Worksheets("Master").Cells(i, 15).Value = txtPrimSalesTerr
Worksheets("Master").Cells(i, 16).Value = CDate(Me.txtPropDate.Value)
Worksheets("Master").Cells(i, 17).Value = txtLT
Worksheets("Master").Cells(i, 18).Value = CDate(Me.txtPromDate.Value)
Worksheets("Master").Cells(i, 19).Value = CDate(Me.txtExpDate.Value)
Worksheets("Master").Cells(i, 20).Value = CDbl(Me.txtCost.Value)
'redacted columns 21-102'
End If
Next
ThisWorkbook.Save
MsgBox ("Your work is saved")
End Sub

However, I understand there is a better way to do this that is more efficient. Based on some research, I came up with this code and can't get it to work.
Code: 
'Update and Save Button'
Private Sub cmbUpdate_Click()
On Error Resume Next

Dim son As String
son = Trim(txtShopOrdNum)
Dim ws As Worksheet
Set ws = Worksheets("Master")
lastrow = ws.Cells(Rows.Count, 1).End(xlUp).Row
For i = 2 To lastrow
'CDbl(txtText.Value) is needed for currency, does not work for percent
If ws.Cells(i, 4).Value = Shop_Order_Number Then
    ws.Cells(i, 1).Value = txtPrefix
    ws.Cells(i, 2).Value = cboStatus
    ws.Cells(i, 3).Value = txtSuffix
    ws.Cells(i, 4).Value = txtShopOrdNum
    ws.Cells(i, 5).Value = txtEmailSubLine
    ws.Cells(i, 6).Value = txtNotes
    ws.Cells(i, 7).Value = cboStage
    ws.Cells(i, 8).Value = CDate(Me.txtStartDate.Value)
    ws.Cells(i, 9).Value = CDate(Me.txtStageDue.Value)
    ws.Cells(i, 10).Value = CDate(Me.txtEndDate.Value)
    ws.Cells(i, 11).Value = txtDays
    ws.Cells(i, 12).Value = cboProcess
    ws.Cells(i, 13).Value = txtPropNum
    ws.Cells(i, 14).Value = cboSalesPer
    ws.Cells(i, 15).Value = txtPrimSalesTerr
    ws.Cells(i, 16).Value = CDate(Me.txtPropDate.Value)
    ws.Cells(i, 17).Value = txtLT
    ws.Cells(i, 18).Value = CDate(Me.txtPromDate.Value)
    ws.Cells(i, 19).Value = CDate(Me.txtExpDate.Value)
    ws.Cells(i, 20).Value = CDbl(Me.txtCost.Value)
    'redacted columns 21-102'
End If
Next
ThisWorkbook.Save
MsgBox ("Your work is saved")
End Sub

I would appreciate your insight.
 
No more efficient, just less to write for the lazy …​
A more efficient way is to use an array for contiguous cells in order to write all at once.​
Why On Error codeline ?! Remove it …​
Any logic reason to duplicate form data to several worksheet rows ?!​
Follow the process in step-by-step mode and check the variables contents …​
I would use a With block on the worksheet CodeName rather than an useless worksheet variable.​
​
 
The two are functionally the same, so what exactly does "I can't get it to work" actually mean?
 
Debaser said:
The two are functionally the same, so what exactly does "I can't get it to work" actually mean?
Click to expand...
When testing, I changed an entry from "Waiting for Approval" to "Standby", clicked the Save command button. The code runs. I get no error message, but the value did not change to "Standby".
 
Marc L said:
No more efficient, just less to write for the lazy …​
A more efficient way is to use an array for contiguous cells in order to write all at once.​
Why On Error codeline ?! Remove it …​
Any logic reason to duplicate form data to several worksheet rows ?!​
Follow the process in step-by-step mode and check the variables contents …​
I would use a With block on the worksheet CodeName rather than an useless worksheet variable.​
​
Click to expand...
Hi Marc,
Can you create an array if you have textboxes and combo boxes?

I do not know how to do step by step mode to check the variables.

Can you give me an example of what you think would work?
 
I've removed it and the first error that popped up was pointing to: CDate(Me.txtPropDate.Value). It is a run-time '13': Type mismatch.
From a little reading I've done, CDate does not like empty cells. The error occurs on those CDate...textboxes that are empty.

I need Excel to recognize these textboxes as dates so I can Pivot off the worksheet. I've made sure the worksheet columns are set in short date format and CDate works fine as long as they are not left empty. However, this is not realistic. Some date textboxes need to remain empty depending upon the situation.

I'm going to remove proprietary information so I can upload a sample.
 
Without an example.
See if this works for you.

Code: 
Private Sub cmbUpdate_Click()
With Sheets("Master")
iRow = .Cells.Find(What:="*", SearchOrder:=xlRows, SearchDirection:=xlPrevious, LookIn:=xlValues).Row + 1
.Cells(iRow, 1).Resize(, 20).Value = Array(txtPrefix, cboStatus, txtSuffix, txtShopOrdNum, txtEmailSubLine, txtNotes, cboStage, txtStartDate, txtStageDue, txtEndDate _
, txtDay, cboProcesss, txtPropNum, cboSalesPer, txtPrimSalesTerr, txtPropDat, txtLT, txtPromDat, txtExpDate, txtCost)
End With
ThisWorkbook.Save
MsgBox ("Your work is saved")
End Sub

and format the dates. (change the format to the one you want)

Code: 
Private Sub UserForm_Initialize()
txtStartDate = Format(txtStartDate, "dd/mm/yyyy")
txtStageDue = Format(txtStageDue, "dd/mm/yyyy")
txtEndDate = Format(txtEndDate, "dd/mm/yyyy")
txtPropDate = Format(txtPropDate, "dd/mm/yyyy")
txtPromDate = Format(txtPromDate, "dd/mm/yyyy")
txtExpDate = Format(txtExpDate, "dd/mm/yyyy")
txtCost = Format(txtCost, "dd/mm/yyyy")
End Sub
 
I'd add a function like:

Code: 
Function GetDate(inputText As String) As Variant
   If Len(inputText) <> 0 Then
      On Error Resume Next
      GetDate = CDate(inputText)
   End If
End Function

then use GetDate in place of CDate.

It seems unlikely to me that you should be looping here, unless you really do have multiple lines for the same order number and need to update them all?
 
Yodelayheewho said:
I'm new to VBA. The following code works. When I click on the command button, it saves my entries to the "Master" worksheet.
Click to expand...

Further to @Debaser's question, something about your original code does not make sense to me. If I set lastrow = 20 and then make a substitution:
Code: 
        If Worksheets("Master").Cells(i, 4).Value = Shop_Order_Number Then
            Worksheets("Master").Cells(i, 1).Value = 9991    ' txtPrefix
            Worksheets("Master").Cells(i, 2).Value = 9992    ' cboStatus
            Worksheets("Master").Cells(i, 3).Value = 9993    ' txtSuffix
            Worksheets("Master").Cells(i, 4).Value = 9994    ' txtShopOrdNum
            Worksheets("Master").Cells(i, 5).Value = 9995    ' txtEmailSubLine
            Worksheets("Master").Cells(i, 6).Value = 9996    ' txtNotes
            Worksheets("Master").Cells(i, 7).Value = 9997    ' cboStage
            Worksheets("Master").Cells(i, 8).Value = 9998    ' CDate(Me.txtStartDate.Value)
            Worksheets("Master").Cells(i, 9).Value = 9999    ' CDate(Me.txtStageDue.Value)
            Worksheets("Master").Cells(i, 10).Value = 99910    ' CDate(Me.txtEndDate.Value)
            Worksheets("Master").Cells(i, 11).Value = 99911    ' txtDays
            Worksheets("Master").Cells(i, 12).Value = 99912    ' cboProcess
            Worksheets("Master").Cells(i, 13).Value = 99913    ' txtPropNum
            Worksheets("Master").Cells(i, 14).Value = 99914    ' cboSalesPer
            Worksheets("Master").Cells(i, 15).Value = 99915    ' txtPrimSalesTerr
            Worksheets("Master").Cells(i, 16).Value = 99916    ' CDate(Me.txtPropDate.Value)
            Worksheets("Master").Cells(i, 17).Value = 99917    ' txtLT
            Worksheets("Master").Cells(i, 18).Value = 99918    ' CDate(Me.txtPromDate.Value)
            Worksheets("Master").Cells(i, 19).Value = 99919    ' CDate(Me.txtExpDate.Value)
            Worksheets("Master").Cells(i, 20).Value = 99920    ' CDbl(Me.txtCost.Value)
            'redacted columns 21-102'
        End If
so I don't have to care about the contents of any of your userform controls, in order to just look at how the data is transferred to the worksheet. What I end up with is a block of cells, A2:T20 where each row is identical. Is that really your intent?

1689879549254.png
 
I really appreciate everyone's input and willingness to help me out. I was able to put together a non-proprietary sample file, so you can see everything. I hope this helps.
 

Attachments

  • TEST_Systems Order Entry Master_v9.xlsm
    508.4 KB · Views: 4
A few comments:
First, it is really bad practice to just put On Error Resume Next at the top of every routine and hope for the best. You should only use that in the most limited scope required.
Second, you are using a table on your Master sheet, so you should use that when reading writing to the sheet.
Third, using named ranges (or tables) rather than hardcoding ranges (e.g. [LISTS!C2:C6]) in the code will make this much easier to maintain.
Fourth, as suspected, you don't need to carry on looping after you find the row you update, so you could exit the loop there. In fact, you coud us Find or application.match to locate the row you want and simply update that without the loop at all. For example:

Code: 
   Dim Shop_Order_Number As String
   Shop_Order_Number = Trim(txtShopOrdNum)
'CDbl(txtText.Value) is needed for currency, does not work for percent
   Dim orderTable As ListObject
   Set orderTable = Worksheets("Master").ListObjects("tblMaster")
   Dim matchRow
   matchRow = Application.Match(Shop_Order_Number, orderTable.ListColumns("SHOP ORDER NUMBER").DataBodyRange, 0)
   If Not IsError(matchRow) Then
      With orderTable.ListRows(matchRow)
         .Range(1, 1).Value = txtPrefix
         .Range(1, 2).Value = cboStatus
         .Range(1, 3).Value = txtSuffix
         .Range(1, 4).Value = txtShopOrdNum
         .Range(1, 5).Value = txtEmailSubLine
         .Range(1, 6).Value = txtNotes
         .Range(1, 7).Value = cboStage
         .Range(1, 8).Value = GetDate(Me.txtStartDate.Value)
         .Range(1, 9).Value = GetDate(Me.txtStageDue.Value)
         .Range(1, 10).Value = GetDate(Me.txtEndDate.Value)
         .Range(1, 11).Value = txtDays
         .Range(1, 12).Value = cboProcess
         .Range(1, 13).Value = txtPropNum
         .Range(1, 14).Value = cboSalesPer
         .Range(1, 15).Value = txtPrimSalesTerr
         .Range(1, 16).Value = GetDate(Me.txtPropDate.Value)
         .Range(1, 17).Value = txtLT
         .Range(1, 18).Value = GetDate(Me.txtPromDate.Value)
         .Range(1, 19).Value = GetDate(Me.txtExpDate.Value)
         .Range(1, 20).Value = CDbl(Me.txtCost.Value)
         .Range(1, 21).Value = txtMargin
         .Range(1, 22).Value = txtPO
         .Range(1, 23).Value = GetDate(Me.txtPODate.Value)
         .Range(1, 24).Value = GetDate(Me.txtPOReGetDate.Value)
         .Range(1, 25).Value = CDbl(txtPOAmt.Value)
         .Range(1, 26).Value = cboTerms
         .Range(1, 27).Value = cboShipVia
         .Range(1, 28).Value = cboShipType
         .Range(1, 29).Value = cboShipChrgs
         .Range(1, 30).Value = txtShipInst
         .Range(1, 31).Value = txtSO
         .Range(1, 32).Value = txtQuote
         .Range(1, 33).Value = txtPM
         .Range(1, 34).Value = txtEE
         .Range(1, 35).Value = txtSys
         .Range(1, 36).Value = txtSCode
         .Range(1, 37).Value = txtBMTH
         .Range(1, 38).Value = txtTransOrdNum
         .Range(1, 39).Value = txtInstlDays
         .Range(1, 40).Value = txtStrtUpDays
         .Range(1, 41).Value = txtTrngDaysOn
         .Range(1, 42).Value = txtTrngDaysTol
         .Range(1, 43).Value = txtVndrSrvDays
         .Range(1, 44).Value = txtSrvTech
         .Range(1, 45).Value = CDbl(Me.txtStndHrs12)
         .Range(1, 46).Value = CDbl(Me.txtStndHrs3)
         .Range(1, 47).Value = CDbl(Me.txtSatSunHol)
         .Range(1, 48).Value = CDbl(Me.txtAddOvrTm)
         .Range(1, 49).Value = CDbl(Me.txtTrvlLess)
         .Range(1, 50).Value = CDbl(Me.txtTrvlMore)
         .Range(1, 51).Value = CDbl(Me.txtAirfare)
         .Range(1, 52).Value = CDbl(Me.txtHotel)
         .Range(1, 53).Value = CDbl(Me.txtCarRntl)
         .Range(1, 54).Value = CDbl(Me.txtMeals)
         .Range(1, 55).Value = CDbl(Me.txtMileage)
         .Range(1, 56).Value = CDbl(Me.txtParking)
         .Range(1, 57).Value = CDbl(Me.txtSrvPrts1)
         .Range(1, 58).Value = CDbl(Me.txtSrvPrts2)
         .Range(1, 59).Value = CDbl(Me.txtBkgFees)
         .Range(1, 60).Value = txtSummary
         .Range(1, 61).Value = CDbl(Me.txtSrvTtl)
         .Range(1, 62).Value = cboSrvGrp
         .Range(1, 63).Value = GetDate(Me.txtConPO.Value)
         .Range(1, 64).Value = GetDate(Me.txtE10.Value)
         .Range(1, 65).Value = GetDate(Me.txtFinance.Value)
         .Range(1, 66).Value = GetDate(Me.txtReqPM.Value)
         .Range(1, 67).Value = GetDate(Me.txtPMAssign.Value)
         .Range(1, 68).Value = GetDate(Me.txtSOATeam.Value)
         .Range(1, 69).Value = GetDate(Me.txtApproved.Value)
         .Range(1, 70).Value = GetDate(Me.txtSOACust.Value)
         .Range(1, 71).Value = GetDate(Me.txtSOAE10.Value)
         .Range(1, 72).Value = GetDate(Me.txtAR.Value)
         .Range(1, 73).Value = GetDate(Me.txtCurPromDate.Value)
         .Range(1, 74).Value = cboRecRev
         .Range(1, 75).Value = txtShipNotes
         .Range(1, 76).Value = GetDate(Me.txtShipped.Value)
         .Range(1, 77).Value = txtName
         .Range(1, 78).Value = txtDiamond
         .Range(1, 79).Value = txtCustID
         .Range(1, 80).Value = txtBT
         .Range(1, 81).Value = txtBTAddr1
         .Range(1, 82).Value = txtBTAddr2
         .Range(1, 83).Value = txtBTCity
         .Range(1, 84).Value = txtBTState
         .Range(1, 85).Value = txtBTZip
         .Range(1, 86).Value = txtBTCntry
         .Range(1, 87).Value = cboTE
         .Range(1, 88).Value = txtCon1
         .Range(1, 89).Value = txtEmail1
         .Range(1, 90).Value = txtCon2
         .Range(1, 91).Value = txtEmail2
         .Range(1, 92).Value = txtSTID
         .Range(1, 93).Value = txtSTName
         .Range(1, 94).Value = txtSTAddr1
         .Range(1, 95).Value = txtSTAddr2
         .Range(1, 96).Value = txtSTCity
         .Range(1, 97).Value = txtSTState
         .Range(1, 98).Value = txtSTZip
         .Range(1, 99).Value = txtSTCntry
         .Range(1, 100).Value = txtEUName
         .Range(1, 101).Value = txtEUID
         .Range(1, 102).Value = txtTimestamp
      End With
   End If
 
Success! I've attached an updated sample file.

First - I appreciate the warning re: On Error Resume Next. I'm still learning and appreciate your insight. I will be more careful.

Second - Makes sense to use the table reference

Third - RE: [LISTS!C2:C6] the named range is tblSrvGrp. Would it be written like this instead? cboSrvGrp.List = [tblSrvGrp].Value

Fourth - The code you provide is awesome. It works great and I'm working on the other command button codes to streamline them as well.

Date challenge: The following code works like a charm!
Debaser said:
I'd add a function like:

Code: 
Function GetDate(inputText As String) As Variant
   If Len(inputText) <> 0 Then
      On Error Resume Next
      GetDate = CDate(inputText)
   End If
End Function

then use GetDate in place of CDate.

It seems unlikely to me that you should be looping here, unless you really do have multiple lines for the same order number and need to update them all?
Click to expand...

However, I have 22 dates and attempted to edit the code accordingly...

Code: 
Function GetDate(Dates As String) As Variant
    Dates(7) = txtStartDate 'Compile error: expected array on "Dates(7)"
    Dates(8) = txtStageDue
    Dates(9) = txtEndDate
    Dates(15) = txtPropDate
    Dates(17) = txtPromDate
    Dates(18) = txtExpDate
    Dates(22) = txtPODate
    Dates(23) = txtPORecDate
    Dates(62) = txtConPO
    Dates(63) = txtE10
    Dates(64) = txtFinance
    Dates(65) = txtReqPM
    Dates(66) = txtPMAssign
    Dates(67) = txtSOATeam
    Dates(68) = txtApproved
    Dates(69) = txtSOACust
    Dates(70) = txtSOAE10
    Dates(71) = txtAR
    Dates(72) = txtCurPromDate
    Dates(73) = cboRecRev
    Dates(75) = txtShipped
    Dates(101) = txtTimestamp
If Len(Dates) <> 0 Then
      On Error Resume Next
      GetDate = CDate(Dates)
   End If
End Function

'Attempt #2 - added Dim Dates(22)
Function GetDate() As Variant
Dim Dates(22) As String
    Dates(7) = txtStartDate
    Dates(8) = txtStageDue
    Dates(9) = txtEndDate
    Dates(15) = txtPropDate
    Dates(17) = txtPromDate
    Dates(18) = txtExpDate
    Dates(22) = txtPODate
    Dates(23) = txtPORecDate
    Dates(62) = txtConPO
    Dates(63) = txtE10
    Dates(64) = txtFinance
    Dates(65) = txtReqPM
    Dates(66) = txtPMAssign
    Dates(67) = txtSOATeam
    Dates(68) = txtApproved
    Dates(69) = txtSOACust
    Dates(70) = txtSOAE10
    Dates(71) = txtAR
    Dates(72) = txtCurPromDate
    Dates(73) = cboRecRev
    Dates(75) = txtShipped
    Dates(101) = txtTimestamp
If Len(Dates) <> 0 Then 'Compile error: Variable required - can't assign to this expression
      On Error Resume Next
      GetDate = CDate(Dates)
   End If
End Function
 

Attachments

  • TEST_Systems Order Entry Master_v9_Debaser.xlsm
    544.7 KB · Views: 2
Last edited by a moderator:
The codeline has no sense on an array so the crash … What is it supposed to do ?​
And again remove the useless On Error codeline !​
​
 
@Marc L My newness to VBA is blaringly obvious. I assumed 'inputText' was a placeholder. I renamed it txtStartDate and assumed I needed to do that for all dates. Long story short, I found my error and figure it out that this code works on ALL dates as is.

The On Error Resume Next was in the code Debasser provided above. I assumed it was ok since he included it. I've since removed it and here is the code without it and it works fine for ALL dates.

Code: 
Function GetDate(inputText As String) As Variant
   If Len(inputText) <> 0 Then
      GetDate = CDate(inputText)
   End If
End Function

@Debaser - Once I understood this code works on ALL dates. I did a happy dance. I appreciate your help!

Thank you all for your insight!
 
Yes. The original ‘On Error Resume Next’ was mine. But, I was referring to Debasser’s post on Thursday at 3:52 AM.
Debaser said:
I'd add a function like:

Code: 
Function GetDate(inputText As String) As Variant
   If Len(inputText) <> 0 Then
      On Error Resume Next
      GetDate = CDate(inputText)
   End If
End Function

then use GetDate in place of CDate.

It seems unlikely to me that you should be looping here, unless you really do have multiple lines for the same order number and need to update them all?
Click to expand...
 
Yodelayheewho said:
Yes. The original ‘On Error Resume Next’ was mine. But, I was referring to Debasser’s post on Thursday at 3:52 AM.
Click to expand...
You are making a beginner mistake by assuming that because Debaser used On Error Resume Next in his small subroutine, that it was OK for you to use as the first line of code in your much larger subroutine. The way he used it was in a limited way for a specific purpose. The way you used it was not. Globally suppressing runtime errors is almost always a bad thing. Putting On Error Resume Next as the first line of code in a complex subroutine cripples your abilty to know what is happening when something goes wrong.
 
You must log in or register to reply here.
Back
Top