Page MenuHomePhabricator

When saving collection settings and then clicking the back icon, page should refresh.
Closed, ResolvedPublic

Description

When editing a collections settings, Pressing the save button then the back button to exit the edit overlay, The page doesn't refresh to reflect the latest changes. Hitting the save button in settings and save button in the edit overlay has the correct behavior. However, when changing the collection name it does not reload the page with the udpated collection name in the url.

Solution:
Refresh the page when saving and hitting the back button.
If the collection name changed, reload the page with the updated name in the URL

Event Timeline

rmoen claimed this task.
rmoen raised the priority of this task from to Needs Triage.
rmoen updated the task description. (Show Details)
rmoen added a subscriber: rmoen.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 26 2015, 9:13 PM
rmoen updated the task description. (Show Details)Jun 26 2015, 9:16 PM
rmoen set Security to None.

Ping @KHammerstein. You should be aware of this bug but there's no Design input needed.

Refreshing should be the last resort as it's time consuming. Why can't we re-render the changed parts using javascript?

@bmansurov this seems like a great idea for a feature.

I don't think my suggestion is a feature. It's an alternative solution to yours.

@bmansurov. The current behavior when making an edit to a collection is reloading the collection to refresh the changes. This bug is simply to fix what is expected after you make a change.

What you are suggesting is something that should be properly added to a sprint and estimated as a normal feature would be. As it is not as trivial as fixing the expected behavior.

Change 221772 had a related patch set uploaded (by Robmoen):
Reload collection when exiting overlay after making changes.

https://gerrit.wikimedia.org/r/221772

phuedx added a comment.EditedJun 30 2015, 9:33 AM

I think @bmansurov's suggested enhancement is a good idea and certainly worthy of investigation. You could even use [history.pushstate](https://developer.mozilla.org/en-US/docs/Web/Guide/API/DOM/Manipulating_the_browser_history#Adding_and_modifying_history_entries) to update the URL if the collection's name has changed.

However, I agree with @rmoen that, for now, we should use what's currently used elsewhere, since this is a relatively minor bug in a feature that's currently on the master branch.

phuedx added a subscriber: Jdlrobson.

Moving to -1 (Needs More Work) per @Jdlrobson's review.

OK. I noticed these refreshes in a couple of other places too. I don't think this should be default behavior. By default we should minimize the wait time and re-render the view without leaving the page.

@bmansurov: I agree. I think the ex-Gatherers will agree with you as well. Perhaps it simply wasn't prioritised when they were building the prototype.

If you are talking about ajax refreshing that is far too complicated in current implementation :/ should be captured in a task somewhere

@Jdlrobson: Does it even need to be AJAX? If the save is successful, then you already know what updates are going to happen (you've just sent the data to the server and it's been saved).

Yes, imagine a collection with 100 items. We paginate on 50. Say you edit the collection and remove item 1-50 and save... you now need to know to show 50-100.

I haven't actually tested this in detail but I think we only show the first 50 members of a collection in the edit screen. Would be good to test it.

Anyway this is just one example I can think of but I think there were other subtle headaches we experienced previously so I'd rather we didn't do this on a whim and gave some thought and prioritisation beforehand in a separate task :).

@Jdlrobson Per your feedback, I added browser tests for changing a collection name. There is another edge case to test for when clicking the back button. I hope this one test satisfies until next week, then I can write the edge case test. We need coverage for delete as well, and it would be helpful to delete the collection this test changes the name of because if we don't it will cause the tests to fail as it will not be able to edit another collection the same name if one already exists with that name. Either that, or change the name back to the original name. I still haven't figured out how to best go about that though as I'm just barely learning ruby/cucumber. I hope this made sense, it's late and I'm tired.

Change 221772 merged by jenkins-bot:
Reload collection when exiting overlay after making changes.

https://gerrit.wikimedia.org/r/221772

Jdlrobson moved this task from Needs triage to In sprint on the Gather board.Jul 3 2015, 6:33 PM

Change 222479 had a related patch set uploaded (by Jdlrobson):
QA: Add browser test for changing a collection name

https://gerrit.wikimedia.org/r/222479

phuedx added a comment.Jul 6 2015, 2:45 PM

Verified on BC. Just waiting on T104720

Change 222479 merged by jenkins-bot:
QA: Add browser test for changing a collection name

https://gerrit.wikimedia.org/r/222479

phuedx closed this task as Resolved.Jul 7 2015, 4:05 PM
phuedx moved this task from Ready for Signoff to Done on the Reading-Web-Sprint-51-YOLO board.