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

#938 Expand fixtures to produce human relatable results #977

Merged
merged 7 commits into from
Sep 24, 2024

Conversation

andrewtavis
Copy link
Member

@andrewtavis andrewtavis commented Sep 21, 2024

Contributor checklist


Description

Various fixes to the fixtures :)

Related issue

Copy link
Contributor

github-actions bot commented Sep 21, 2024

Thank you for the pull request!

The activist team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Development rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you!

Maintainer checklist

  • The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution

    • The contributor's name and icon in remote commits should be the same as what appears in the PR
    • If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local activist repo
  • The TypeScript and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The Playwright end to end and Zap penetration tests have been ran and are passing (if necessary)

  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@andrewtavis andrewtavis changed the title #938 Exapnd fixtures to produce human relatable results #938 Expand fixtures to produce human relatable results Sep 21, 2024
Copy link

netlify bot commented Sep 21, 2024

Deploy Preview for activist-org canceled.

Name Link
🔨 Latest commit 37dc259
🔍 Latest deploy log https://app.netlify.com/sites/activist-org/deploys/66f34f7215f22e000818d0a9

@andrewtavis
Copy link
Member Author

Note: Before this is merged, I will add in directions for onboarding new users. Navigate to XYC in order to see organizations, etc.

Copy link
Member

@wkyoshida wkyoshida left a comment

Choose a reason for hiding this comment

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

🚀

Comment on lines 38 to 41
num_users = options.get("users")
num_orgs_per_user = options.get("orgs_per_user")
num_groups_per_org = options.get("groups_per_org")
num_events_per_org = options.get("events_per_org")
Copy link
Member

Choose a reason for hiding this comment

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

The errors with the pr_ci_backend workflow at least, appear to be due to the above 4 possibly being either an int or None (despite the defaults that are specified in the parser arguments on lines L32-L35).

Would doing for instance options["users"] as opposed to options.get("users") still work? Or at least specifying the default here as in options.get("users", 10) so that the .get() calls don't possibly return None by default.

Copy link
Collaborator

@to-sta to-sta Sep 22, 2024

Choose a reason for hiding this comment

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

The get method return type is int | None. Because the method will return None instead of throwing a KeyError, if the key does not exist. We can just use square bracket notation options["users"] and the errors are gone 😃.

@andrewtavis
Copy link
Member Author

I still don't know what's up with the frontend error, but is what it is for now. I'm hoping that it goes away when we update the frontend dependencies, which is needed at this point.

df04381 Adds in a devMode Pinia store, with devMode.active being used to see if loclhost:3000 is in the route. This is the same technique used for disabling the captcha in dev mode, but now universal. With this I've gone in and changed the landing page in dev mode to replace the Request access button with View organizations and View events. I also figured out the line clamp for search result cards.

The last thing I want to do here is play around with the events page a bit to make sure that we're loading in events for that, and that clicking on one gets to a fully populated page from the backend. I won't finish all the hydration, but I think that it's important to make it easier for people who are working on events 🗺️

@andrewtavis
Copy link
Member Author

Frontend error's still a problem, but I'm going to merge. Tons of changes included in here at this point 😊 New ones are that the events are also being listed, and the work for the backend to do this has also been gotten to. Likely are some minor things that need to get taken care of. Please let me know if you see something! Also feel free to send along a PR with any needed changes :)

@andrewtavis andrewtavis merged commit 14a90f6 into main Sep 24, 2024
6 of 7 checks passed
@andrewtavis andrewtavis deleted the 938-expand-fixtures branch September 24, 2024 23:51
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.

3 participants