IQSS / dataverse

Open source research data repository software
http://dataverse.org
Other
877 stars 485 forks source link

Dataset Publish - Missing success msg + broken tooltips and other js rebind functions + unfortunate "DRAFT was not found" on page reload #7653

Open mheppler opened 3 years ago

mheppler commented 3 years ago

~Two~ THREE issues, the green dataset publish success msg has gone missing, the tooltips and other js rebind functions are broken, and when I reload the dataset pg in the browser after said "silent" success, there is a big, yellow, unfortunate "DRAFT version was not found" warning msg, because the URL reads "version=DRAFT" and not "version=1.0"

Saw this on localhost and reproduced on demo (5.3)

  1. create a draft dataset... save
  2. publish... see yellow "publish in progress" lock warning msg
  3. see page spinner check the system if dataset lock goes away...
  4. page sees dataset lock is removed.. lock warning is removed... blue "DRAFT" label under title changes to "Version 1.0"...

And that's it.

No big green SUCCESS msg let you know you're dataset is published for the world to see. You have to piece together that the removal of the lock msg, and the changing of the status label (if you notice it) is "visual indication" enough.

The bundle has a dataset publish success message, dataset.message.publishSuccess=This dataset has been published.

In DatasetPage.java (line 2532) there appears to be a call for it as well...

    private String releaseDataset(boolean minor) {
        if (session.getUser() instanceof AuthenticatedUser) {
            try {
                final PublishDatasetResult result = commandEngine.submit(
                    new PublishDatasetCommand(dataset, dvRequestService.getDataverseRequest(), minor)
                );
                dataset = result.getDataset();
                // Sucessfully executing PublishDatasetCommand does not guarantee that the dataset 
                // has been published. If a publishing workflow is configured, this may have sent the 
                // dataset into a workflow limbo, potentially waiting for a third party system to complete 
                // the process. So it may be premature to show the "success" message at this point. 
                if ( result.isCompleted() ) {
                    JsfHelper.addSuccessMessage(BundleUtil.getStringFromBundle("dataset.message.publishSuccess"));
                } 
                ...

This change appears to have been introduced to solve ~Unexplained publishing failures #7303~ Fix unnecessary locking in Authentication Service; improve Publish #6918, which required a 4.20 patch from @landreev, when he introduce the publish lock to the system. He has also informed me that the loss of the big green success msg was discussed and determined to be acceptable.

(UPDATE) Plus, all of the tooltips and other js rebind functions are broken.

With that said, I would also like to point a THIRD bug related to this. After the dataset is published, if you reload the page in your browser, you receive a big yellow "DRAFT was not found" warning msg, which is because the URL you reloaded in your browser is still "dataset.xhtml?version=DRAFT" after you have published, and not "dataset.xhtml?version=1.0".

Screen Shot 2021-03-04 at 4 42 11 PM
landreev commented 3 years ago

To clear some minor confusion:

This change appears to have been introduced to solve Unexplained publishing failures #7303

It was not #7303, the PR #7118, for the issue #6918 was the one. The behavior was not introduced in #7118 either: there was NEVER a success message, when publishing was being done asynchronously. But prior to 5.0, the asynchronous mode was only used when there was more than 10 files in the dataset. And starting with 5.0/the PR above, publishing is ALWAYS asynchronous. Hence no publish success message for anyone.

So yes, it was discussed prior to 5.0, and we agreed that it was an acceptable setup for the time being. If we want to improve it, sure we can try and figure out what needs to be done and how much work it is.

It's definitely possible to redirect the user to the new published version; so that they don't get the "no draft" message. But we would need to add some extra code, to verify that the publishing succeeded, and the published version exists. Otherwise we'll be back with the situation where, if publishing fails, the user gets sent to the last published version, instead of the unpublished draft - which is even more confusing. (we had issues opened for that; now closed since #7118 fixed it).

So a general problem with the asynchronous mode is that the page doesn't get the return status of the publish command directly. All it knows is that the dataset is no longer locked. So we'll need to add some extra logic there to figure out if it failed or succeeded.

mheppler commented 3 years ago

Thank you for the clarification, @landreev and apologies for misrepresenting any of the details we discussed. Appreciate your time to discuss expected system behavior and the history lesson on what happens behind the UI curtain during publish.

sbarbosadataverse commented 2 years ago

@scolapasta where is this in the list of topics? not sure what it means when it's ready for development...Thanks. Just seeking an update.

landreev commented 2 years ago

I'm wondering if I should just open a dedicated issue just for the "version=DRAFT" part, that results in the "DRAFT was not found" message on the page. People keep justifiably complaining about it; and I believe I can fix it very easily. The other parts here are less obvious. Like it's not clear whether the page really needs a "success" message on publish. (Being redirected to the successfully published version may be considered a sufficient confirmation of success - ?). But if we decide that it is needed, it's not clear how to do that (because of the async. nature of publishing).

As for the issue with version=draft that stays on the url, I believe we introduced it while fixing a more annoying and confusing behavior - it was the other way around: if an attempt to publish failed, instead of staying on the unpublished draft, the user was being redirected back to the last published version, if it existed. Do we actually need that "DRAFT not found" message at all? Should we quietly redirect to the last published version instead, if there's no DRAFT? - That would be so much easier than messing with that redirect-on-unlock...

sbarbosadataverse commented 2 years ago

Yes, please open.

On Mon, Mar 28, 2022, 5:48 PM landreev @.***> wrote:

I'm wondering if I should just open a dedicated issue just for the "version=DRAFT" part, that results in the "DRAFT was not found" message on the page. People keep justifiably complaining about it; and I believe I can fix it very easily. The other parts here are less obvious. Like it's not clear whether the page really needs a "success" message on publish. (Being redirected to the successfully published version may be considered a sufficient confirmation of success - ?). But if we decide that it is needed, it's not clear how to do that (because of the async. nature of publishing).

As for the issue with version=draft that stays on the url, I believe we introduced it while fixing a more annoying and confusing behavior - it was the other way around: if an attempt to publish failed, instead of staying on the unpublished draft, the user was being redirected back to the last published version, if it existed. Do we actually need that "DRAFT not found" message at all? Should we quietly redirect to the last published version instead, if there's no DRAFT? - That would be so much easier than messing with that redirect-on-unlock...

— Reply to this email directly, view it on GitHub https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_IQSS_dataverse_issues_7653-23issuecomment-2D1081186047&d=DwMCaQ&c=WO-RGvefibhHBZq3fL85hQ&r=8R6PzVqt1PEocauQgZMGXsGz29-nb19M7eqlo1d8EVs&m=pDeVzeDcXTCZQp6OvHZhhr80ci3vUR_xOksXFT_o5QQ1Lwkf0GNyvQ-hcco9-NG6&s=be_h_mLvrWWrMP_V1OPnkbTIktWTCkAFysM6MrMcFto&e=, or unsubscribe https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AB7P2KWNTKZ6FRSBLIP7X43VCISLRANCNFSM4YUBGPGA&d=DwMCaQ&c=WO-RGvefibhHBZq3fL85hQ&r=8R6PzVqt1PEocauQgZMGXsGz29-nb19M7eqlo1d8EVs&m=pDeVzeDcXTCZQp6OvHZhhr80ci3vUR_xOksXFT_o5QQ1Lwkf0GNyvQ-hcco9-NG6&s=h0AL9w5Fsy3vZCLgW6qaeQYN8IxF8nW6aCKrhQljevM&e= . You are receiving this because you commented.Message ID: @.***>

landreev commented 2 years ago