[Issue #506] Allow linking between series

[andrepapoti]

Description

Allow linking between two series.
A new field called related_series was added to the Series model where you can reference other Series. This feature is can be used to link different versions of the same Series to easily build a version history.
The linking of a Series is bidirectional so if you link A to B automatically B is linked to A.
For this feature a new API endpoint was created so the API version was bumped to v1.4.

Related

Closes #506

[stephenfin]

Apologies for the delays getting this. I'll try to respond faster to future revisions. I have some minor nits in the docs but my bigger question is this: should this be a dedicated API, or could we allow this as part of a new update series API? For example:

PATCH /api/v1.4/series/1 HTTP/1.1
Content-Type: text/json

{
    "related": [2, 3]
}

This is what we do for patches now, so I wonder if there's any reason not to do the same for series?

(fwiw, I have a clear preference for sticking to RESTful APIs where possible, so I'm wondering what reason, if any, there is for not doing things that way 🙂)

[stephenfin]

Suggested change

	1.4, 3.1, ✓
	1.4, 3.2, ✓

[stephenfin]

Suggested change

	API v1.3
	=================
	API v1.3
	========

[stephenfin]

Also, we probably want to expose events for this? However, we should be careful not to avoid the pitfall encountered in related patches, namely that we returned the related_patches attribute as it was, despite that fact this would change over time as patches were added/removed.

[andrepapoti]

@stephenfin updated the API to follow your suggestion.

My initial idea was to not require the user to just send the new values instead of appending them to the previous related_series in the request's body, but considering that the Patch already follows the format you suggested it and it's not an issue for the clients to get the current related_series of a Series

[hero24]

I have tried running this PR and accessing /api/series/953/ through web that without this PR works well, now causes internal server error. It seems to be caused by line 942 of the models.py file inside is_editable method. Also, are we adding a web interface to this? Is there going to be like a 'related series' section on series, or is only API change that is to be used in standalone clients?

[andrepapoti]

I have tried running this PR and accessing /api/series/953/ through web that without this PR works well, now causes internal server error. It seems to be caused by line 942 of the models.py file inside is_editable method. Also, are we adding a web interface to this? Is there going to be like a 'related series' section on series, or is only API change that is to be used in standalone clients?

I'll investigate the issue that you mentioned
The planned work for this issue only included the linking through the API

[kuba-moo]

Connecting this back to #506 "related_series" does not imply in any way that the connected series is an older or newer version. It would be better to make the contents of the related_series an [object in JSON speak|dict in Python speak] rather than a array of ints.

[andrepapoti]

@hero24 Just to confirm, you are getting this error when doing a PATCH request correct? At least for visualization I'm not getting any issues

This error that you probably got is happening because you are using an user to authenticate that does not have a person associated with it. I've put the block inside a try/except block so you don't get an error on the interface anymore, you'll only get a "You do not have permission to perform this action." message instead. To go further on your test linking the series you'll either have to authenticate with an user that's linked to the series submitter or it's a superuser

I've also modified so that superusers can link any series.

Currently there is no interface for this feature since there's no proper series view, but I'm working on them (#589) and this functionality could be could be added to it after either of the PRs are merged.

[andrepapoti]

@kuba-moo Sorry, I don't know if I got your first point very well, do you believe 'related_series' to not be a descriptive enough name?
If that's the case we can rename it to something like: 'versions' or any other name if you have a better idea for it.
Regarding your point to have these relations in an object instead of an array what would be the idea with it? What do you imagine to be the key:value pairs? Values are only added to the current array after they pass the validation that they are a different version of the same patch (at least it should but I'm not sure if the current available attributes allow us to be very restrictive with it, it will still have to rely on good usage by the users)

[kuba-moo]

At least 3 relationships come to mind immediately:

• series which is a newer version of the current one
• series which is an older version of current one
• dependencies (series which have to be applied for the current one to work)
  I'm happy with just tracking second of those, but as you say we should then rename the field. Maybe "previous_version"? Or "supersedes" / "superseded_by" to match the patch state to which we set the old series ?

[hero24]

@hero24 Just to confirm, you are getting this error when doing a PATCH request correct? At least for visualization I'm not getting any issues

This error that you probably got is happening because you are using an user to authenticate that does not have a person associated with it. I've put the block inside a try/except block so you don't get an error on the interface anymore, you'll only get a "You do not have permission to perform this action." message instead. To go further on your test linking the series you'll either have to authenticate with an user that's linked to the series submitter or it's a superuser

I've also modified so that superusers can link any series.

Currently there is no interface for this feature since there's no proper series view, but I'm working on them (#589) and this functionality could be could be added to it after either of the PRs are merged.

I got the error when I tried opening the API call in browser for series x, by default I believe it should return a JSON object with current series state, listing all the fields, but with this patch, when I tried it, it crashed the web page.

[stephenfin]

At least 3 relationships come to mind immediately:

* series which is a newer version of the current one

* series which is an older version of current one

* dependencies (series which have to be applied for the current one to work)
  I'm happy with just tracking second of those, but as you say we should then rename the field. Maybe "previous_version"? Or "supersedes" / "superseded_by" to match the patch state to which we set the old series ?

The last of these is now handled but the series that was posted to the mailing list here and recently merged.

@andrepapoti @victor-accarini I assume you are still interested in addressing the other two cases? If so, I guess this PR now needs a rebase and some rework to handle the changes that other series introduced (DB schema changes, a v1.4 API, etc.)

[victor-accarini]

@stephenfin yes, I'll update the PR with the two other cases, should I follow the same approach used by Hassick? And add a Supersedes tag to the parser?

Also, should I put that behind another "feature flag" for the project?

[victor-accarini]

I took a closer look today and I'm not sure a Supersedes: tag is what you guys want, since usually there is a search for related MessageIDs or the In-Reply-To and References headers, but it would simplify the problem 😬.

I will try to paint the picture that I have in my mind.

Single patch

single patch -> Create a series (v1 by default) with patch title

To update the single patch the user can send the patch again or reply with the new body.

single patch reply -> Create a new series (v1 as well) since the "number" of the patch is the same as the previous
single patch new -> Ditto

Series of patches

Those are the assumptions that I'm making:

• We don't know the order that the patches will arrive or if the user will send duplicates (probably will)
• The user can reply with a new revision(v1,v2,...) to the cover/first patch or any other patch for the series
• We can't use the date of the patches as a parameters for superseding (maybe a fallback?) since the user might have an older commit in the series

v1 1/3, v1 2/3, v1 3/3 -> Create a series v1
reply with new v1 2/3 -> Create a new series also with v1 (duplicate)
reply any previous message with v2 */* -> Create a new series with new version
...rinse and repeat

Now I have some questions:

• A reply with a "new" version of only one patch - a fix or addressing a review - should be considered a new series and supersede the previous? Or should be handled in a different way?
• I have a v1 series, a v2 series, and I send a v3, the superseded should contain v1, v2 or only v2? Should we address the case were we receive v1, v3, and then v2?

[kuba-moo]

Dependencies and versioning are a bit different. The user has to tell us the dependency with a tag.
Superseding is currently done with an independent bot. The bot uses heuristics relevant for the project, usually something along the lines of "same subject" or "subject tagged vN+1 and there is a link to vN in the cover letter".

IOW my ask is to just have the ability to store the relationship in patchwork, and on the kernel side @mricon's existing bot can do the parsing and linking using the ReST API.