SQL/ACCESS Function Not Working As Intended

Question

Public Function GetPendingChangeOrders(strJ As String) As Double

strSQL = "SELECT DISTINCT Sum(jcdetail.cost) AS SumOfcost " & 
                    "FROM jcchangeorder INNER JOIN jcdetail ON (jcchangeorder.ordernum = jcdetail.ponum) AND (jcchangeorder.jobnum =jcdetail.jobnum) " & 
                    "GROUP BY jcdetail.jobnum, jcdetail.type, jcchangeorder.type, IIf(DLookUp(""type"",""jcchangeorderstep"",""jobnum = '"" & [jcchangeorder].[jobnum] & ""' and ordernum = '"" & [ordernum] & ""' and Type = 20"")=20,-1,0) " & _
                    "HAVING (((jcdetail.jobnum)='" & strJ & "') AND ((jcdetail.type)=19) AND ((jcchangeorder.type)<>2) AND ((IIf(DLookUp(""type"",""jcchangeorderstep"",""jobnum = '"" & [jcchangeorder].[jobnum] & ""' and ordernum = '"" & [ordernum] & ""' and Type = 20"")=20,-1,0))=0));"


Set rs = dbs.OpenRecordset(strSQL, dbOpenSnapshot, dbReadOnly, dbReadOnly)

 If Not rs.EOF Then
    dblResult = Nz(rs.Fields(0), 0)
    rs.Close
    Set rs = Nothing
    GetPendingChangeOrders = dblResult
Else
    GetPendingChangeOrders = 0
End If
End Function

So I got tossed into some MS-Access database with VBA/SQL statements all over. I am literally a beginner, but I have managed to figure some things out, and familiarize myself with our database that we use to print out job reports. Some of the call functions are setup wrong, and are pulling from the wrong tables, and I basically need some help figuring out which way I should be going to tackle this.

Currently if we run the report, and it calls "GetPendingChangeOrders" it does what it is supposed to do, but when we look at what is under pending. It shows a result even though it has a status of 21(DENIED) inside of "JCCHANGEORDERSTEP" table. I included images of it.

JCCHANGEORDER has the same as columns as JCCHANGEORDERSTEP(JOBNUM,ORDERNUM,TYPE) but the types in JCCHANGEORDER just has a type of 1 which I assume says hey I'm active.

JCCHANGEORDERSTEP contains 1 initiated (pending), 20 (approved), 21(denied). It filters out the 20's from results on report, but not 21. So I just need some help, and an explanation of why just adding 21 into the mix didn't work.

Thank you for your time.

EDIT-1 ADDED IMGS IMGUR ACCESS PICTURES

Could you possibly rewrite that last paragraph to explain what `PENDING` is and split the paragraph into smaller comprehensible chunks? Imagine that you have never seen your database and re-read the paragraph and you'll see what I mean. Maybe include your sample data, or at least a screenshot of the issue. — Lee Mac, Nov 14 '18 at 19:25
Can you reword again? Your pictures do not illustrate issue clearly. What is the fundamental problem? Is the query filtering out items that should be in output or vice versa not correctly excluding them? — Parfait, Nov 14 '18 at 20:38
Pending CO is the report result. Its filtering out 20s approved from COSTEP, but not 21 denied in the same table. Others are to show where and what they contain. — , Nov 14 '18 at 20:44

StoneGiant · Answer 1 · 2018-11-14T22:25:29.417

This part of the query in your HAVING and GROUP BY clauses is what's giving you problems:

IIf(DLookUp("type",
            "jcchangeorderstep",
            "jobnum = ' [jcchangeorder].[jobnum] ' and 
              ordernum = ' [ordernum] ' and
              Type = 20")=20,-1,0))=0);

It's convoluted and very difficult to read. But it is saying, "If this job and order appears in JCCHANGEORDERSTEP with a type of 20, exclude it." So, that's what you need to fix.

The whole query should probably be fixed in a variety of ways. But I think this might get you where you need to go.

strSQL = "SELECT DISTINCT Sum(jcdetail.cost) AS SumOfcost " & _
           "FROM jcchangeorder " & _
                "INNER JOIN jcdetail " & _
                         "ON (jcchangeorder.ordernum = jcdetail.ponum) " & _
                        "AND (jcchangeorder.jobnum =jcdetail.jobnum) " & _
          "GROUP BY jcdetail.jobnum, " & _
                   "jcdetail.type, " & _
                   "jcchangeorder.type, " & _
                   "DLookUp(""type"",""jcchangeorderstep"",""jobnum = '"" & [jcchangeorder].[jobnum] & ""' and ordernum = '"" & [ordernum] & ""' and Type = 1"") " & _
         "HAVING (jcdetail.jobnum='" & strJ & "' AND " & _
                 "jcdetail.type=19 AND " & _
                 "jcchangeorder.type <> 2) AND  " & _
                 "DLookUp(""type"",""jcchangeorderstep"",""jobnum = '"" & [jcchangeorder].[jobnum] & ""' and ordernum = '"" & [ordernum] & ""' and Type = 1"")=1;"

What I have done is changed to condition to say "If this job and order appears in JCCHANGEORDERSTEP with a type of 1, include it." Without actually seeing your data and testing the code myself, I can't promise this will work. There may be some typos, so I've explained what I'm trying to do, so you can fix them.

Also, take some time to go through the Stack Overflow tour. This community can be a great help if you work with it.

Try This:
After discussing the desired results with the OP, it appears that this would be a better solution. It gives the sum of all change orders that only have a change order step of PENDING.

strSQL = _
"SELECT SUM(JCD.cost) AS sumofcost " & _
  "FROM jcchangeorder JCCO  " & _
       "INNER JOIN jcdetail JCD  " & _
               "ON JCCO.ordernum = jcd.ponum  " & _
              "AND JCCO.jobnum = jcd.jobnum  " & _
       "INNER JOIN (SELECT JCCOS.ponum,  " & _
                          "JCCOS.jobnum  " & _
                     "FROM jcchangeorderstep JCCOS  " & _
                    "GROUP BY JCCOS.ponum,  " & _
                             "JCCOS.jobnum  " & _
                   "HAVING Count(*) = 1  " & _
                      "AND First(JCCOS.type) = 1) JCSELECT  " & _
               "ON JCCO.ordernum = JCSELECT.ponum  " & _
                  "AND JCCO.jobnum = JCSELECT.jobnum  " & _
 "GROUP BY JCD.jobnum,  " & _
          "JCD.type,  " & _
          "JCCO.type "
"HAVING JCD.jobnum='" & strJ & "' AND " & _
       "JCD.type=19 AND " & _
       "JCCO.type <> 2;"

JCCO, JCCOS, and JCD are SQL aliases. SQL understands them. JCSELECT is an aliased subquery. JCSELECT creates a set of all job/orders that only have a step of PENDING.

Stone I thank you for your response, and included some pictures to help. — , Nov 14 '18 at 19:45
From your example, it looks like a order/job can be both pending and approved or pending and denied. Are you looking for those jobs that are pending but neither approved nor denied? (If so, I need to revise my answer.) — StoneGiant, Nov 14 '18 at 19:49
I will use my pictures to explain. jcdetail will show everything about a job. I filtered it for just change orders(19). So in our accounting software if you make a change order it throws the data into "changeorderstep" which will be a initiated(1), so essentially its pending, but if you add a new event say approved(20) it is still in "changeorderstep" underneath it, but its working to filter out 20s, but not 21s(denied). jcchangeorder just has type 1s and order numbers. JCdetail also includes "ponums" as changeorder is "conums" and yes I read your comment wrong. You are correct. — , Nov 14 '18 at 19:56
I've added a solution at the bottom that _should_ give you the sum of everything that only has a 1 in the type column of jcchangeorderstep. If it works, (typos notwithstanding) I'll modify my answer and add explanations. — StoneGiant, Nov 14 '18 at 20:21
I tested and added in the proper names for everything you had. The jcselect threw me off so I put in jcdetail for them. But as I run it the RS below it doesn't want to work with what you have. Please advise. — , Nov 14 '18 at 22:10
If you changed the jcselect, you broke it. Try it exactly as is first. We can open a discussion chat, I think. — StoneGiant, Nov 14 '18 at 22:11
It has an extra semi-colon. Fixed. Try it now. If that doesn't do it, I'll review later. I'll have access to Access and can do tests of my own at that point. — StoneGiant, Nov 14 '18 at 22:25
Okay, look for an update tomorrow unless Lee Mac's answer does what you want. — StoneGiant, Nov 14 '18 at 22:43

score 0 · Accepted Answer · edited Jun 20 '20 at 09:12

0

Having looked at your images and studied your existing SQL code, I think the following SQL query may be more suitable and altogether more readable:

select sum(d.cost) as sumofcost 
from 
    (
        jcchangeorder o inner join jcdetail d
        on o.ordernum = d.ponum and o.jobnum = d.jobnum
    ) inner join
    (
        select distinct s.jobnum, s.ordernum 
        from jcchangeorderstep s 
        where s.type = 1
    ) q
    on o.jobnum = q.jobnum and o.ordernum = q.ordernum
where
    o.jobnum = ?job and d.type = 19 and o.type <> 2

Here, the inclusion of jcdetail records for which the jcchangeorderstep.type = 1 is handled by the inner join between the tables, rather than a separate dlookup for every record.

You could implement this in your function in the following way:

Public Function GetPendingChangeOrders(strJ As String) As Double
    Dim strS As String
    strS = strS & "select sum(d.cost) "
    strS = strS & "from "
    strS = strS & "    ( "
    strS = strS & "        jcchangeorder o inner join jcdetail d "
    strS = strS & "        on o.ordernum = d.ponum and o.jobnum = d.jobnum "
    strS = strS & "    ) inner join "
    strS = strS & "    ( "
    strS = strS & "        select distinct s.jobnum, s.ordernum "
    strS = strS & "        from jcchangeorderstep s "
    strS = strS & "        where s.type = 1 "
    strS = strS & "    ) q "
    strS = strS & "    on o.jobnum = q.jobnum and o.ordernum = q.ordernum "
    strS = strS & "where "
    strS = strS & "    o.jobnum = ?job and d.type = 19 and o.type <> 2 "
    
    Dim rst As DAO.Recordset
    With CurrentDb.CreateQueryDef("", strS)
        .Parameters(0) = strJ
        Set rst = .OpenRecordset
        If Not rst.EOF Then
            rst.MoveFirst
            GetPendingChangeOrders = Nz(rst.Fields(0), 0)
        End If
        rst.Close
    End With
End Function

EDIT:

Following the subsequent comments, the following seems more aligned with your requirements:

select sum(d.cost) 
from  
    jcchangeorder o inner join jcdetail d 
    on o.ordernum = d.ponum and o.jobnum = d.jobnum
where
    o.jobnum = jobparam and
    d.type = 19 and
    o.type <> 2 and
    not exists 
    (
        select 1 from jcchangeorderstep s 
        where s.jobnum = o.jobnum and s.ordernum = o.ordernum and s.type <> 1
    )

This may be implemented in your VBA function in the following way:

Public Function GetPendingChangeOrders(strJ As String) As Double
    Dim strS As String
    strS = strS & "select sum(d.cost) "
    strS = strS & "from  "
    strS = strS & "    jcchangeorder o inner join jcdetail d "
    strS = strS & "    on o.ordernum = d.ponum and o.jobnum = d.jobnum "
    strS = strS & "where "
    strS = strS & "    o.jobnum = jobparam and "
    strS = strS & "    d.type = 19 and "
    strS = strS & "    o.type <> 2 and "
    strS = strS & "    not exists "
    strS = strS & "    ( "
    strS = strS & "        select 1 from jcchangeorderstep s "
    strS = strS & "        where s.jobnum = o.jobnum and s.ordernum = o.ordernum and s.type <> 1 "
    strS = strS & "    ) "

    Dim rst As DAO.Recordset
    With CurrentDb.CreateQueryDef("", strS)
        .Parameters("jobparam") = strJ
        Set rst = .OpenRecordset
        If Not rst.EOF Then
            rst.MoveFirst
            GetPendingChangeOrders = Nz(rst.Fields(0), 0)
        End If
        rst.Close
    End With
End Function

edited Jun 20 '20 at 09:12

Community

1
1

answered Nov 14 '18 at 20:15

Lee Mac

15,615
6
32
80

I will try this when I get back from lunch. Is there any reason why this one over the other? – Nov 14 '18 at 20:32
I have never seen a parameter placeholder in DAO like `?job`. Also, do note: you can parameterize saved queries in MS Access (i.e., first block code) without VBA concatenation. – Parfait Nov 14 '18 at 20:34
@Parfait Re `?job`, I learnt this technique from Erik von Asmuth [here](https://stackoverflow.com/a/49509616/7531598); I realise you can parameterize saved queries, but thought it may be easier to provide the OP with a solution that didn't rely on them creating additional queries. – Lee Mac Nov 14 '18 at 22:40
1

@ThomasRichardson I would always suggest using parameterised SQL queries over concatenation - even if the chances of a SQL injection are negligible, it is good practice. – Lee Mac Nov 14 '18 at 22:44
Very interesting! Never knew about the qmark approach. Re saved queries, in MS Access they are more efficient than VBA string queries run on the fly as engine saves best execution plan! – Parfait Nov 14 '18 at 22:44
@Parfait Thanks! - I didn't realise there was a performance improvement to be had from using saved queries. – Lee Mac Nov 14 '18 at 22:47
@LeeMac I am getting an error with yours being. syntax error (missing operator) in query expression 'o.jobnum = ?job and d.type = 19 and o.type <>2 Also check out stonegiant reply he was on the right track of what I needed it to do. – Nov 14 '18 at 22:53
@ThomasRichardson How are you calling the function? – Lee Mac Nov 14 '18 at 23:35
@LeeMac In my MS-database I run view report. Got to page 7, text box control source is =GetPendingChangeOrder runs the module. I dont know why it is even referencing aia. – Nov 14 '18 at 23:45
@ThomasRichardson Your original function required a single argument (the jobnum), therefore, the function would need to be supplied with such argument when called, e.g. `=GetPendingChangeOrders("180410")` – Lee Mac Nov 14 '18 at 23:47
I forgot to mention this database is spaghetti code all over. I just wanted 21 filtered out the same way its doing to 20. T_T The data is pulled over ODBC then filtered into a local table after running the report then part of the report design has modules, VBA, and continues from there.. – Nov 14 '18 at 23:58
@ThomasRichardson Your original function would still have to be evaluated with the argument supplied. Could you show the *exact* content of the ControlSource of your textbox? – Lee Mac Nov 15 '18 at 00:00
=GetPendingChangeOrders([jobnum]) – Nov 15 '18 at 00:04
@ThomasRichardson Thanks - that looks correct. I'm at a loss to explain the `aiarfc.jobnum` error. – Lee Mac Nov 15 '18 at 00:15
Here is a pastebin of just module 3 with everything that just happening to be there. https://pastebin.com/YGz5uhpT – Nov 15 '18 at 00:20
@ThomasRichardson Looking at your excerpt, I think the issue may be the global declaration of the `strSQL` variable already present in your code; I've therefore changed the name of this variable in my code to avoid clashes - please try it now, and remember to comment out any other definitions of the same function name. – Lee Mac Nov 15 '18 at 00:32
@ThomasRichardson Great stuff - glad to hear it's working well for you. Please mark my answer as the solution (and upvote if you wish) if everything is sorted so that the question appears resolved. – Lee Mac Nov 15 '18 at 00:41
@LeeMac Fancy that it posted. Now nothing is in pending CO's. Time to test by putting one in there. Oh boy it worked when I took denied out, and I just tested it with a approved. Oh man Lee you are a life saver. Now should I apply this same type of code vs the other functions in the same module to cut down on how slow this bad boy is? – Nov 15 '18 at 00:41
@Parfait The question mark is no different than any other character in a parameter name. DAO will treat any unknown name in an SQL statement as a parameter. Perhaps the only surprising thing is that the question mark is allowed as a name character in the first place, but otherwise there is nothing special about it. The parameters could have been named Xp1 and Xp2 and had the same effect. – C Perkins Nov 15 '18 at 04:21

SQL/ACCESS Function Not Working As Intended

2 Answers2

EDIT: