Opened 7 years ago
Last modified 7 years ago
#5360 review enhancement
Adding Collections to addmedia and batchaddmedia, and tags to batchaddmedia.
|Reported by:||Daniel Krol||Owned by:|
The addmedia command allows you to specify tags to add to your submitted item, but batchaddmedia does not. Further, neither of these commands allow you to specify a collection to add your items to. I'd like these features to be added (and in fact I'm working on a patch to do just that).
For reference, here is a mailing list conversation that I started on the topic. (I'm moving the discussion here to reduce noise on the list): http://lists.mediagoblin.org/pipermail/devel/2015-November/001305.html
Current status is that I have the feature working, and I even added some tests for these two commands (which afaik were missing). However there are some error cases that I am trying to avoid introducing. I'll put details in the comments, since it's still getting hashed out. You can probably follow my progress here: https://github.com/orblivion/mediagoblin/compare/batchaddmedia-collection-tag-on-master?expand=1
Change History (8)
comment:1 by , 7 years ago
comment:2 by , 7 years ago
Or, maybe there's an assumption that you won't have hidden, unprocessed things attached to collections? Maybe we should just leave it as such after all, so the user can see it and manually remove it from the collection?
Ideally of course I just wouldn't add it unless it's processed. The problem is that (especially if the user chooses
--celery) I can't prevent it in a straightforward manner.
comment:3 by , 7 years ago
And as far as ordering within a collection view, I was wrong. Though it doesn't seem to set a
position value when you add an item to the collection (again, even in the existing web UI), it does attempt to use the
position field for ordering. Here's the relevant code:
def get_collection_items(self, ascending=False): #TODO, is this still needed with self.collection_items being available? order_col = CollectionItem.position if not ascending: order_col = desc(order_col) return CollectionItem.query.filter_by( collection=self.id).order_by(order_col)
And here's what I tried changing it to, to have it default to
id in lieu of
def get_collection_items(self, ascending=False): #TODO, is this still needed with self.collection_items being available? order_cols = CollectionItem.position, CollectionItem.id if not ascending: order_cols = map(desc, order_cols) return CollectionItem.query.filter_by( collection=self.id).order_by(*order_cols)
This makes the weird bug with the first item showing up out of place go away. The weird thing (to me) is that it's descending by default, just like (for instance) the tag view. I would have expected that collections show up in ascending order. Are collections supposed to be descending? Interestingly it happens to show up in (more or less) ascending order before I make this change. I guess it defaults to order_by(ascending id kindof).
Anyway, maybe I'll make it a personal TODO to make another PR with changes to this view, regarding unprocessed items, and item ordering.
For now I'll run with the assumption that all of these issues can be pushed off to other patches, and I'll go ahead and squarsh my changes and change this ticket to review status.
comment:4 by , 7 years ago
|Status:||new → review|
comment:5 by , 7 years ago
I just force pushed a couple fixups. Here's the current hash, in case I decide to change it again: https://github.com/orblivion/mediagoblin/commit/2056d5d5f23645e15fbc097ad59fc224748dda40
comment:6 by , 7 years ago
Sorry, force pushed some more. Currently: https://github.com/orblivion/mediagoblin/commit/e28a8677fff6022eca728053db898ee4298b1b51
comment:7 by , 7 years ago
comment:8 by , 7 years ago
This branch is batchaddmedia-collection-tag-on-master, just for clarity.
Some notes on the outstanding issues I mentioned on the list, and a couple others:
1) Regarding celery allowing silent transcoding errors: As I mentioned to you on the list, my initial plan was to have it give a warning, or "are you sure y/n", if you try to do the batch command with both celery and collections. You then decided that there should be something like a "skip warnings" option in case the command is run as part of a script. I decided that rather than add a bunch of weird options and functionality it's simpler to just add a simple warning to
--celeryin the help text. It's in my WIP on github. Thoughts?
2) The Collection view seems to show unprocessed media entries. This is probably because until my patch, it was impossible to add to a collection other than in the web UI, and the web UI didn't let you work on items until they were fully processed. So, you never had a need to filter these unprocessed items out. I think I will have to fix this along with this patch (http://i.imgur.com/D0daEJW.gifv) lest I introduce a bug.
3) That said, these unprocessed items raised my eyebrow a bit when I saw them in my big vacation collection. These unprocessed items were in the middle of my collection, where I ran into some video transcoding problems as I was doing my import. It lead me to notice the following weirdness, though I can't say for sure whether it's an unrelated bug: It seems that in the collection view, when I'm on page x where x > 1, the first item in the collection shows up as the last item on the page. In fact, it shows up _instead of_ the item that ought to be there. I freaked out because I thought that I was missing that last-item-in-the-page, but it turned out that the item wasn't actually missing, nor did it fail to get added to the collection. It's just a bug that manifests in the collection view for me right now. For the record, my mediagoblin instance is on
v0.8.0+ an old version of my changes, though I accidentally partially migrated my db to what was on
masterabout a week ago (it had some problems migrating), which may put this in a weird state.
4) Ordering seems to be unenforced in collections anyway(?). I tried adding a collection item by hand (this time in a legit
master, not my weird deployed instance) and the
None. From a quick look, I didn't see
positionused in any sort of view filter. My conclusion was that this is a "TODO" for you folks; is that correct? So perhaps I shouldn't worry *so much* about this just now? I do want to make sure that once it is implemented, it's considered for
batchaddmedia. I suppose you'll grep around for
Collectionwhen that happens. Though, maybe while I try to fix the "showing unprocessed items" bug, I can also at least add an order-by-timestamp or something, as a temporary fix to the problem I describe in
3)? Otherwise I'll leave it alone.
5) On the list I also mentioned a "continue where you left off" hint for when you're doing
batchaddmediaand you get an error when in the middle of a csv file. While it's related to my change and relatively simple to do, I believe is out of the scope of this bug because it's not strictly a response to a new problem I'm introducing. I'd rather put it in a subsequent patch. Let me know if you disagree.