Skip to content

Adjust the 'attachments' property of Documents to find all files. #345

Closed
wants to merge 1 commit into from

4 participants

@darkwing
Mozilla member
darkwing commented Jul 5, 2012

This uses pretty simple logic to scan a Document's HTML for both
MindTouch and kuma file URL patterns, and then uses Q objects to build
a correct query to return the corresponding Attachment objects. If no
file URLs are found in the Document, it'll return an empty Attachment
queryset.

fix bug 770397 - Displaying attachments table under document and tags

fix bug 770563 - Show tags and file attachments regardless of TOC

Bugfix: actually wrap the file queries in Q objects.

Changing file URL method

@darkwing
Mozilla member
darkwing commented Jul 5, 2012

This is a consolidated, merged PR of:

#339

and:

#343

@darkwing
Mozilla member
darkwing commented Jul 5, 2012

Fix file size duplication.

@lmorchard lmorchard commented on an outdated diff Jul 5, 2012
apps/wiki/tests/test_views.py
@@ -1846,4 +1847,111 @@ def test_legacy_redirect(self):
'filename': f['filename']})
resp = self.client.get(mindtouch_url)
eq_(301, resp.status_code)
+<<<<<<< HEAD
@lmorchard
Mozilla member
lmorchard added a note Jul 5, 2012

Merge markers - bad stuff here! Can't merge with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lmorchard lmorchard and 1 other commented on an outdated diff Jul 5, 2012
apps/wiki/tests/test_views.py
ok_(a.get_absolute_url() in resp['Location'])
+=======
@lmorchard
Mozilla member
lmorchard added a note Jul 5, 2012

Maybe just the conflict marker chunk needs to be removed?

@darkwing
Mozilla member
darkwing added a note Jul 5, 2012

Found, fixed. Sorry about that, not sure how that happened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@darkwing darkwing Adjust the 'attachments' property of Documents to find all files.
This uses pretty simple logic to scan a Document's HTML for both
MindTouch and kuma file URL patterns, and then uses Q objects to build
a correct query to return the corresponding Attachment objects. If no
file URLs are found in the Document, it'll return an empty Attachment
queryset.

fix bug 770397 - Displaying attachments table under document and tags

fix bug 770563 - Show tags and file attachments regardless of TOC

Bugfix: actually wrap the file queries in Q objects.

Changing file URL method

Removing unnecessary filesize() method from AttachmentRevisions model

Fixing merge conflict
fe75794
@groovecoder groovecoder commented on the diff Jul 6, 2012
apps/wiki/forms.py
@@ -392,3 +394,36 @@ def clean_slug(self):
if '/' in self.cleaned_data['slug']:
raise forms.ValidationError(SLUG_INVALID)
return super(RevisionValidationForm, self).clean_slug()
+
+
+class AttachmentRevisionForm(forms.ModelForm):
+ # Unlike the DocumentForm/RevisionForm split, we have only one
+ # form for file attachments. The handling view will determine if
+ # this is a new revision of an existing file, or the first version
+ # of a new file.
+ #
+ # As a result of this, calling save(commit=True) is off-limits.
+ class Meta:
+ model = AttachmentRevision
+ fields = ('file', 'title', 'description', 'comment')
+
+ def save(self, commit=True):
@groovecoder
Mozilla member
groovecoder added a note Jul 6, 2012

why do we have commit=True default value if it raises an error?

@ubernostrum
ubernostrum added a note Jul 6, 2012

Mostly because it's the standard API for ModelForm, and toggling the default, or requiring it to be specified explicitly, would break unaware code in ways that doesn't raise the NotImplementedError.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@groovecoder groovecoder commented on the diff Jul 6, 2012
apps/wiki/views.py
# TODO: For now this just grabs and serves the file in the most
- # naive way, since that ensures compatibility for the most common
- # case where we just want to show the file contents embedded in a
- # document.
- #
- # In the future, this should grow to be multiple views -- one
- # legacy view to support document-embedded file URLs, and then
- # more full-featured views for showing metadata, revision history,
- # uploading new versions, etc.
+ # naive way. This likely has performance and security implications.
@groovecoder
Mozilla member
groovecoder added a note Jul 6, 2012

we should use some code like https://github.com/mozilla/kuma/blob/master/apps/demos/models.py#L641 to black-list certain mime-types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@groovecoder
Mozilla member

We should also add these models to the django admin interface.

@groovecoder groovecoder commented on the diff Jul 6, 2012
+ # Files.
+ url(r'^files/new/$',
+ 'wiki.views.new_attachment',
+ name='wiki.new_attachment'),
+ url(r'^files/(?P<attachment_id>\d+)/$',
+ 'wiki.views.attachment_detail',
+ name='wiki.attachment_detail'),
+ url(r'^files/(?P<attachment_id>\d+)/edit/$',
+ 'wiki.views.edit_attachment',
+ name='wiki.edit_attachment'),
+ url(r'^files/(?P<attachment_id>\d+)/history/$',
+ 'wiki.views.attachment_history',
+ name='wiki.attachment_history'),
+ url(r'^files/(?P<attachment_id>\d+)/(?P<filename>.+)$',
+ 'wiki.views.raw_file',
+ name='wiki.raw_file'),
@groovecoder
Mozilla member
groovecoder added a note Jul 6, 2012

should these be in wiki views? I guess then they'd be /en-US/docs/files/* ?

@groovecoder
Mozilla member
groovecoder added a note Jul 6, 2012

in any case, we should exempt these urls from the url prefixer? (I forget how to do this)

@ubernostrum
ubernostrum added a note Jul 6, 2012

They need to be top-level since they don't really want to end up under docs/ and subject to the documentation URL hierarchy. Agree that we need to keep the prefixer off 'em, and I don't actually know how to do that.

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

Closing in favor of PR 353.

@ubernostrum ubernostrum closed this Jul 6, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.