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

Grouping feature #719

Merged
merged 35 commits into from
Sep 18, 2023
Merged

Conversation

iamhks
Copy link
Contributor

@iamhks iamhks commented Jul 26, 2023

Summary

Updating the existing Grouping Feature

@chaitanya-cloudknox
Copy link

@iamhks if id is string can you use uuid to generate default values during migration?
Cc @tuxology

@iamhks
Copy link
Contributor Author

iamhks commented Jul 27, 2023

@iamhks if id is string can you use uuid to generate default values during migration? Cc @tuxology

@chaitanya-cloudknox If you see the intial.py file
image
We do not have the id defined, it's auto created by django.
image
I can add let say: '1' over there and it generates the migrations file. But when I try to migrate, it gives me error saying for id you have two default values which is not possible.

@kamthamc
Copy link
Collaborator

@iamhks will this work https://docs.djangoproject.com/en/4.2/howto/writing-migrations/#migrations-that-add-unique-fields ?

I think you can add id value even if its auto created

@iamhks
Copy link
Contributor Author

iamhks commented Jul 27, 2023

@iamhks will this work https://docs.djangoproject.com/en/4.2/howto/writing-migrations/#migrations-that-add-unique-fields ?

I think you can add id value even if its auto created

@kamthamc @chaitanya-cloudknox I tried that too:

root@985d47cce9db:/zubhub_backend/zubhub# python manage.py migrate
SystemCheckError: System check identified some issues:

ERRORS:
creators.CreatorGroup: (models.E004) 'id' can only be used as a field name if the field also sets 'primary_key=True'.

And if I set primary key to true for id:

Running migrations:
  Applying creators.0011_auto_20230727_1143...Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
psycopg2.errors.InvalidTableDefinition: multiple primary keys for table "creators_creatorgroup" are not allowed


The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "manage.py", line 21, in <module>
    main()
  File "manage.py", line 17, in main
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/__init__.py", line 419, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python3.8/site-packages/django/core/management/__init__.py", line 413, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/base.py", line 354, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/base.py", line 398, in execute
    output = self.handle(*args, **options)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/base.py", line 89, in wrapped
    res = handle_func(*args, **kwargs)
  File "/usr/local/lib/python3.8/site-packages/django/core/management/commands/migrate.py", line 244, in handle
    post_migrate_state = executor.migrate(
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/executor.py", line 117, in migrate
    state = self._migrate_all_forwards(state, plan, full_plan, fake=fake, fake_initial=fake_initial)
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/executor.py", line 147, in _migrate_all_forwards
    state = self.apply_migration(state, migration, fake=fake, fake_initial=fake_initial)
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/executor.py", line 227, in apply_migration
    state = migration.apply(state, schema_editor)
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/migration.py", line 126, in apply
    operation.database_forwards(self.app_label, schema_editor, old_state, project_state)
  File "/usr/local/lib/python3.8/site-packages/django/db/migrations/operations/fields.py", line 104, in database_forwards
    schema_editor.add_field(
  File "/usr/local/lib/python3.8/site-packages/django/db/backends/base/schema.py", line 508, in add_field
    self.execute(sql, params)
  File "/usr/local/lib/python3.8/site-packages/django/db/backends/base/schema.py", line 145, in execute
    cursor.execute(sql, params)
  File "/usr/local/lib/python3.8/site-packages/django/db/backends/utils.py", line 98, in execute
    return super().execute(sql, params)
  File "/usr/local/lib/python3.8/site-packages/django/db/backends/utils.py", line 66, in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
  File "/usr/local/lib/python3.8/site-packages/django/db/backends/utils.py", line 75, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/usr/local/lib/python3.8/site-packages/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
  File "/usr/local/lib/python3.8/site-packages/django/db/utils.py", line 90, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/usr/local/lib/python3.8/site-packages/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: multiple primary keys for table "creators_creatorgroup" are not allowed

@iamhks iamhks changed the base branch from master to grouping-feature July 28, 2023 02:36
Copy link

@chaitanya-cloudknox chaitanya-cloudknox left a comment

Choose a reason for hiding this comment

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

Nice work. Most of it LGTM. some minor comments.

  • Translations are missing in some files
  • Translations are off in some files
  • When setting a state Promise is not required.
  • When using Promise.all, check if you need Promise.allSettled/Promise.all
  • Try to avoid !important in css

@@ -137,40 +137,64 @@ def __str__(self):


class CreatorGroup(models.Model):
creator = models.OneToOneField(
Creator, on_delete=models.CASCADE, primary_key=True)
id = models.UUIDField(

Choose a reason for hiding this comment

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

@tuxology is there a auto formatter for python? The indentation/styling seems to be off


class Meta:
model = CreatorGroup
fields = ('groupname', 'description', 'members', 'created_on', 'projects_count')

Choose a reason for hiding this comment

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

i think group_name is more readable

zubhub_backend/zubhub/creators/views.py Outdated Show resolved Hide resolved
zubhub_frontend/zubhub/package.json Outdated Show resolved Hide resolved
zubhub_frontend/zubhub/src/App.js Outdated Show resolved Hide resolved
return (
<Box marginY={6}>
<FormControl fullWidth>
<label className={commonClasses.commonClasses}>

Choose a reason for hiding this comment

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

Missing translations in the file

const [selectedProjects, setSelectedProjects] = useState([]);
const handleSetState = (obj) => {
if (obj) {
Promise.resolve(obj).then((obj) => {

Choose a reason for hiding this comment

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

promise is not required

Choose a reason for hiding this comment

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

And missing translations in file


if (loading) {
return <LoadingPage />;
} else if (profile && Object.keys(profile).length > 0) {

Choose a reason for hiding this comment

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

simplify this for readability.

if (loading) {
 return loading
} else if(!profile || !profile.username) {
return "something"
}
return (<div />)

return res
.then(res => {
if (res.project && res.project.title) {
if (projects[0]) {

Choose a reason for hiding this comment

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

instead check Array.isArray(projects)

color="textPrimary"
className={classes.titleStyle}
>
{t('Edit Team Info')}

Choose a reason for hiding this comment

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

looks like translation is off in the page

@iamhks iamhks marked this pull request as ready for review August 14, 2023 13:59
@tuxology tuxology added the enhancement New feature or request label Aug 14, 2023
@iamhks iamhks changed the base branch from grouping-feature to master August 25, 2023 14:12
@tuxology
Copy link
Member

tuxology commented Sep 2, 2023

Hey @iamhks Thanks a lot for this PR. Here are a few comments based on initial testing which I was able to do finally. I had to use a clean VM, do make init to ensure all migrations ran. I also was able to see the integrated UI with new sidebar! Cool stuff. Just to make sure celery is running fine, I also made these changes #730 but that should not affect.

Blockers

  • Can't proceed to "Next" if you keep the admin empty. Ideally, if admin is empty, the current user should be assigned as admin. Upon investigating, issue with props
    image
    image

  • When you assign an admin and try to proceed, the next page shows up where you can select a project. Upon project selection, when you click "Next", you get an "IntegrityError"

image

I could not proceed and review other parts of the feature since could not move past this.

UX Bugs

  • In the Invite team members page, the choice of "card" element is incorrect as it doesn't match the UX in Figma design. There are issues in spacing also. You can refer to figma design to ensure this is correct.

Improvements

@tuxology
Copy link
Member

tuxology commented Sep 4, 2023

Thanks for the last fixes @iamhks. I was able to proceed with more testing now and have a set of new bugs/improvements and we would be almost ready for a release 👍

Blockers

  • For /all/teams, the team card is not-clickable. Also refer to Figma for font spacing/padding and Follow Button design for the teams card while you are fixing this. Eg. team card font should not be a hyperlink right now
  • Clicking on "Add Projects" button takes you to the /create-team view instead of a view where you can select your projects to be added

UX Bugs

  • If you do not select a project or project doesn't exist and you click "Create Team" - we get no feedback that project is required for creating a team. Error exists in response where projects can't be null. Maybe we should show error in UI as error message "Could not create a team. Please create a project or select a project from the list"

Screenshot 2023-09-01 173203

  • When creating teams, padding in "Project selection" section is incorrect. Review the project cards and their container in My Profile for quick reference
  • For /all/teams the title font size and position could be adjusted. Refere "My Projects" for reference for now instead of Figma.
  • Padding and spacing of sections is inconsistent for /teams/$NAME page. Esp. for about Us section the right corners radius is zero so its not consistent. Also title gets cropped.

Screenshot 2023-09-04 141158

  • Use outline button for "Follow" button on team cards. Also ensure size is not too big as its currently. Refer Figma
  • When editing a team, upon editing text and clicking edit, the page should be redirected to the team details view /teams/$NAME. In addition the text for team deletion says any "job or talent" will be deleted. Remove this line - it doens't make sense. Do we mean to say the project will be deleted? Do team associated projects get deleted when deleting a team?

image

Improvements

  • After team creation is done, we do not get a modal indicating we created a team successfuly. Refer Figma
  • For /teams/all in Figma we also had a distinction between all teams on platform vs Your teams, In the same view, the teams card text/color/spacing is not same as in Figma
  • In My Profile, For your own team, we should possibly not show a Follow button. If you are viewing someone else's profile, maybe showing teams makes sense.

@tuxology
Copy link
Member

Leaving an update here as we merge this now for future reference:

This is an exciting upcoming feature 🎉

Copy link
Member

@tuxology tuxology left a comment

Choose a reason for hiding this comment

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

Finally ready after lots of reviews 🎉

@tuxology tuxology merged commit 7aa0021 into unstructuredstudio:master Sep 18, 2023
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.

4 participants