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

📝🧹 Domain: Adds docstrings for the WithinLocation mixin and has_encrypted attribute #1625

Merged
merged 7 commits into from
Jul 4, 2023

Conversation

rosschapman
Copy link
Contributor

@rosschapman rosschapman commented Jun 30, 2023

Contributors! Help me understand if these docs may be superfluous or tautological. I'm on a spirited adventure toward completing issue #1622 listening to small-time, regional punk rock/pop -- not my typical tranquil soundscape for procedural doc-writing 😅 .

My goal here was to synthesize a couple learnings around Convene's bespoke code and third-party deps in a way that would be beneficial to other contributors who are uninitiated to the codebase and/or unfamiliar with Ruby/Rails to increase developer ease-of-use.

app/models/utility.rb Outdated Show resolved Hide resolved
@rosschapman rosschapman requested a review from a team June 30, 2023 22:41
Copy link
Member

@anaulin anaulin left a comment

Choose a reason for hiding this comment

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

This looks useful and accurate to me. Thank you!

(opinions about our location nonsense omitted to protect the innocent, as well as the not-so-innocent)

Copy link
Member

@zspencer zspencer left a comment

Choose a reason for hiding this comment

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

Thank you! I think this is an improvement.

Some additional context, some or none of which may also be useful:

  • If we don't use [space, room, :a, :b, :c] with form_with or link_to, then we would not have a seam for switching between domain-based spaces and non-domain based spaces.
  • It would be nice if we were able to pull the code that leverages the location bits into the same place, so it seems less like an absurd wart and more like a thing that solves the discrete problem of allowing us to support spaces on custom domains or on the neighborhood domain.
  • The arguments for or against manually typing [space, room, marketplace, :checkout] vs marketplace.location(child: :checkout) are ultimately 6 in one, a half-dozen in the other. Feel free to do it however you want in the code you're writing;
  • Perhaps if we called it ResourceLocation instead of WithinLocation it would feel less clunky; or if it lived in the ViewComponent layer so it's not at the Model layer...
  • If we want to get rid of the location abstraction, we can; we just have to find every call and replace it with what the appropriate arguments for polymorphic_url should be.
  • There is also probably a way to make it so writing space_room_marketplace_checkout_path(space, room, marketplace) will not break the form or link on custom domains that will be more "normal" looking to people who are averse to polymorphic_url.

TL/DR: the Model#location stuff is there to A) support custom domains and B) make resourceful routing more convenient. If there is a smerter way to deal with it; please feel free to replace it with the smert version.

Co-authored-by: Zee <50284+zspencer@users.noreply.github.com>
@rosschapman
Copy link
Contributor Author

rosschapman commented Jul 4, 2023

If we don't use [space, room, :a, :b, :c] with form_with or link_to, then we would not have a seam for switching between domain-based spaces and non-domain based spaces.

I think that makes sense. Though you might have to show me exactly in the helpers how that's "enforced".

It would be nice if we were able to pull the code that leverages the location bits into the same place...

Yeah, definitely a good cleanup task for later!

The arguments for or against manually typing [space, room, marketplace, :checkout] vs marketplace.location(child: :checkout) are ultimately 6 in one, a half-dozen in the other. Feel free to do it however you want in the code you're writing

Except for the added maintenance cost of defining location for each resource, sure. But likely balanced by the convenience of the mental separation for a developer doing view work. Like, if I'm just adding a link in a view, I don't need to consider the entire domain object graph (if I can trust my Resource modeling is correct 😅 ). It's probably too early to tell what the impact of this abstraction may be? If it was JavaScript Land I would advise against writing abstractions against routers, which are fairly fungible parts.

Perhaps if we called it ResourceLocation instead of WithinLocation it would feel less clunky; or if it lived in the ViewComponent layer so it's not at the Model layer...

Yeah...the Resource... root would be helpful in this type of module name mold!

@rosschapman rosschapman merged commit 157c5ee into main Jul 4, 2023
@rosschapman rosschapman deleted the prep-issue-1622 branch July 4, 2023 02:25
@zspencer zspencer changed the title 📝 Adds docstrings for the WithinLocation mixin and has_encrypted attribute 📝🧹 Domain: Adds docstrings for the WithinLocation mixin and has_encrypted attribute Jul 11, 2023
@zspencer zspencer added 🧹 refactor Includes non-behavioral changes 📝 documentation Make words more better good labels Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📝 documentation Make words more better good 🧹 refactor Includes non-behavioral changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants