UI-9435 - Better handling of calculated measures #126
Conversation
There was a problem hiding this comment.
As mentioned in the PR description, this file just corresponds to splitting the actual logic of the function from the I/O.
The only logic change is the shouldUpdateCalculatedMeasures boolean passed in the migration from 4.3 to 5.0.
| * | ||
| * Mutates `contentServer`, `errorReport` and `counters`. | ||
| */ | ||
| export async function migrateContentServer({ |
There was a problem hiding this comment.
I like the move to isolate migrateContentServer from file system reads and writes and from console outputs, in order to make it more easily unit-testable.
But I am afraid that having two functions: migrateContentServer and migrateContentServerJson could introduce confusion in the future. So instead, I'd suggest to introduce a function outputErrors, to remove migrateContentServerJson and to inline the FS read/writes and the call to outputErrors directly in bin.
What's next is out of scope of this PR
but I believe that the atoti-ui-migration codebase already has a problem of legibility / maintainability and I don't expect our team members to be as efficient on it than on Atoti UI, precisely because of naming and confusing splits. The same applies to other parts, but for instance just to migrate 5.0 dashboards to 5.1, we go through:
- migrateContentServer
- getMigrateDashboards (which returns a function)
- migrate (which does not have a readily accesible JS doc as it comes from an iteration through a list, and which takes
migrateFooBarfunctions as arguments) - migrate_50_to_51 (one instance in this list)
- migrateDashboards (which takes serialization/deserialization functions as arguments)
- migrateDashboard (singular)
I don't think that this is necessarily too many functions, but I definitely think that it would be more approachable if these functions had names reflecting what they do, instead of each being called "migrateSomething", and if we could avoid complex patterns such as functions taking functions as arguments or returning functions.
This was just to explain why I think it's important to pay a particular attention to simplicity in this codebase at the moment. This is of course out of scope of this PR and we don't need to discuss it in depth now.
There was a problem hiding this comment.
migrateContentServer vs migrateContentServerJson does not seem confusing to me.
And it has the pro of keeping: one CLI command = one script (without extra work on top of it).
That being said, it's not a strong opinion, and I like the idea of encapsulating the logging of the outcome anyway 👍
To be consistent I did the same for migrateNotebook.
While doing this, I notice that migrateNotebook is not really generic contrarily to migrateContentServer.
We'd have to adapt it in order to make it work with several iterative steps.
Also, the outcome logging/error reporting should be the same IMO, except that it should point to notebook cells rather than content server files.
This is out of scope of the PR.
The current design was the outcome of a technical design session involving us two, Thibault and Elias.
I'm not sure if there is a more complete design document, but here is a Slack thread which shows us agreeing on this almost exact function split/naming.
If I remember correctly, our goal was to make it as easy as possible to write the actual migration logic, as opposed to worrying about the manipulation of entries on the content server and having to duplicate a lot of code/reinvent the wheel for each migration step.
And so we were ready to have some generic abstraction/indirections that are not the easiest to read, if it allowed to achieve this goal.
Which I think we did: as for example it's quite intuitive and nice to write one pure migrateWidget(legacyState): migratedState function per migration step, and the structure is consistent.
I'm not married at all to the current design and would be happy to discuss improvements, but if we do go for a redesign I think it's nice to remember our initial intent.
There was a problem hiding this comment.
I remember this session indeed. I've just felt like I needed way more time than I should in order to get productive in the recent work I've had to do here and it's tempting to look at refactoring without removing the isolution of the pure widgetState migration.
| filename: (pathData) => { | ||
| return pathData.chunk.name === "bin" ? "cli/[name].js" : "[name].js"; | ||
| filename: () => { | ||
| return "[name].js"; |
There was a problem hiding this comment.
Do we even need to specify this function to Webpack in this case?
Unrelated, but moving bin.js seems unrelated to this PR and also seems like it could introduce a bug during the next release. Did you test it?
There was a problem hiding this comment.
I tested with a repo setup like this:
{
"name": "migration-tests",
"version": "1.0.0",
"description": "",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
},
"dependencies": {
"atoti-ui-migration": "file://../atoti-ui-migration"
},
"keywords": [],
"author": "",
"license": "ISC"
}
where I point to my local atoti-ui-migration.
Running the command like on UI-8803 does execute the CLI and provide a migration output and report.
I agree that we should double check in the next release though.
| doesReportIncludeStacks, | ||
| onError, | ||
| behaviorOnError: onError, |
There was a problem hiding this comment.
Out of scope, since this problem existed before this PR, but this renaming of variable is an easily avoidable obstacle to debugging, because it means that from a function to the next, the variable is named one thing or another, creating an artificial difficulty during debugging.
Here, I would rename everything either onError or behaviorOnError.
But no need to do anything about it in this PR.
There was a problem hiding this comment.
Yes I completely agree. I seem to remember that we didn't do it earlier because we didn't want to break the migration commands for users of the CLI ?
As long as we update the doc too though I don't see that being a big issue.
And a meet-in-the-middle approach would be to use behaviorOnError as the command arg name, and put onError as an alias so that it doesn't break anything.
antoinetissier
deleted the
personal/a.tissier/UI-9435-better-calculated-measures-handling
branch
See UI-9435
A bit of context:
We made the choice internally of not migrating unused calculated measures from 5.0 to 5.1, because it goes from an Atoti UI saved calculated measure to an Atoti server cube calculated measure.
As the name suggests, each Atoti server calculated measure belongs to a specific cube.
And if the 5.0 calculated measure isn't used in any dashboard/widget, we cannot know which cube that is.
So both for this technical reason, and for the sake of cleanup, we thought we could remove the calculated measure altogether.
With the complaint in the ticket, we're going for the alternative of:
The diff may look big but the added logic is quite small, and I just refactored a little bit to be able to test migrating from any version to any version through a pure function:
migrateContentServer.