Problem/Motivation
Now that entity translation is in, it it's a bit of a pain to have to edit something (or view it) and then click on the translation tab. It would be nice to be able to go to the translation overview right from listings.
Proposed resolution
Include translation operation in overviews for translatable entities
Remaining tasks
- (done) ui review. @Bojhan agrees to premise in #2 via irc
- (done) first patch. @Lukas von Blarer in #9
- (done) follow-up patch with final approach by @ancamp in #12, plus further followup rerolls
- (done) reviews by @Schnitzel @Gábor Hojtsy @attiks @YesCT @plach
- (done) screenshots by @YesCT in #20 and #58
- (done) tests by @Schnitzel in #23
- (done) showing just test fail by @nagwani in #26, and later again in #42
User interface changes
API changes
done.
Comment | File | Size | Author |
---|---|---|---|
#58 | add-s01-notrans-2012-11-21_1836.png | 78.48 KB | YesCT |
#58 | add-s02-with_trans-2012-11-21_1839.png | 85.19 KB | YesCT |
#57 | interdiff-42-47.txt | 981 bytes | YesCT |
#57 | history.txt | 1.54 KB | YesCT |
#55 | drupal-add_translation_links_to_overviews-1832778-54.patch | 11.19 KB | Schnitzel |
Comments
Comment #1
Gábor HojtsyI think the hard part about it will be that you need to check per entity on each row. The set of operations will possibly be different for different nodes based on whether they are translatable or not (have a base language, node type configured to be translatable, etc). Same for taxonomy terms, etc. I think the visibility/access rules for the menu tab would need to be applied essentially.
Comment #2
Gábor Hojtsy@Bojhan agrees (via IRC).
Comment #3
Lukas von BlarerI agree with that. Should I give it a try? Where can I start?
Comment #4
Gábor Hojtsy@Lukas: I'd look into how are the node listing, taxonomy listed, user listing, comment listing, etc. are handled and add the translation option if the entity is translatable.
Comment #5
ancamp CreditAttribution: ancamp commentedComment #6
ancamp CreditAttribution: ancamp commentedI have been looking into the code to figure out how to modify the list:
node.admin.inc
node.api.php
translation.module
translation_entity.module
I am still learning, and i will get back to it tomorrow!
Comment #7
attiks CreditAttribution: attiks commentedFor nodes have a look at node_admin_nodes inside node.admin.inc around line 550
Comment #8
Lukas von BlarerWe should try to solve #1783964: Allow entity types to provide menu items before looking further into this.
Comment #9
Lukas von BlarerI created a patch with ytsurk. We implemented a part of #1783964: Allow entity types to provide menu items to try to see if this approach makes sense. What do you guys think? Is this the right way to go?
Comment #10
ytsurkthe idea is using the getOperations of the EntityListController.
any entity can provide its own ListController.
if an operation needs to be added to a different entity, we introduced the hook_entity_operations.
there is a check missing so far, to recognize to what entity the operation should be added.
not sure if this would be the best solution ..
Comment #11
Gábor HojtsyErm, I don't think it would fly to add a list controller but not actually use it. These listings will most probably be implemented as VBO views in core. That is being proposed in #1828410: Provide a bulk_form element for actions. So implementing these as list controllers does not actually seem to advance towards a later goal either (which would maybe a possibility to argue for why we have them only minimally implemented and not much used).
So I think for now to make this work to improve usability tremendously, we want to just go and add the operations conditionally. The views/VBO conversion will later figure out how to convert this. We don't need to make this depend on anything.
Comment #12
ancamp CreditAttribution: ancamp commentedI have added a translation link form the drop down OPERATIONS list in admin overviews if the entity (node/user/comment/taxonomy) is translatable.
Comment #13
YesCT CreditAttribution: YesCT commentedGabor and I checked to see if TestEntity (test_entity) had a listing page to do the same thing there and there is no listing page for TestEntity, so that's ok.
Still needs a code review, a manual test and screenshots of the new screen.
Comment #14
Schnitzel CreditAttribution: Schnitzel commenteddid a Short review directly on the screen with @ancamp and found some minor changes.
Comment #15
ancamp CreditAttribution: ancamp commentedApplied the changes given by Schnitzel.
Comment #16
ancamp CreditAttribution: ancamp commenteduups, forgot to upload the patch.
Comment #17
attiks CreditAttribution: attiks commentedForgot to mark it as RTBC, good work
Comment #18
ytsurknot a very generic solution - but definitely the intended behavior for the 4 included entities ..
or maybe - we wanted to go too far ;)
Comment #19
Lukas von BlarerGreat to see progress on this! ytsurk an me kind of wanted to demo the list controller to see how well it does work with content listings instead of settings listings. But let's in this case look at this later on.
Comment #20
YesCT CreditAttribution: YesCT commentedHere is what the lists look like with translate added to the dropbuttons.
Node list
Term list
User list
Comment list
(Note: I could not find the comment list and had to have attiks tell me where it was. I didn't even know there was a comment list. It's nice!)
Comment #21
webchickI think Gábor wanted to look at this first.
Comment #22
Schnitzel CreditAttribution: Schnitzel commentedand I'm writing tests currently :)
Comment #23
Schnitzel CreditAttribution: Schnitzel commentedUpdated patch with Tests :)
Testing translation links for:
- Terms
- Users
- Nodes
- Comments
also testing that for bundles which are not translatable there are no translation links (expect User as there are no bundles)
Comment #24
YesCT CreditAttribution: YesCT commentedThere is more to do over all, but there is a small part of this that could be a novice task: uploading a version of the patch from #23 which is just the tests without the fix so the testbot can show it fails.
Comment #25
nagwani CreditAttribution: nagwani commentedUpdated patch after removing fixes so the testbox can show it fails
Comment #26
nagwani CreditAttribution: nagwani commentedAdding interdiff
Comment #27
YesCT CreditAttribution: YesCT commentedJust a quick note of something. I did not look through the tests in detail.
Both of these say ContentAdminPage. I think the one for comments should say
function testTranslateLinkContentCommentAdminPage()
or maybe just
function testTranslateLinkCommentAdminPage()
Comment #29
YesCT CreditAttribution: YesCT commentedThese are questions I had about the patch (fix only) from #16. I dont think anything is really wrong, but @ancamp if you can answer the questions, that would be nice; I'm curious.
this is a picky whitespace thing, but in the comment.admin.inc is the only time the blank line is added between the new block and the $options line that follows. The node, taxonomy and user do not have that blank line. So I would suggest either take out this blank. Or, also add it in the other files if the blank line is helping readability of the code you are adding in by separating it a bit from near code.
comments and users are adding a links['translate'] array, and nodes and terms are adding a operations['translate'] array.
The screenshots in comment #20 show translate being added to each "OPERATIONS" column.
And why is user the only one to use $translation_enabled instead of module_invoke()? Maybe because users do not use a bundle?
why the change from drupal_get_destination() to destination()? Just to make it consistant with the other tests?
this seems like an unrelated change.
Oh, I see. You are making it consistant with the single quote pattern used in the other $operations arrays.
I remember we talked about this at badcamp with Schnitzel and xjm. They said if it was very close to a block of code you were adding and related, that it made sense to go ahead and fix it.
Still at needs review because @webchick wanted to see what Gabor had to say in general about the fix or the tests. But I wonder if he already said it in #11.
Comment #30
Gábor HojtsyI think the main question and what was mentioned above as "patch is not too generic" is that we do these links in the modules themselves instead of making translation_entity.module do it for them. The reason for that is that translation_entity.module just cannot know about all the entity listings and their formats and where would the links go in each list and how. Eg. there are file entities, but no listing. So if a contrib module implements entity types, it would need a reproducible pattern from core. If core makes it look like translation_entity can do this for others on behalf of them, and it does not work for abitrary entity listings (how would it?), then that is a wrong message. So that is why I agree putting this into specific modules makes most sense.
Also, ideally the entity/content listings would be VBO views in core, so the whole thing we do here will be refactored to serve there and then the operations will move to some kind of operation method somewhere. They are not there now, and overloading this issue to add controllers and operations callbacks of all kinds I think would be too much, especially since we don't know how VBO is related.
So I think for the applicability to other contribs, this approach in the patch is well suited. It does not look elegant, but all other operations are also defined locally and introducing a custom operations override method would be overkill at this point, using form_alters would not be generic since the forms have slightly different structures.
Comment #31
YesCT CreditAttribution: YesCT commentedI'll do two little things (the test name from #27 and the white space from #29 since I complained about them. :)
Comment #32
plachI agree with Gabor: for now we don't really have a cleaner solution for this. We might want to revisit the implementation once we have a more generic and unified API to build on. Meanwhile this is a nice UX improvement that it's good to have in.
Using
module_invoke()
is a more elegant solution than what we are doing now in the regular comment display (we could fix that one too), however we should be invokingtranslation_entity_translate_access()
instead, otherwise we aren't checking that the user is actually allowed to visit the translation overview. Currently clicking the link may result in a 403.Same as above, but why we need a variable here instead of just putting the call in the if test? This would be more consistent.
Comment #33
plachComment #34
Schnitzel CreditAttribution: Schnitzel commentedI have some answers to @YesCT and @plach
@plach:
performance, calling it once is much much faster then all the time for each User. As there are no bundles for Users you can only enable translations once for Users and we don't have to check it every time a user is shown.
oh, awesome found :) @YesCT when you are changin all the things already could you do this directly also? If not assign to me later then I will do this
@YesCT
exactly same question from plach, reason: performance.
Performance too, the destination is the same for every node, so why loading it for every node.
Comment #35
plachSorry, I missed the loop :)
Comment #36
YesCT CreditAttribution: YesCT commentedNot going to get to this tonight. So unassigning it form myself. I'll check in on it when I can come back to it, but if someone else addresses the reviewer comments first, that's ok. :)
Comment #37
Schnitzel CreditAttribution: Schnitzel commented- fixed things from YesCT in #27 and #29
- now using translation_entity_translate_access() as suggested in #32, which also means now that we have to run translation_entity_translate_access() for every single user. Als had to fix the tests as they did not have in: 'translate any entity'
Comment #38
Schnitzel CreditAttribution: Schnitzel commentedOne thing I really hate to do:
but if not
$entity->entityType();
intranslation_entity_translate_access()
does not work.We should definitely use full Entities in user_admin_account(), but to change all this would be too much of work, so I would suggest to create follow ups for this.
Comment #39
YesCT CreditAttribution: YesCT commentedComment #40
YesCT CreditAttribution: YesCT commentedsorry. cross post.
Comment #41
YesCT CreditAttribution: YesCT commentedI'm reviewing this. Found some comment grammar nits I'll fix. I'll also post a patch and a just tests patch.
Comment #42
YesCT CreditAttribution: YesCT commentedHere is the whole patch. (it passes locally). I have some questions about it... so I'll post a review of my own patch in a few.
There is also a tests only patch that should fail (it fails locally).
There is also an interdiff to 37: I added period to the end of // comment sentences, and blank likes to before the class closing that were removed when the tests for this were added, and I changed non_translatable to untranslatable to match what was used in the main et-ui issue.
There is also a just cleanup and fix file ... just for completeness in case someone wants to look at that.
Comment #43
YesCT CreditAttribution: YesCT commentedJust a question: Are pages by default in the et-ui tests not translatable and articles are translatable?
Needs work because...
user is the only test added here that does not check that translations is not added when appropriate.
I think this is because users are either all translatable or all not. So, I think the test should also disable translation for users and test with assertNoLink that the translation link is not added to the user list.
Next
unassigning myself so @Schnitzel (or someone else) can respond or fix.
Comment #44
Schnitzel CreditAttribution: Schnitzel commentedyes, Entity Translation UI Tests which are used define this by default (not drupal core default).
Not my opinion: The Tests are testing if a new feature implemented (the translation links) is working correctly.
The Entity Translation UI Tests which are already in, also do not Test if Access is successfully denied if translation is not enabled to an Entity.
As we are here only adding links and not real functionality I would suggest that we make a follow up to create better test coverage for Entity Translation UI and add these Tests there.
back to needs review.
Comment #45
YesCT CreditAttribution: YesCT commentedgiven my questions were answered, RTBC.
Comment #46
Gábor HojtsyMinor code style violation here. Looks good otherwise.
Comment #47
rvilarHere is a patch that fixes comment on #46
Comment #49
Gábor Hojtsy#47: drupal-add_translation_links_to_overviews-1832778-47.patch queued for re-testing.
Comment #51
YesCT CreditAttribution: YesCT commented@rvilar (or other) an interdiff between #42 and #47 might help.
Comment #52
rvilar@YesCT I only remove the ';' character that Gabor shows in comment #46
Comment #53
Gábor Hojtsy@rvilar: you know that is funny because I did not notice that even. I noticed another issue which is still not solved (see below). But wondering why would it not pass tests now :/
The other code style problem was not solved, the indentation is still off on the closing parenthesis.
Comment #54
Schnitzel CreditAttribution: Schnitzel commentedrecreated the patch of #42 and only changed the codestyle issues, see interdiff.
Comment #55
Schnitzel CreditAttribution: Schnitzel commentedComment #56
attiks CreditAttribution: attiks commentedComment #57
YesCT CreditAttribution: YesCT commented@rvilar Thanks a lot for jumping in here. It was a mystery why the bot had trouble. But, I think the RewriteBase might have been the cause.
(Now I had to roll back to Nov 10 to get the old patches to apply and I'm no git expert, so I might have made an error... but I tried to sort it out. See the history.)
Comment #58
YesCT CreditAttribution: YesCT commentedI'll update the summary too.
in #21 webchick asked for Gabor to look at this.
Gabor addressed the approach in #30
plach and Schnitzel had a nice look and back and forth
then Gabor looked again #46
So I think it's been looked at well.
I did another manual test just to make sure the latest stuff is good.
More screen shots from before are in #20
But here is a quick reshot with patch from 54, first without translation enabled and then with translation enabled.
with translation enabled
Comment #58.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary to show the recent screenshot and use the issue summary template to make it easy for a committer to see what the status is.
Comment #59
Gábor HojtsyWoot, let's get this in :)
Comment #60
Dries CreditAttribution: Dries commentedLooks good to me. Committed to 8.x. Thanks.
Comment #61
YesCT CreditAttribution: YesCT commented#1848164: Provide translate option for entities in contextual links. is kind of related
Comment #62.0
(not verified) CreditAttribution: commentedUpdated issue summary clarify that no api changes were made.