Closed (fixed)
Project:
Diff
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
19 Sep 2016 at 14:27 UTC
Updated:
5 Oct 2016 at 16:34 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
johnchqueTrivial patch.
Comment #3
miro_dietikerGenerally agree, we need to get rid off the duplication.
But since the summary has the format of text, it needs space.
Instead of exposing it as a separate "Summary" column, i would add it at the previous location (below the date) instead.
Also, to guarantee uniqueness, we should assertUnique once in tests.
Comment #4
miro_dietikerComment #5
johnchqueOk, gonna fix that then.
Comment #6
johnchqueShould be better now.
Comment #8
miro_dietikerWe should have updated the issue summary and provided a screenshot as it is a UI change.
So two confusions:
- The duplication: I realised the spaghetti code at RevisionOverviewForm is highly redundant and should and can be be unified.
- I don't like this key shifting for +1. And then i realised in RevisionOverviewForm, we are loading count($vids)-1 twice because we just pass in the previous revision id and then load it deep inside summary(). You try to delegate loading, but still need to maintain conditions if $key isn't existing. That's bad delegation. If it's the last revision, just don't pass in a second revision(_id) at all. But i think we should switch $previous_revision_id with $previous_revision and load early. Small API change although beta, but IMHO mostly internal.
Committing, back to needs work to create the issue follow-ups. Please close when issues created.
Comment #9
johnchque#2804021: Improve getRevisionDescription
#2804015: Refactor redundancy of RevisionOverviewForm
Created followups.