Page MenuHomePhabricator

WWT: Refactor the relationship between the RevisionPopupWidget, Model and Controller for clarity
Closed, ResolvedPublic

Description

As a WWT user, I want the revision details pop-up to be implemented in an efficient and thoughtful manner, so that I can use the tool with ease.

Background: Quite a lot has changed since the original plan was created for the behavior of the Revision popup. Initially, it was simple and straight forward enough to have the majority of the logic exist in the small widget and update itself. However, as we went into caching promises and adding a model and controller, it is a good time to revisit the internal behavior of the revision popup and the way it presents itself with the cached API promises.

Acceptance Criteria:

  • Investigate the current implementation of the revision details pop-up to determine if:
    • There are any architectural issues or concerns that should be addressed
    • There are any security issues that should be addressed
  • Refactor the relationship between the RevisionPopupWidget, Model and Controller for clarity

Event Timeline

Restricted Application added a project: Community-Tech. · View Herald TranscriptDec 17 2019, 10:32 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
ifried updated the task description. (Show Details)Jan 2 2020, 11:09 PM
ifried updated the task description. (Show Details)

@Mooeypoo I think this has slightly changed the display of revision popups.

Before (commit f31f3be773cd4622e69b8f548837faa5e0695f11):

After:

That I can see:

  • There is more padding around the content
  • The edit summary is not in italics
  • The size diff is on a different line (and has no spaces around it)

To better show lack of spacing around size diff:

Before:

After:

dom_walden added a comment.EditedJan 10 2020, 6:41 PM

And I don't know what is going on here, edit summary cut off:

IPv6 addresses cut off (possibly also long contributor names) (regression compared to T232211):

Fix is in PR https://github.com/wikimedia/WhoWroteThat/pull/138

As per conversation with @ifried , I made the ipv6 usernames (and any username that is long) "wrap" to the next line. We can examine and see if we want an alternative approach.

Note that there are still a couple of things that we might need to tighten up. For example, In this page there is a diff with size 0 and no comment (I suspect the number of added bytes was the same as the number removed) which looks a bit weird, but hiding it isn't really a good option either. https://en.wikipedia.org/wiki/Haitham_bin_Tariq_Al_Said (click on 1954 inside the parentheses in the first sentence to see this)

I think these are edge cases, some of them happened probably before this refactor, that we might need to slowly figure out.

MusikAnimal added a subscriber: MusikAnimal.EditedJan 14 2020, 4:52 AM

In this page there is a diff with size 0 and no comment (I suspect the number of added bytes was the same as the number removed) which looks a bit weird, but hiding it isn't really a good option either.

It used to show the diff size on the first line, but only if there was no edit summary (e.g. commit 095b311). I don't recall why that changed. Maybe we could give that another try?

OK I fixed the comments, and also fixed up the request from T242954: WWT Popup: Padding changes [x-small] so this is ready for re-review.

I ran a selenium script to check the correct data is in the revision popup for a small sample of articles from enwiki, eswiki and trwiki.

I also briefly smoke tested version 0.15.4.0, just to see if anything is obviously wrong.

A lot of testing of this has also been done in T231508.

There are a few more changes in https://github.com/wikimedia/WhoWroteThat/pull/138, below is taken from the change log:

As Ilana observed.

  • Wrap long usernames (and IPv6 usernames)

See, for example, ~/wikimedia/commtech/who-wrote-that/ipv6.png

  • Add spacing after diff numbers, but only if there is a comment

Compare above screenshot with, ~/wikimedia/commtech/who-wrote-that/withcomment.png

  • Bring back the italicization of the comment.

See above screenshot.

  • Place the size diff at the intro line if there's no comment, or near the comment if that comment exists.

See above two screenshots.

There is a bug with hidden edit summaries, raised as T246203.

  • Set padding at 0.75em and 0.5em for the popup (see T242954)

Tested with T242954.

WWT version tested: 0.15.4.0 and commit 570b32217db2351afe511ce0bde1db9837676875.

ifried closed this task as Resolved.Feb 26 2020, 5:59 PM
ifried moved this task from Product sign-off to Done on the Community-Tech (Kanban-Q3-2019-20) board.

The refactor work has been released, so I'm marking this work as Done.