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

ensure shape key is the upper-cased id #1442

Merged
merged 1 commit into from
Oct 11, 2024
Merged

Conversation

meguiraun
Copy link
Contributor

fixing an issue when the shape id reported by the server is lower cased (p1), but the UI expects upper cased (P1)

how to reproduce:
1 - Create a new point in the ui, check the state and you will see the ui state as "P1"
2 - (you can run data collections on this)
3 - Refresh the page, check the state and you will see the ui state as "p1"
4 - Now you cannot run tasks as the addTask method will try to access the shape with "P1" key

If you enable and the. disable the 3click centring, you would end up with both keys ("p1" and "P1") in the state (???)

I tried to hack my way into the to_camelmethod for achieving this but I got no luck.

@meguiraun meguiraun added the bug label Oct 9, 2024
@marcus-oscarsson
Copy link
Member

Thanks, @meguiraun. Seems like a viable fix. How about looking into why its uppercase to begin with. Would it make sense to make sure that its lower case to start with and then simply rely on to_camel completely, just a thought ?

@meguiraun
Copy link
Contributor Author

I assumed uppercase was what it should be, as set by the UI... I can have a look at the reducer...

@meguiraun
Copy link
Contributor Author

we have already a similar approach here: https://github.com/mxcube/mxcubeweb/blob/develop/mxcubeweb/core/components/sampleview.py#L147

and the tasks are added based on the shape ID (capitalized), see https://github.com/mxcube/mxcubeweb/blob/develop/ui/src/components/Tasks/DataCollection.jsx#L53

I feel this MR is the less intrusive approach

@marcus-oscarsson
Copy link
Member

Ok, I think you are right. It seems like this is the approach that is used elsewhere in the code base. It is a bit error prone as we need to remember to extract the ID, I guess thats why we have this problem ;) . Lets keep it as this for the moment then.

@marcus-oscarsson marcus-oscarsson merged commit 668af1e into develop Oct 11, 2024
19 checks passed
@marcus-oscarsson marcus-oscarsson deleted the fix_shape_key_id branch October 11, 2024 11:53
@marcus-oscarsson marcus-oscarsson restored the fix_shape_key_id branch October 14, 2024 08:11
Comment on lines +269 to +272
# shape key comes case lowered from the to_camel (2dp1), this breaks ui
# lest ensure it upper case by only came casing the dict data
_shape = {shape.id: to_camel(shape_dict[shape.id])}
return {"shapes": _shape}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @meguiraun, we tested your changes and run into the following issue when there are no shapes in the sample_view hwr:
Screenshot from 2024-10-14 10-23-07
Another issue is, when we there are multiple shapes in sample_view, it seems to return a dict that only contains one element, I would suggest the following change:
_shape[shape.id] = to_camel(shape_dict[shape.id])
and initializing _shape before the for-loop, if this sounds correct to you i can make a new PR for this, as this one has already been accepted?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe a more elegant way would be to just change the update function of shape_dict so that the function would look like this:

    def get_shapes(self):
        shape_dict = {}

        for shape in HWR.beamline.sample_view.get_shapes():
            s = shape.as_dict()
            # shape key comes case lowered from the to_camel (2dp1), this breaks ui
            # lest ensure it upper case by only came casing the dict data
            shape_dict.update({shape.id: to_camel(s)})

        return {"shapes": shape_dict}

Which would remove the necessity of an additional dict object (_shape)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, please go ahead with a PR

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

Successfully merging this pull request may close these issues.

3 participants