Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Issue #506] Allow linking between series #587

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[Issue #506] Allow linking between series #587

wants to merge 6 commits into from

Conversation

andrepapoti
Copy link

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

@andrepapoti andrepapoti mentioned this pull request Mar 20, 2024
Open
stephenfin
stephenfin reviewed Apr 6, 2024
View reviewed changes
Copy link
Member

@stephenfin stephenfin left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂)

docs/api/rest/index.rst Outdated
@@ -275,6 +275,7 @@ Supported Versions
1.1, 2.1, ✓
1.2, 2.2, ✓
1.3, 3.1, ✓
1.4, 3.1, ✓
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1.4, 3.1, ✓
1.4, 3.2, ✓

docs/api/rest/schemas/v1.3.rst Outdated
Comment on lines 1 to 2
API v1.3
=================
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
API v1.3
=================
API v1.3
========

@stephenfin
Copy link
Member

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
Copy link
Author

@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
Copy link

hero24 commented Oct 29, 2024

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
Copy link
Author

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
Copy link

kuba-moo commented Nov 1, 2024

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
models: Add 'related_series' field to Series model
a2cac80 
Closes getpatchwork#506

Signed-off-by: andrepapoti <[email protected]>
@andrepapoti
serializers: Add 'related_versions' field to series serializer
6e3d250 
For two series to be linked togheter they must belong to the same project

Closes getpatchwork#506

Signed-off-by: andrepapoti <[email protected]>
@andrepapoti
serializers: Optimize query performance on SeriesMixin for 'related_s…
14078c5 
…eries'

Ensure the retrival of series is keept at O(1) complexity.

Closes getpatchwork#506

Signed-off-by: andrepapoti <[email protected]>
@andrepapoti
urls: Create endpoint to link two series
e9c5545 
Closes getpatchwork#506

Signed-off-by: andrepapoti <[email protected]>
@andrepapoti
tests: Add tests for linking series feature
2ea356c 
Closes getpatchwork#506

Signed-off-by: andrepapoti <[email protected]>
@andrepapoti
release-notes: Add release notes for the Series linking feature
a91a667 
Closes getpatchwork#506

Signed-off-by: andrepapoti <[email protected]>
@andrepapoti
Copy link
Author

andrepapoti commented Nov 4, 2024 •
edited
Loading

@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
Copy link
Author

@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
Copy link

kuba-moo commented Nov 5, 2024

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
Copy link

hero24 commented Nov 5, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenfin stephenfin stephenfin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow linking to older versions of a series
4 participants
@andrepapoti @stephenfin @hero24 @kuba-moo