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

Fix issues with URL #33

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Fix issues with URL #33

wants to merge 8 commits into from

Conversation

Sae126V
Copy link
Member

@Sae126V Sae126V commented Feb 27, 2025

Resolves GT-687

- We need this currently because it is breaking the current code flow and returning an internal server error.
- In the DB we have few sites with dot in their SiteName
- Perhaps, Nice-To-Have is either from the codebase we restrict showing the sites with dot(Unnecessary load to fetch) or restrict the sites to make their way to DB.
- Removed the restriction on grid/<SiteName> fetching the data with
  one year ONLY.
- urls.py: We need this re_path() for grid and cloud to avoid getting
  the message saying "Could not resolve URL for hyperlinked
  relationship"
- We need Django Rest Framework fetching the data based on the
  SiteName - We need this for couple of reasons
    1) To avoid showing the data with the URL containing pk(As a Number like) in it
    2) To fetch the data using SiteName as primary lookup field.
- Sometimes gridsync/<SiteName> fetches the whole list again,
  ignoring the lookup_up field because we were using list()
- We are using get_serializer() with `many=True` to handle complex
  data which is returning multiple objects with the single siteName.
- Fixed issue with HyperlinkedModelSerializer, it seems to NOT able
  to work with two lookup_fields before.
- Simplified the split() functionality.

Desc

from rest_framework.reverse import reverse
Copy link
Member

Choose a reason for hiding this comment

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

Don't ditch the spacing. 3rd party imports should be separated from local application imports. https://peps.python.org/pep-0008/#imports

Copy link
Member Author

@Sae126V Sae126V Feb 27, 2025

Choose a reason for hiding this comment

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

Ahh!.. Missed this. I will add this, adrian. Done

@@ -22,6 +22,12 @@ class Meta:
'updated'
)

# Sitename substitutes pk
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean substitutes for the pk?

Copy link
Member Author

@Sae126V Sae126V Feb 27, 2025

Choose a reason for hiding this comment

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

Adrian, If the question is regarding Do you mean substitutes for the pk? - YES. It is just sitename substitutes pk because of the lookup_field. Because lookup_field default is look at the pk

    # Sitename substitutes pk
    lookup_field = 'SiteName'


class GridSiteSyncSubmitHSerializer(serializers.HyperlinkedModelSerializer):
# Override default format with None so that Python datetime is used as
# ouput format. Encoding will be determined by the renderer and can be
# formatted by a template filter.

# We need this because HyperlinkedModelSerializer seems to NOT able to work with two lookup_fields
class MultipleFieldLookup(serializers.HyperlinkedIdentityField):
Copy link
Member

Choose a reason for hiding this comment

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

An object defined inside an object? Is there somewhere better it could go?

Copy link
Member Author

@Sae126V Sae126V Feb 27, 2025

Choose a reason for hiding this comment

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

MultipleFieldLookup -> This class is ONLY capable to match object instance to its URL representation based on a specific scenario only - The scenario is when SiteName and YearMonth - Which is why I left it live inside the object, Adrian.

I have updated the code now. It make sense.

@@ -178,7 +180,6 @@ def retrieve(self, request, pk=None):
Site,
max(LatestEndTime) AS LatestPublish
FROM VSuperSummaries
WHERE Year=2019
Copy link
Member

Choose a reason for hiding this comment

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

Oh! That's where the issue was with the fetched data. 🤦🏻

Good spot!

Copy link
Member

Choose a reason for hiding this comment

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

(It was meant to be a temporary change to improve performance during testing.)

@Sae126V
Copy link
Member Author

Sae126V commented Feb 27, 2025

I have updated the code, Adrian. Thanks

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

Successfully merging this pull request may close these issues.

2 participants