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

Theme config does not take effect #155

Merged
merged 7 commits into from
Jul 22, 2024
Merged

Conversation

lcduong
Copy link
Contributor

@lcduong lcduong commented Jul 16, 2024

Fixes #137 Theme config does not take effect

Short description of what this resolves:

Theme config does not take effect

Changes proposed in this pull request:

  • Update theme.config.colors with colors data from API response
  • Change color default for theme

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@lcduong
Copy link
Contributor Author

lcduong commented Jul 16, 2024

image

Note: for upload logo file, [urls] [media] must be configured in config file
e.g:
[urls]
media=http://localhost:8443/media/

since /media is expose by server app, so need to config nginx for expose via webapp.

@lcduong lcduong marked this pull request as ready for review July 16, 2024 08:28
@lcduong lcduong marked this pull request as draft July 16, 2024 08:28
@lcduong lcduong marked this pull request as ready for review July 17, 2024 09:00
@lcduong lcduong mentioned this pull request Jul 17, 2024
4 tasks
@@ -53,7 +54,8 @@ def get_closest_zoom_lang(world):
class ZoomViewMixin:
@cached_property
def world(self):
w = get_object_or_404(World, domain=self.request.headers["Host"])
world_domain = re.sub(r":\d+$", "", self.request.headers["Host"])
Copy link
Member

Choose a reason for hiding this comment

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

Please use Django's built-in request.get_host().

@return: theme data of a world
"""
world = get_object_or_404(World, id=kwargs["world_id"])
return Response(WorldSerializer(world).data['config']['theme'])
Copy link
Member

Choose a reason for hiding this comment

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

Please handle KeyError.

@hongquan
Copy link
Member

hongquan commented Jul 17, 2024

I think it is not safe to assume that the API response is always in the shape that the frontend expects (like missing some fields, or different data type).
It is better to go through a "validation step" (using https://docs.superstructjs.org/ or https://zod.dev/). This process will show us (developers) how the API is unmatched, so that we can fix frontend, or backend code easier. But I'm not sure if we should require our frontend developers to apply this practice (I feel it is a bit advance).

In the future, when we can migrate our frontend to Vue 3 + TypeScript, this "validation step" will be mandatory, because not only it help developers to find where data is wrong, but also provide type information (TypeScript) for IDE to produce good autocomplete.

try:
world = get_object_or_404(World, id=kwargs["world_id"])
return Response(WorldSerializer(world).data['config']['theme'])
except KeyError:
Copy link
Member

Choose a reason for hiding this comment

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

If KeyError happens, it means that us, the website owner, did not prepare enough data, or misconfigured the website. We need to do logger.error to inform our team.

For end user, we can throw error 503 to tell that this error is our side, temporary and can work again in the future.

world = get_object_or_404(World, id=kwargs["world_id"])
return Response(WorldSerializer(world).data['config']['theme'])
except KeyError:
logger.error(f"error happened when trying to get theme data of world: " + kwargs["world_id"])
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect use of logger. Check guide in fossasia/eventyay-talk#130

@mariobehling mariobehling merged commit fb5bfe6 into fossasia:development Jul 22, 2024
1 of 7 checks passed
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

Successfully merging this pull request may close these issues.

Theme config does not take effect
3 participants