routes_schedules : notes on lines are displayed at service level

xavierraffin commented 9 years ago

In a routes_schedules request, comment (notes) on lines are displayed at service level instead of line level. This seems to be a bug (accordingly to Stephan Simart :-)

I would like to correct this myself, but I am waiting for exact instruction about what should be the correct behaviour.

eturck commented 9 years ago

We've discussed this quite a bit with @antoine-de and @stifoon and I'm going to recap the different things I've found and take some screenshot to illustrate our problem.

Datas

Starting with the datas. NTFS files (only showing useful fields), and particularly :

lines.txt :

line_id,external_code,line_code,comment_id
12,12,61,l_8

routes.txt :

route_id,external_code,is_forward,line_id
12_A,61_A,1,12

trips.txt :

trip_id,route_id,external_code,comment_id
2192,12_A,2192,
2195,12_A,2195,t_4

comments.txt :

comment_id, comment_name
l_8,Offre septembre 2015
t_4,e - Sauf le mercredi

What do we have ?

One line, 61, with a comment.
One route, 61_A,
2 trips, one with a comment, one without a comment,
2 comments.

You can download the complete ntfs files here : https://github.com/eturck/fixture_issue_comments_route_schedules/raw/master/ntfs_issue_comments.zip Or look at them : https://github.com/eturck/fixture_issue_comments_route_schedules

The problem

Comment on lines are displayed on multiple levels on route_schedules. It's not really a problem in itself, but when we have a comment on a trip too, they are represented on the same level with no distinction.

If we call http://<jormungandr_server>/v1/coverage/<instance_name>/routes/route:12_A/route_schedules?from_datetime=20150901T050000 we have :

As you can see the comment of the line is displayed at the first level. When we look at the headers : route_schedules_headers

Both notes are displayed on the header of the trip with is own comment. On the other header there is only the note of the line. When we have both, I've seen nothing allowing me to tell to which object the note refer to.

Investigation

After looking at the code I think it's the intended behavior. @stifoon said the other day that you're looking into associating a type to each note, I guess for the same sort of reason?

In time_table/route_schedules.cpp : https://github.com/CanalTP/navitia/blob/8b4d11f136cc2836aed085ac0ff46e21af7f5ffc/source/time_tables/route_schedules.cpp#L291 We're filling a pb_object by passing vj_display_information. It brings us (through another function only calling fill_pb_object with a few nullptr) to https://github.com/CanalTP/navitia/blob/8b4d11f136cc2836aed085ac0ff46e21af7f5ffc/source/type/pb_converter.cpp#L1357

In this function we're doing (https://github.com/CanalTP/navitia/blob/8b4d11f136cc2836aed085ac0ff46e21af7f5ffc/source/type/pb_converter.cpp#L1371):

fill_pb_object(vj->journey_pattern->route, data, pt_display_info,max_depth,now,action_period);
// in fill_pb_object for the route this code is executed :
fill_pb_object(comment, data, pt_display_info->add_notes(), max_depth, now, action_period);

And (https://github.com/CanalTP/navitia/blob/8b4d11f136cc2836aed085ac0ff46e21af7f5ffc/source/type/pb_converter.cpp#L1392)

fill_pb_object(comment, data, pt_display_info->add_notes(), max_depth, now, action_period);

Long story short (you probably know this code better than me), we're adding comments of the route and the vj in the same pt_display_info->add_notes(). There doesn't seems to be an easy way to differentiate them.

The comments on the route are here since the function creation : https://github.com/CanalTP/navitia/commit/a9277cce3f0fcfb6bed4423d8af3b10db509822e#diff-9db5f8b2306c44a2a59af3215f2beb47R687 And the vj comments have been added a few month ago : https://github.com/CanalTP/navitia/commit/198075d3f5d59f876220a79bd4af9d33d327babc#diff-9db5f8b2306c44a2a59af3215f2beb47R1287

Conclusion

This leave me with a few question :

Is it working as intended (it could really be)?
Is there a way for me to differentiate the two, other than saying "if it's at route_schedule level it's a note on the line I should ignore it in the header if I want to have only the trip notes"?
You've said something about associating a type to each note, is it something you are going to release soon?

Thank you if you've made it this far :D. Have a nice day!

stifoon commented 8 years ago

We will cut this issue in 2 part:

first: a bug on notes associated on routes or lines. Those kind of notes should be linked on route_schedule level only
second: an evolution on notes to expose a new property "reference_code" to drive the code reference ;) This feature seems to be realized with https://github.com/CanalTP/navitia/issues/1278

We will take the bug ASAP, we will talk about reference_code with other user in Kisio

antoine-de commented 8 years ago

I think the right way to correct this is not to put the link of the line, route and network in the row but in the route_schedule. It seems the right place for those link and this way the line|route|networks notes will automagically move the the route_schedule

thus:

  "disruptions":[  ],
  "notes":[
   ...
  ],
  "route_schedules":[
    {
      "display_informations":{
        ...
      },
      "table":{
        "headers":[
          {
            "display_informations":{
              "direction":"stop_area:stop2",
              "code":"",
              "description":"",
              "links":[

              ],
              "color":"",
              "physical_mode":"name physical_mode:Car",
              "headsign":"vj1",
              "commercial_mode":"",
              "equipments":[
                "has_wheelchair_accessibility"
              ],
              "label":"line:A",
              "network":"base_network"
            },
            "additional_informations":[
              "regular"
            ],
            "links":[
              {
                "type":"line",
                "id":"line:A"
              },
              {
                "type":"vehicle_journey",
                "id":"vj1"
              },
              {
                "type":"route",
                "id":"line:A:0"
              },
              {
                "type":"physical_mode",
                "id":"physical_mode:Car"
              },
              {
                "type":"network",
                "id":"base_network"
              },
              {
                "internal":true,
                "type":"notes",
                "id":"note:9086145111820679267"
              },
              {
                "internal":true,
                "type":"notes",
                "id":"note:9086145111820679267"
              },
              {
                "internal":true,
                "type":"notes",
                "id":"note:2762169579135187400"
              }
            ]
          }
        ],
        "rows":[
          {
            "stop_point":{
              ...
            },
            "date_times":[
             ...
            ]
          },

        ]
      },
      "links":[
        {
          "type":"notes",
          "id":"note:9086145111820679267"
        },
        {
          "type":"notes",
          "id":"note:9086145111820679267"
        }
      ]
    }
  ]
}

will become:


  "disruptions":[  ],
  "notes":[
...
  ],
  "route_schedules":[
    {
      "display_informations":{
...
      },
      "table":{
        "headers":[
          {
            "display_informations":{
              "direction":"stop_area:stop2",
              "code":"",
              "description":"",
              "links":[

              ],
              "color":"",
              "physical_mode":"name physical_mode:Car",
              "headsign":"vj1",
              "commercial_mode":"",
              "equipments":[
                "has_wheelchair_accessibility"
              ],
              "label":"line:A",
              "network":"base_network"
            },
            "additional_informations":[
              "regular"
            ],
            "links":[
              {
                "type":"vehicle_journey",
                "id":"vj1"
              },
              {
                "type":"physical_mode",
                "id":"physical_mode:Car"
              },
              {
                "internal":true,
                "type":"notes",
                "id":"note:9086145111820679267"
              },
            ]
          }
        ],
        "rows":[
          {
            "stop_point":{
              ...
            },
            "date_times":[
             ...
            ]
          },

        ]
      },
      "links":[
        {
          "type":"notes",
          "id":"note:9086145111820679267"
        },
       {
                "type":"line",
                "id":"line:A"
              },
              {
                "type":"route",
                "id":"line:A:0"
              },
              {
                "type":"network",
                "id":"base_network"
              },
              {
                "internal":true,
                "type":"notes",
                "id":"note:2762169579135187400"
              }
      ]
    }
  ]
}

l-vincent-l commented 8 years ago

Ça mérite une v2 Le 24 nov. 2015 5:42 PM, "Antoine D" notifications@github.com a écrit :

I think the right way to correct this is not to put the link of the line, route and network in the row but in the route_schedule. It seems the right place for those link and this way the line|route|networks notes will automagically move the the route_schedule

thus:

"disruptions":[ ], "notes":[ ... ], "route_schedules":[ { "display_informations":{ ... }, "table":{ "headers":[ { "display_informations":{ "direction":"stop_area:stop2", "code":"", "description":"", "links":[

          ],
          "color":"",
          "physical_mode":"name physical_mode:Car",
          "headsign":"vj1",
          "commercial_mode":"",
          "equipments":[
            "has_wheelchair_accessibility"
          ],
          "label":"line:A",
          "network":"base_network"
        },
        "additional_informations":[
          "regular"
        ],
        "links":[
          {
            "type":"line",
            "id":"line:A"
          },
          {
            "type":"vehicle_journey",
            "id":"vj1"
          },
          {
            "type":"route",
            "id":"line:A:0"
          },
          {
            "type":"physical_mode",
            "id":"physical_mode:Car"
          },
          {
            "type":"network",
            "id":"base_network"
          },
          {
            "internal":true,
            "type":"notes",
            "id":"note:9086145111820679267"
          },
          {
            "internal":true,
            "type":"notes",
            "id":"note:9086145111820679267"
          },
          {
            "internal":true,
            "type":"notes",
            "id":"note:2762169579135187400"
          }
        ]
      }
    ],
    "rows":[
      {
        "stop_point":{
          ...
        },
        "date_times":[
         ...
        ]
      },

    ]
  },
  "links":[
    {
      "type":"notes",
      "id":"note:9086145111820679267"
    },
    {
      "type":"notes",
      "id":"note:9086145111820679267"
    }
  ]
}

] }

will become:

"disruptions":[ ], "notes":[... ], "route_schedules":[ { "display_informations":{... }, "table":{ "headers":[ { "display_informations":{ "direction":"stop_area:stop2", "code":"", "description":"", "links":[

          ],
          "color":"",
          "physical_mode":"name physical_mode:Car",
          "headsign":"vj1",
          "commercial_mode":"",
          "equipments":[
            "has_wheelchair_accessibility"
          ],
          "label":"line:A",
          "network":"base_network"
        },
        "additional_informations":[
          "regular"
        ],
        "links":[
          {
            "type":"vehicle_journey",
            "id":"vj1"
          },
          {
            "type":"physical_mode",
            "id":"physical_mode:Car"
          },
          {
            "internal":true,
            "type":"notes",
            "id":"note:9086145111820679267"
          },
        ]
      }
    ],
    "rows":[
      {
        "stop_point":{
          ...
        },
        "date_times":[
         ...
        ]
      },

    ]
  },
  "links":[
    {
      "type":"notes",
      "id":"note:9086145111820679267"
    },
   {
            "type":"line",
            "id":"line:A"
          },
          {
            "type":"route",
            "id":"line:A:0"
          },
          {
            "type":"network",
            "id":"base_network"
          },
          {
            "internal":true,
            "type":"notes",
            "id":"note:2762169579135187400"
          }
  ]
}

] }

— Reply to this email directly or view it on GitHub https://github.com/CanalTP/navitia/issues/1033#issuecomment-159331286.

antoine-de commented 8 years ago

In theory yes, but I would argue that technically this does not break the interface since links are anonymous container. I can indeed break integration but I don't think it likely since those links were useless (I checked no integration on our side would be broken).

What do you think about this @eturck @xavierraffin ?

eturck commented 8 years ago

@antoine-de Just to be sure. The idea is just to remove line|route|networks notes from the header and move them in the route_schedule links, and just keep vj notes in this header? But lines notes are already at route_schedule level no (maybe not routes / networks notes)?

Sorry it has been a few months and I don't remember exactly how everything works ;)

antoine-de commented 8 years ago

yes the idea is just to move line|route|networks links + all the related note links from the header to the routes_schedule. (There are indeed only line note for the moment in the route_schedule)

eturck commented 8 years ago

@antoine-de That's perfect then!

hove-io / navitia