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

Panels do not handle sub-versions correctly #4986

Open
Jakob37 opened this issue Oct 30, 2024 · 6 comments
Open

Panels do not handle sub-versions correctly #4986

Jakob37 opened this issue Oct 30, 2024 · 6 comments

Comments

@Jakob37
Copy link
Collaborator

Jakob37 commented Oct 30, 2024

Describe the bug

When sub-versioning a panel 1.10 or 1.20 the version is handled as a float, truncating the zero.

Furthermore, version 1.9 seems to be ranked "higher" than for instance 1.11, making me think it is doing a numerical comparison to check the latest panel.

Here, adding first 1.2 and then 1.20. You see that it checks for whether 1.2 exists when adding 1.20 and aborting as it already is there.

panel_versioning

After adding also version 1.11, 1.9 still shows up as the most recent version. (I verified that after adding 1.91, it is indeed updated to show 1.91).

scout_panel_view

To Reproduce

Create a panel with version 1.1, and then 1.10 to observe the first issue.

Then create a panel with 1.9 and subsequently 1.11 to observe the second issue.

Additional context

I am running Scout version 4.90.1 during testing.

@dnil
Copy link
Collaborator

dnil commented Oct 30, 2024

That seems very plausible. Adding single panel version via CLI is very rare here nowadays, so can easily see this slipping past.

@dnil dnil added the bug label Oct 30, 2024
@northwestwitch
Copy link
Member

Have you started with this one @dnil ? Otherwise I can!

@northwestwitch northwestwitch self-assigned this Oct 30, 2024
@northwestwitch
Copy link
Member

There is no way that panel 1.20 can be saved into the database as a float, because it will always become 1.2. I guess it needs to be cast as a decimal (pymongo has a specific type named Decimal128), but then we need to make sure that floats and decimals coexist and become comparable throughout the code, both python and jinja.

@northwestwitch
Copy link
Member

northwestwitch commented Oct 30, 2024

Meh, I don't really know how to fix this without messing up the database. We either:

  1. Decide that from now on all panel versions become decimals instead of floats, and then we run a script to convert all panel versions to decimals in the database and adjust the scout code accordingly -- I don't like this option because it has the potential to introduce bugs, among other issues, like the fact that it is incorrect anyway to render a version number as a float, because what if you wanted to have version 1.20.1 ? --
  2. Force the user that is updating panels using the command line to provide a version which will work fine as a float
  3. Remove the --version entirely as an option from the command line, so that versions are handled by scout itself

Option 3 seems the easiest to me. Any suggestions?

@Jakob37
Copy link
Collaborator Author

Jakob37 commented Oct 31, 2024

Meh, I don't really know how to fix this without messing up the database. We either:

1. Decide that from now on all panel versions become decimals instead of floats, and then we run a script to convert all panel versions to decimals in the database and adjust the scout code accordingly _-- I don't like this option because it has the potential to introduce bugs, among other issues, like the fact that it is incorrect anyway to render a version number as a float, because what if you wanted to have version 1.20.1 ? --_

2. Force the user that is updating panels using the command line to provide a version which will work fine as a float

3. Remove the --version entirely as an option from the command line, so that versions are handled by scout itself

Option 3 seems the easiest to me. Any suggestions?

Hmm. Assigning version directly have been useful to us, when restoring panels which accidentally was removed. Sounds a bit unfortunate to completely remove it.

For me, point 2 sounds better. And being very clear in the scout load panel interface on the expected format. I guess the issue I encountered here could be seen more like a misunderstanding of the expected format than a bug.

Might still lead to misunderstanding with 2.1 vs 2.01 though 🤔 Not sure how to get around that without more drastic changes.

@northwestwitch
Copy link
Member

Hmm. Assigning version directly have been useful to us, when restoring panels which accidentally was removed. Sounds a bit unfortunate to completely remove it.

For me, point 2 sounds better. And being very clear in the scout load panel interface on the expected format. I guess the issue I encountered here could be seen more like a misunderstanding of the expected format than a bug.

Might still lead to misunderstanding with 2.1 vs 2.01 though 🤔 Not sure how to get around that without more drastic changes.

Mmm this is a clear example of poor judgment when the functionality has been created. The easiest would have been accepting only integers, which is what happens when you create/update panels using the web interface.

I've been testing a bit on my new PR but there is no really a way around the problem that 1.2 is the same numbers as 1.20 if you are either using floats or decimals. Things change if you use strings, but you don't want to go there because then you'll lose the possibility of sorting by numerical version.

I need to think a bit more about this

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

Successfully merging a pull request may close this issue.

3 participants