Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Rockon] when adding storage pre-addition summary screen displays incorrectly #2933

Closed
Hooverdan96 opened this issue Dec 9, 2024 · 6 comments
Assignees

Comments

@Hooverdan96
Copy link
Member

As a continuation of #2904 that was fixed by #2914:

Thanks to forum user stevek who observed a similar behavior when adding storage to an installed Rockon:

https://forum.rockstor.com/t/syncthing-add-storage-function/10321/

I assume, it's probably the same root cause, just in a different place of the coding.

@phillxnet phillxnet added this to the 5.1.X-X Stable release milestone Dec 9, 2024
@FroggyFlox
Copy link
Member

Following the description of the issue in the forum thread linked above, we indeed see the following as the summary of the changes to be instantiated after the "Add Storage" procedure:
image

The Backbone template is

The display of newly-added volumes is thus defined by the handlebars helper: display_newVolumes, itself defined in:

initHandlebarHelpers: function() {
Handlebars.registerHelper('display_newVolumes', function() {
// Display newly-defined shares and their corresponding mapping
// for confimation before submit in settings_summary_table.jst
var html = '';
for (share in this.new_volumes) {
html += '<tr>';
html += '<td>Share</td>';
html += '<td>' + this.new_volumes[share] + '</td>';
html += '<td>' + share + '</td>';
html += '</tr>';
}
return new Handlebars.SafeString(html);
});

We could thus swap the 2 rows in questions to fix the bug, but this would most likely just cover the real source of the "bug" here. We indeed, use the construct for newLabels and newCnets and these are fine. We should thus probably fix the mapping of share:internal_representation.

@FroggyFlox
Copy link
Member

We should thus probably fix the mapping of share:internal_representation.

This is defined at:

this.share_map = this.model.get('shares');
this.share_map[this.$('#volume').val()] = this.$('#share').val();
this.model.set('shares', this.share_map);

Swapping the key:value here as follows...

        this.share_map = this.model.get('shares');
        // this.share_map[this.$('#volume').val()] = this.$('#share').val();
        this.share_map[this.$('#share').val()] = this.$('#volume').val();
        this.model.set('shares', this.share_map);

... then rebuilding the static files...

cd /opt/rockstor
poetry run django-admin collectstatic

... and we now have:
image

FroggyFlox added a commit to FroggyFlox/rockstor-core that referenced this issue Dec 27, 2024
The internal:external representation of volumes:shares in the summary
table showed at the end of the Add Storage to a Rock-On process was
the inverse to what it should have been (inverse of table headers).
This commit swaps the order in the underlying JS object to correct this.
@FroggyFlox FroggyFlox self-assigned this Dec 27, 2024
@FroggyFlox
Copy link
Member

While the table is now displayed, properly, the update process itself fails, however:
image

This is due to the fact that our backend is currently expecting the dict to be

key: volume_name
value: share_name

While it would be more appropriate to be consistent across our various update objects here, the amount of refactoring required in our backend does not fit the current very late stage in our testing phase. I thus favor the first option laid out early in this thread: adjusting the display_newVolumes handlebars helper. @phillxnet, would you agree here?

FroggyFlox added a commit to FroggyFlox/rockstor-core that referenced this issue Dec 27, 2024
The internal:external representation of volumes:shares in the summary
table showed at the end of the Add Storage to a Rock-On process was
the inverse to what it should have been (inverse of table headers).

This commit adjusts the Handlebars helper used to display the newly
added volume to swap the internal:external representations.
@phillxnet
Copy link
Member

@FroggyFlox Re:

While it would be more appropriate to be consistent across our various update objects here, the amount of refactoring required in our backend does not fit the current very late stage in our testing phase. I thus favor the first option laid out early in this thread: adjusting the display_newVolumes handlebars helper. @phillxnet, would you agree here?

Agreed. We can address the referenced inconsistencies when we get to switching out some of our interface technologies. As you say, we should try to avoid deeper changes at the end of a testing phase.

@FroggyFlox
Copy link
Member

Thanks for the feedback!
I'll move the associated PR from "draft" to "ready" now that we agree on the approach, then.

phillxnet added a commit that referenced this issue Dec 28, 2024
…rrect-summary

Swap order of newVolumes representation in summary table #2933
@FroggyFlox
Copy link
Member

Closing as fixed by #2942.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants