When updating an invoice, nested resources are not updated

When I edit an invoice, the batches and tasks are not updated. Here is the payload which is sent:

{
  "@id": "http://localhost:8000/customer-invoices/1/",
  "title": "test test",
  "identifier": "0123456789000",
  "state": "",
  "invoicingDate": "2020-10-21",
  "tvaRate": 13,
  "additionalText": "",
  "batches": [
    {
      "@id": "http://localhost:8000/batchs/1/",
      "title": "Lot 1",
      "tasks": [
        {
          "@id": "http://localhost:8000/tasks/1/",
          "title": "Tache 1",
          "htAmount": 10
        },
        {
          "@id": "http://localhost:8000/tasks/2/",
          "title": "Tache 2",
          "htAmount": 22
        }
      ]
    }
  ],
  "@context": {
    "@vocab": "http://happy-dev.fr/owl/#",
    "rdf": "http://www.w3.org/1999/02/22-rdf-syntax-ns#",
    "rdfs": "http://www.w3.org/2000/01/rdf-schema#",
    "ldp": "http://www.w3.org/ns/ldp#",
    "foaf": "http://xmlns.com/foaf/0.1/",
    "name": "rdfs:label",
    "acl": "http://www.w3.org/ns/auth/acl#",
    "permissions": "acl:accessControl",
    "mode": "acl:mode",
    "geo": "http://www.w3.org/2003/01/geo/wgs84_pos#",
    "lat": "geo:lat",
    "lng": "geo:long"
  }
}

@calummackervoy do you see something wrong in this?

changed the description

looks fine to me! I'll try to replicate this later today to confirm it's a server issue

As a side-note, are invoices supposed to work in a federation? There are several fields with constraints (NOT NULL, NOT BLANK) which isn't possible at the moment djangoldp-packages/djangoldp#224

Replicated this and found multiple (backend) bugs.. You weren't getting a 403?

Provisional estimate of 5h for this, based on the last issue. @balessan the budget I asked for to extend the unit tests in serializer overlaps with this... ideally I could have something like 8h to methodically work through everything, fixing this as part of it

We're literally just about to freeze for Plup's changes though so I think it might be a week until we can merge this @matthieu ... is that okay?

Took it yes. @calummackervoy I think it can live a week as is, but anyway @plup freeze was before now I think.

Ok I'll tell HD Paris about the delay. No problem for me!

I think that one is good now ?

What should I update to test this?

Or maybe it's deployed on test1?

@matthieu : We need @calummackervoy confirmation. To test it would be update of djangoldp. But unsure.

@calummackervoy if test1 is up to date, you can test to edit an invoice here: http://cdn.startinblox.com/invoicing-demo (credentials: hdp / hdp)

@matthieu : how do you log-in ?

There are some sib-core load issues too:

how do you log-in ?

If you are not already login on test1, it is supposed to redirect on auto-login automatically

There are some sib-core load issues too:

This is expected as we need the beta to make the invoicing demo work, but oidc imports the core@0.14. It should work properly even with this

The issue is fixed? I haven't looked into this since I asked if it was okay to delay until after the freeze

So I think it's not. Why did you say it could be OK @balessan ?

I thought it was related to fixes delivered for Coopedia, but I may have been wrong.

@balessan @calummackervoy can we expect this and the #24 (closed) to be fixed this week or should I tell HD Paris that it won't be ready yet?

It's on @calummackervoy hands to confirm but I would be careful.

My point being: it should not be your role to discuss that with the customer directly.

Agree! I was supposed to reach back to Berangere with the demo this week, but if it's not ready, who is reaching back with udpates? @alexbourlier maybe?

So if that's Bérengère it's the product owner not the customer so that's fine to me.

@matthieu I started expanding the unit tests for this yesterday but could you delay the demo until next Wednesday if possible please? That would give us plenty of time

@balessan I've been doing the unit test expansion in the MR for DjangoLDPv2.1 (djangoldp-packages/djangoldp!193 (merged))

Ok! @calummackervoy when the fix is merged, if you publish it on test1, the demo here should work directly

the Batch model is defined like this:

class Batch(Model):
    invoice = models.ForeignKey(CustomerInvoice, on_delete=models.CASCADE, related_name='batches')

The issue is that when LDPSerializer analyses the Batch model it receives, it reads that the invoice key is not defined and throws a ValidationError. We can't just send that key explicitly (like we have in the past), because in this example the nested Tasks don't know the urlid of their parent Batch because it hasn't been created yet

If we define it like this:

class Batch(Model):
    title = models.CharField(max_length=255, blank=True, null=True)

class Invoice(Model):
    batches = models.ManyToManyField(Batch, blank=True)

Then we workaround the issue, because we are first creating a version of the Batch model which does not require an invoice field, and then attaching it to the invoice after creation (LDPSerializer.attach_related_object)

This could be the quick fix which gets the HD Paris demonstration to work ASAP @matthieu @balessan ?

This workaround is not suitable when we need to store information on the through-model, e.g. djangoldp_circle.CircleMember is a through-model which adds a field is_admin. A through-model is a model which sits between two models in a many-to-many relationship, in this case CircleMember is between Circle and user

For a proper fix I believe that it should be possible for LDPSerializer to inject the connecting fields such as invoice and batch to complete the request. I've opened an issue for this here: djangoldp-packages/djangoldp#333

This is obviously harder and I'd rather do it without clients waiting urgently for it

Assuming you're happy with the workaround I'll make the change and deploy it to test1 so that we can validate it

It's ok for me, I think it makes sense :)

mentioned in merge request djangoldp-packages/djangoldp-invoice!8 (closed)

This one seems to be working: https://core-demo.startinblox.com/

Make sure you're logged in as admin to test (#24 (closed) )

The branch has the merge deployed, when validated need to merge

• djangoldp-packages/djangoldp-invoice!8 (closed)
• djangoldp-packages/djangoldp-project!46 (merged)

When logged in as admin, the customerInvoices and freelanceInvoices are not returned in projects/1100, so I got an error and no invoice is shown

I have this too... the project's 'customerInvoices' has disappeared from the serializer for no apparent reason? Back to the drawing board

Cache issue. Looks like changing the relationships hasn't fixed the issue though, the screenshot above is of a creation and the bug is for PUT requests

@matthieu now I think it's working ? :)

I don't understand why but on the demo, everything works fine except if I edit the first amount of the first batch (50€). Can you reproduce this?

Yeah I also have this behaviour, but only with that task in particular

mentioned in merge request djangoldp-packages/djangoldp-invoice!10 (merged)

mentioned in issue djangoldp-packages/djangoldp#333

closed via merge request djangoldp-packages/djangoldp-invoice!10 (merged)