Update readme and changelog - #1239
MSchmoecker wants to merge 12 commits into
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
cafd7c9 to
8d9c389
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1239 +/- ##
==========================================
+ Coverage 95.69% 95.82% +0.13%
==========================================
Files 694 705 +11
Lines 25443 26277 +834
Branches 1307 1340 +33
==========================================
+ Hits 24347 25181 +834
+ Misses 902 899 -3
- Partials 194 197 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c1be768 to
ae528ce
Compare
15b248b to
e1df979
Compare
e1df979 to
13c573a
Compare
| version = ( | ||
| PackageVersion.objects.select_for_update(of=("self",)) | ||
| .select_related("package", "package__owner") | ||
| .get(pk=version.pk) | ||
| ) | ||
| version.package.ensure_user_can_manage_wiki(agent) |
There was a problem hiding this comment.
Lock aquired before checking permissions. The view requires only IsAuthenticated. So any logged-in user, including one with no relation to the package can aquire a lock.
| ] | ||
| constraints = [ | ||
| models.CheckConstraint( | ||
| check=models.Q(document__in=["readme", "changelog"]), |
There was a problem hiding this comment.
class Document is created above, could that replace the usage of these magic values?
| ) | ||
| version.package.ensure_user_can_manage_wiki(agent) | ||
|
|
||
| if "changelog" in overrides and version.pk != version.package.latest_id: |
There was a problem hiding this comment.
"changelog" seems to be a magic value here, use Document class?
| if document == "readme": | ||
| content, filename = version.readme_override, "README.md" | ||
| elif document == "changelog": | ||
| content, filename = version.changelog_override, "CHANGELOG.md" |
There was a problem hiding this comment.
"readme" and "changelog" are magic values
| for document in PackageVersionMarkdownRevision.Document.values: | ||
| if document not in overrides: | ||
| continue | ||
|
|
||
| content = overrides[document] | ||
| # Compare under the lock so retries do not duplicate history or | ||
| # change attribution when the stored override has not changed. | ||
| if content == getattr(version, f"{document}_override"): | ||
| continue | ||
| is_override = content is not None | ||
| updates[f"{document}_override"] = content | ||
| updates[f"{document}_override_edited_at"] = now if is_override else None | ||
| updates[f"{document}_override_edited_by"] = agent if is_override else None | ||
| revisions.append( | ||
| PackageVersionMarkdownRevision( | ||
| version=version, | ||
| document=document, | ||
| content=content if is_override else getattr(version, document), | ||
| is_override=is_override, | ||
| recorded_at=now, | ||
| edited_at=now, | ||
| edited_by=agent, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Nit: rather than building field names from enum values (f"{document}_override"), could we map each Document to its field names explicitly in one place and use that here and in the history/download views? For example:
@dataclass(frozen=True)
class MarkdownFields:
original: str
override: str
edited_at: str
edited_by: str
MARKDOWN_FIELDS = {
Document.README: MarkdownFields(
original="readme",
override="readme_override",
edited_at="readme_override_edited_at",
edited_by="readme_override_edited_by",
),
Document.CHANGELOG: MarkdownFields(
original="changelog",
override="changelog_override",
edited_at="changelog_override_edited_at",
edited_by="changelog_override_edited_by",
),
}
# usage in the service
for document, fields in MARKDOWN_FIELDS.items():
...
if content == getattr(version, fields.override):
continue
updates[fields.override] = content
updates[fields.edited_at] = ...
# usage in the history/download views
fields = MARKDOWN_FIELDS[document]
getattr(version, fields.original)
| related_name="markdown_revisions", | ||
| on_delete=models.CASCADE, | ||
| ) | ||
| document = models.CharField(max_length=9, choices=Document.choices) |
There was a problem hiding this comment.
Why is max_length set to 9 here? Adding any longer document type later needs a column-altering migration.
|
|
||
|
|
||
| class MarkdownChangesQuerySerializer(serializers.Serializer): | ||
| after = serializers.IntegerField(default=0, min_value=0, max_value=2**63 - 1) |
There was a problem hiding this comment.
This is used in row 69. Why is this max_value used?
| @pytest.fixture | ||
| def author_version(): | ||
| version = PackageVersionFactory() | ||
| member = TeamMemberFactory(team=version.package.owner, role="owner") |
There was a problem hiding this comment.
Nit: We should aim for using enums if they are available. I believe there is one for role. Same for the other tests as well.
Allows the user to update the readme and changelog, without having to bump a new version.
This intentionally adds the readme_override and changelog_override to a specific PackageVersion, therefore being handled as a temporary override until the next package version is bumped. It also does not modify existing package zips, the readme and changelog are only visible on the website and API.
The UI for updating is inside the Manage Package modal. It's a file upload to prevent modders from making changes in place and uploading an outdated readme next time. The file upload is only browser site, the content is transmitted as plain text over the API.
I'm not 100% sure if the invalidate_cache_on_commit_async is actually needed here.
Solves #16, #101